Re: [Xen-devel] [PATCH 4/6] x86/xstate: Fix latent bugs in expand_xsave_states()

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 12.09.16 at 14:29, <andrew.cooper3@xxxxxxxxxx> wrote:
> On 12/09/16 12:41, Jan Beulich wrote:
>>>>> On 12.09.16 at 11:51, <andrew.cooper3@xxxxxxxxxx> wrote:
>>> @@ -205,11 +222,9 @@ void expand_xsave_states(struct vcpu *v, void *dest, 
>>> unsigned int size)
>>>  
>>>          if ( src )
>>>          {
>>> -            ASSERT((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>> +            BUG_ON((xstate_offsets[index] + xstate_sizes[index]) <= size);
>>>              memcpy(dest + xstate_offsets[index], src, xstate_sizes[index]);
>>>          }
>>> -        else
>>> -            memset(dest + xstate_offsets[index], 0, xstate_sizes[index]);
>> So I have difficulty seeing why this memset() wasn't sufficient: It
>> precisely covers for the respective component being in default
>> state.
> 
> No it doesn't.  The loop skips over all bits which are not set in xstate_bv.

Well, yes, I had corrected myself in the following sentence, resulting
in me just asking for the commit message to get clarified.

> I had (erroneously) come to the conclusion that the "if ( src )" check
> only caught the case where we had bad comp_offsets[] information, but
> rereading the logic, that case would actually corrupt the legacy SSE header.
> 
> Overall, it turns out that the "if ( src )" is unconditionally taken.

Oh, I see (same applies to my then wrong comment on patch 6):
We iterate over xstate_bv here, and components with their flag
set in xstate_bv won't see NULL coming back from get_xsave_addr().
I'm sorry for the noise then.

Jan


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