Re: [Xen-devel] [PATCH for-4.5 RFC v2] x86/HVM: Unconditionally crash guests on repeated vmentry failures

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 27/11/14 11:16, Jan Beulich wrote:
>>>> On 27.11.14 at 11:26, <tim@xxxxxxx> wrote:
>> At 08:42 +0000 on 27 Nov (1417074133), Jan Beulich wrote:
>>>>>> On 26.11.14 at 18:43, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>> My v1 patch only fixes the VMX side of things.  SVM doesn't explicitly
>>>> identify a failed vmentry and lets it fall into the default case of the
>>>> vmexit handler.  As such, with v1, the infinite loop still affects AMD
>>>> hardware.
>>> Right; I should have said "something along the lines of v1". An SVM
>>> part is needed, but that should probably extend beyond what you
>>> proposed in v2: There are a number of "goto exit_and_crash"
>>> statements ahead of where you place your addition. I think they
>>> all need to be treated similarly.
>>>
>>> I therefore think we should revert the VMX part of 28b4baacd5
>>> and make SVM behavior consistent with what results for VMX:
>>> Crash the guest unconditionally on VMEXIT_INVALID, without
>>> looking for recurring VM entry failures. See below/attached.
>>>
>>> Jan
>>>
>>> x86/HVM: prevent infinite VM entry retries
>>>
>>> This reverts the VMX side of commit 28b4baac ("x86/HVM: don't crash
>>> guest upon problems occurring in user mode") and gets SVM in line with
>>> the resulting VMX behavior. This is because Andrew validly says
>>>
>>> "A failed vmentry is overwhelmingly likely to be caused by corrupt
>>>  VMC[SB] state.  As a result, injecting a fault and retrying the the
>>>  vmentry is likely to fail in the same way."
>>>
>>> Reported-by: Andrew Cooper <andrew.cooper3@xxxxxxxxxx>
>>> Signed-off-by: Jan Beulich <jbeulich@xxxxxxxx>
>> Looking at the SVM side, AFAICT you're trying to filter out
>> VMEXIT_INVALID exits that couldn't be handled by nested SVM, but I
>> think it's fine just to always crash on nested-SVM failures: we know
>> the guest wasn't in user mode because it successfully executed VMRUN.
>> And looking at it, the other users of that label are for unexpected
>> debugging exits, which can't be caused by the guest userspace either.
>>
>> So how about this for the SVM side, reverting to crashing for
>> everything except new, unsupported exit types?
> Generally a good idea, but there are two paths to exit_and_crash
> (for VMEXIT_EXCEPTION_DB and VMEXIT_EXCEPTION_BP) which I
> think would better crash only conditionally.

I would agree - these two should only be conditional crashes. If the
intercepts are enabled, userspace is perfectly capable of generating the
conditions behind the back of the kernel.

~Andrew

>
>> --- a/xen/arch/x86/hvm/svm/svm.c
>> +++ b/xen/arch/x86/hvm/svm/svm.c
>> @@ -2675,16 +2675,18 @@ void svm_vmexit_handler(struct cpu_user_regs *regs)
>>          break;
>>  
>>      default:
>> -    exit_and_crash:
>>          gdprintk(XENLOG_ERR, "unexpected VMEXIT: exit reason = %#"PRIx64", 
>> "
>>                   "exitinfo1 = %#"PRIx64", exitinfo2 = %#"PRIx64"\n",
>>                   exit_reason, 
>>                   (u64)vmcb->exitinfo1, (u64)vmcb->exitinfo2);
>> -        if ( vmcb_get_cpl(vmcb) )
>> +        if ( vmcb_get_cpl(vmcb) ) {
>>              hvm_inject_hw_exception(TRAP_invalid_op,
>>                                      HVM_DELIVER_NO_ERROR_CODE);
>> -        else
>> -            domain_crash(v->domain);
>> +            break;
>> +        }
>> +        /* else fall through */
>> +    exit_and_crash:
>> +        domain_crash(v->domain);
>>          break;
>>      }
> Additionally this re-arrangement would make some domain_crash()
> invocations "silent" (i.e. no other accompanying message), but that's
> of course easy to fix.
>
> And finally, if doing it that way we might better remove the
> exit_and_crash label altogether and call domain_crash() directly
> in the places we mean it to be called.
>
> Jan
>


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel