Re: [PATCH v2 6/6] x86/xstate: Fix latent bugs in compress_xsave_states()

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

On 13/09/16 09:27, Jan Beulich wrote:
>>>> On 12.09.16 at 18:21, <andrew.cooper3@citrix.com> wrote:
>> compress_xsave_states() mustn't read xstate_bv or xcomp_bv before first
>> confirming that the input buffer is large enough.  It also doesn't cope with
>> compressed input.  Make all of these problems the callers responsbility to
>> ensure.
>>
>> Simplify the decompression logic by inlining get_xsave_addr().  As xstate_bv
>> is previously validated, dest won't ever been NULL.
>>
>> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Reviewed-by: Jan Beulich <jbeulich@suse.com>
> albeit, as expressed before I would prefer ...
>
>>  void compress_xsave_states(struct vcpu *v, const void *src, unsigned int size)
>>  {
>>      struct xsave_struct *xsave = v->arch.xsave_area;
>> +    void *dest;
>>      uint16_t comp_offsets[sizeof(xfeature_mask)*8];
>> -    u64 xstate_bv = ((const struct xsave_struct *)src)->xsave_hdr.xstate_bv;
>> -    u64 valid;
>> +    u64 xstate_bv, valid;
>>  
>> -    ASSERT(!xsave_area_compressed(src));
>> +    BUG_ON(!v->arch.xcr0_accum);
>> +    BUG_ON(size != xstate_ctxt_size(v->arch.xcr0_accum));
>> +    BUG_ON(xsave_area_compressed(src));
> ... if at least the ASSERT() remained one.

Hmm fine.  At least that is only data corruption issue, not an buffer
boundary issue.

~Andrew

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