Re: [PATCH v2 23/28] target/i386: Honor xfeatures in xrstor_sigcontext

[Richard Henderson <richard.henderson@linaro.org>]

On 4/9/24 08:09, Richard Henderson wrote:
> On 4/8/24 21:44, Paolo Bonzini wrote:
>>> +        /*
>>> +         * Restore the features indicated in the frame, masked by
>>> +         * those currently enabled.  Re-check the frame size.
>>> +         * ??? It is not clear where the kernel does this, but it
>>> +         * is not in check_xstate_in_sigframe, and so (probably)
>>> +         * does not fall back to fxrstor.
>>> +         */
>>
>> I think you're referring to this in __fpu_restore_sig?
>>
>>          if (use_xsave()) {
>>                  /*
>>                   * Remove all UABI feature bits not set in user_xfeatures
>>                   * from the memory xstate header which makes the full
>>                   * restore below bring them into init state. This works for
>>                   * fx_only mode as well because that has only FP and SSE
>>                   * set in user_xfeatures.
>>                   *
>>                   * Preserve supervisor states!
>>                   */
>>                  u64 mask = user_xfeatures | xfeatures_mask_supervisor();
>>
>>                  fpregs->xsave.header.xfeatures &= mask;
>>                  success = !os_xrstor_safe(fpu->fpstate,
>>                                            fpu_kernel_cfg.max_features);
>>
>> It is not masking against the user process's xcr0, but qemu-user's xcr0
>> is effectively user_xfeatures (it's computed in x86_cpu_reset_hold() and
>> will never change afterwards since XSETBV is privileged).
> 
> No, I'm talking about verifying that the xstate_size is large enough.
> 
> In check_xstate_in_sigframe,
> 
>          if (fx_sw->magic1 != FP_XSTATE_MAGIC1 ||
>              fx_sw->xstate_size < min_xstate_size ||
> 
> Check for the trivially too small case (fxregs + header).
> 
>              fx_sw->xstate_size > current->thread.fpu.fpstate->user_size ||
> 
> Check for the trivially too large case (presumably this is to catch stupidly large values, 
> assuming garbage).
> 
>              fx_sw->xstate_size > fx_sw->extended_size)
> 
> Check for trivial mismatch between fields.
> 
>                  goto setfx;
> 
> But there's a middle case: if xfeatures > 3, then xstate_size must be > min_xstate_size.
> 
> I know that the kernel will handle any #GP in xrstor_from_user_sigframe, but there doesn't 
> seem to be a real check for reading garbage beyond the given size.

Oh, I meant to mention, following the

     __fpu_restore_sig:
         user_xfeatures = fx_sw_user.xfeatures;
         ...
         if (!ia32_fxstate)
             restore_fpregs_from_user(..., user_xfeatures, ...)

     restore_fpregs_from_user(..., xrestore, ...)
         xrestore &= fpu->fpstate->user_xfeatures;
         __restore_fpregs_from_user(..., xrestore, ...)

path.


r~