Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/i386/signal.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
index d015fe520a..fd09c973d4 100644
--- a/linux-user/i386/signal.c
+++ b/linux-user/i386/signal.c
@@ -612,6 +612,7 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
     struct target_fpx_sw_bytes *sw = (void *)&fxstate->sw_reserved;
     uint32_t magic1, magic2;
     uint32_t extended_size, xstate_size, min_size, max_size;
+    uint64_t xfeatures;
 
     switch (fpkind) {
     case FPSTATE_XSAVE:
@@ -628,10 +629,25 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
             xstate_size > extended_size) {
             break;
         }
+
+        /*
+         * 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.
+         */
+        xfeatures = tswap64(sw->xfeatures) & env->xcr0;
+        min_size = xsave_area_size(xfeatures, false);
+        if (xstate_size < min_size) {
+            return false;
+        }
+
         if (!access_ok(env_cpu(env), VERIFY_READ, fxstate_addr,
                        xstate_size + TARGET_FP_XSTATE_MAGIC2_SIZE)) {
             return false;
         }
+
         /*
          * Check for the presence of second magic word at the end of memory
          * layout. This detects the case where the user just copied the legacy
@@ -644,7 +660,8 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
         if (magic2 != FP_XSTATE_MAGIC2) {
             break;
         }
-        cpu_x86_xrstor(env, fxstate_addr, -1);
+
+        cpu_x86_xrstor(env, fxstate_addr, xfeatures);
         return true;
 
     default:
-- 
2.34.1

On 4/9/24 07:02, Richard Henderson wrote:
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/i386/signal.c | 19 ++++++++++++++++++-
>   1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
> index d015fe520a..fd09c973d4 100644
> --- a/linux-user/i386/signal.c
> +++ b/linux-user/i386/signal.c
> @@ -612,6 +612,7 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
>       struct target_fpx_sw_bytes *sw = (void *)&fxstate->sw_reserved;
>       uint32_t magic1, magic2;
>       uint32_t extended_size, xstate_size, min_size, max_size;
> +    uint64_t xfeatures;
>   
>       switch (fpkind) {
>       case FPSTATE_XSAVE:
> @@ -628,10 +629,25 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
>               xstate_size > extended_size) {
>               break;
>           }
> +
> +        /*
> +         * 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).

Paolo

> +        xfeatures = tswap64(sw->xfeatures) & env->xcr0;
> +        min_size = xsave_area_size(xfeatures, false);
> +        if (xstate_size < min_size) {
> +            return false;
> +        }
> +
>           if (!access_ok(env_cpu(env), VERIFY_READ, fxstate_addr,
>                          xstate_size + TARGET_FP_XSTATE_MAGIC2_SIZE)) {
>               return false;
>           }
> +
>           /*
>            * Check for the presence of second magic word at the end of memory
>            * layout. This detects the case where the user just copied the legacy
> @@ -644,7 +660,8 @@ static bool xrstor_sigcontext(CPUX86State *env, FPStateKind fpkind,
>           if (magic2 != FP_XSTATE_MAGIC2) {
>               break;
>           }
> -        cpu_x86_xrstor(env, fxstate_addr, -1);
> +
> +        cpu_x86_xrstor(env, fxstate_addr, xfeatures);
>           return true;
>   
>       default:

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.


r~

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~