Re: [PATCH] VMX: don't crash processing 'd' debug key

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 07/11/13 14:49, Jan Beulich wrote:
>>>> On 07.11.13 at 15:34, Andrew Cooper <andrew.cooper3@citrix.com> wrote:
>> On 07/11/13 10:44, Jan Beulich wrote:
>>> --- a/xen/arch/x86/hvm/vmx/vmx.c
>>> +++ b/xen/arch/x86/hvm/vmx/vmx.c
>>> @@ -675,7 +675,17 @@ void vmx_get_segment_register(struct vcp
>>>  {
>>>      unsigned long attr = 0, sel = 0, limit;
>>>  
>>> -    vmx_vmcs_enter(v);
>>> +    /*
>>> +     * We may get here in the context of dump_execstate(), which may have
>>> +     * interrupted context switching between setting "current" and
>>> +     * vmx_do_resume() reaching the end of vmx_load_vmcs(). That would make
>>> +     * all the VMREADs below fail if we don't bail right away.
>>> +     */
>>> +    if ( unlikely(!vmx_vmcs_enter(v)) )
>>> +    {
>>> +        memset(reg, 0, sizeof(*reg));
>>> +        return;
>>> +    }
>> What are the implications of this?  All callers unconditionally expect
>> this to succeed, and use the results straight as-are.
>>
>> On the other hand, I am not certain how we could go about dealing with
>> the error.
> The thing is that for all "normal" code paths we won't get there.
> Hence it'll only be that - rather than crashing - no useful
> information will be displayed for the debug key. (And obviously,
> should vmx_vmcs_enter() nevertheless end up failing on a
> "normal" code path, all callers properly looking at the results
> should produce #GP faults to the guest [present bit clear,
> minimal permissions, and segment limit zero].)
>
> Jan
>

Hmm yes.  I certainly cant see any normal codepaths which would get
here, and the crash path certainly needs fixing.

Reviewed-by: Andrew Cooper <andrew.cooper3@citrix.com>