Re: [Xen-devel] [PATCH 2/6] x86/domctl: Fix TOCTOU race with the use of XEN_DOMCTL_getvcpuextstate

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 15:09, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/09/16 13:33, Jan Beulich wrote:
>>>>> On 12.09.16 at 14:02, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> On 12/09/16 12:17, Jan Beulich wrote:
>>>>>>> On 12.09.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>>>> A toolstack must call XEN_DOMCTL_getvcpuextstate twice; first to find the 
>>> size
>>>>> of the buffer to use, and a second time to get the actual content.
>>>>>
>>>>> The reported size was based on v->arch.xcr0_accum, but a guest which 
>>>>> extends
>>>>> its xcr0_accum between the two hypercalls will cause the toolstack to 
>>>>> fail 
>>> the
>>>>> evc->size != size check, as the provided buffer is now too small.  This 
>>>>> causes
>>>>> a hard error during the final phase of migration.
>>>>>
>>>>> Instead, return return a size based on xfeature_mask, which is the maximum
>>>>> size Xen will ever permit.  The hypercall must now tolerate a
>>>>> toolstack-provided buffer which is overly large (for the case where a 
>>>>> guest
>>>>> isn't using all available xsave states), and should write back how much 
>>>>> data
>>>>> was actually written into the buffer.
>>>> To be honest, I'm of two minds here. Part of me thinks this is an
>>>> okay change. However, in particular ...
>>>>
>>>>> --- a/xen/arch/x86/domctl.c
>>>>> +++ b/xen/arch/x86/domctl.c
>>>>> @@ -1054,19 +1054,25 @@ long arch_do_domctl(
>>>>>              unsigned int size;
>>>>>  
>>>>>              ret = 0;
>>>>> -            vcpu_pause(v);
>>>>>  
>>>>> -            size = PV_XSAVE_SIZE(v->arch.xcr0_accum);
>>>>>              if ( (!evc->size && !evc->xfeature_mask) ||
>>>>>                   guest_handle_is_null(evc->buffer) )
>>>>>              {
>>>>> +                /*
>>>>> +                 * A query for the size of buffer to use.  Must return 
>>>>> the
>>>>> +                 * maximum size we ever might hand back to userspace, 
>>> bearing
>>>>> +                 * in mind that the vcpu might increase its xcr0_accum 
>>> between
>>>>> +                 * this query for size, and the following query for data.
>>>>> +                 */
>>>>>                  evc->xfeature_mask = xfeature_mask;
>>>>> -                evc->size = size;
>>>>> -                vcpu_unpause(v);
>>>>> +                evc->size = PV_XSAVE_SIZE(xfeature_mask);
>>>>>                  goto vcpuextstate_out;
>>>>>              }
>>>>>  
>>>>> -            if ( evc->size != size || evc->xfeature_mask != 
>>>>> xfeature_mask )
>>>>> +            vcpu_pause(v);
>>>>> +            size = PV_XSAVE_SIZE(v->arch.xcr0_accum);
>>>>> +
>>>>> +            if ( evc->size < size || evc->xfeature_mask != xfeature_mask 
>>>>> )
>>>> ... the relaxation from != to < looks somewhat fragile to me, going
>>>> forward.
>>> What is fragile about it?  It is a very common pattern in general, and
>>> we use it elsewhere.
>> If we get handed too large a buffer, what meaning do you want to
>> assign to the extra bytes? Them being meaningless is just one
>> possible interpretation.
> 
> I am confused, and I think you are too.  There is no meaning to the
> bytes; this is getvcpuextstate, not setvcpuextstate.

Oh, indeed. I'm sorry.

Jan


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