qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Richard Henderson <richard.henderson@linaro.org>
To: Paolo Bonzini <pbonzini@redhat.com>, qemu-devel@nongnu.org
Subject: Re: [PATCH v2 23/28] target/i386: Honor xfeatures in xrstor_sigcontext
Date: Tue, 9 Apr 2024 08:09:17 -1000	[thread overview]
Message-ID: <f67bf3a0-d684-4fe7-af05-bf2c1f273e42@linaro.org> (raw)
In-Reply-To: <ecc1ce60-c31f-423b-8d5e-ba5138323212@redhat.com>

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~


  reply	other threads:[~2024-04-09 18:09 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2024-04-09  5:02 [PATCH for-9.1 v2 00/28] linux-user/i386: Properly align signal frame Richard Henderson
2024-04-09  5:02 ` [PATCH v2 01/28] target/i386: Add tcg/access.[ch] Richard Henderson
2024-04-09  7:09   ` Paolo Bonzini
2024-04-09  5:02 ` [PATCH v2 02/28] target/i386: Convert do_fldt, do_fstt to X86Access Richard Henderson
2024-04-09  7:52   ` Paolo Bonzini
2024-04-09  5:02 ` [PATCH v2 03/28] target/i386: Convert helper_{fbld, fbst}_ST0 " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 04/28] target/i386: Convert do_fldenv " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 05/28] target/i386: Convert do_fstenv " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 06/28] target/i386: Convert do_fsave, do_frstor " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 07/28] target/i386: Convert do_xsave_{fpu, mxcr, sse} " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 08/28] target/i386: Convert do_xrstor_{fpu, " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 09/28] tagret/i386: Convert do_fxsave, do_fxrstor " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 10/28] target/i386: Convert do_xsave_* " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 11/28] target/i386: Convert do_xrstor_* " Richard Henderson
2024-04-09  5:02 ` [PATCH v2 12/28] target/i386: Split out do_xsave_chk Richard Henderson
2024-04-09  5:02 ` [PATCH v2 13/28] target/i386: Add rbfm argument to cpu_x86_{xsave, xrstor} Richard Henderson
2024-04-09  5:02 ` [PATCH v2 14/28] target/i386: Add {hw, sw}_reserved to X86LegacyXSaveArea Richard Henderson
2024-04-09  5:02 ` [PATCH v2 15/28] linux-user/i386: Drop xfeatures_size from sigcontext arithmetic Richard Henderson
2024-04-09  5:02 ` [PATCH v2 16/28] linux-user/i386: Remove xfeatures from target_fpstate_fxsave Richard Henderson
2024-04-09  5:02 ` [PATCH v2 17/28] linux-user/i386: Replace target_fpstate_fxsave with X86LegacyXSaveArea Richard Henderson
2024-04-09  5:02 ` [PATCH v2 18/28] linux-user/i386: Split out struct target_fregs_state Richard Henderson
2024-04-09  5:02 ` [PATCH v2 19/28] linux-user/i386: Fix -mregparm=3 for signal delivery Richard Henderson
2024-04-09  7:31   ` Paolo Bonzini
2024-04-09  5:02 ` [PATCH v2 20/28] linux-user/i386: Return boolean success from restore_sigcontext Richard Henderson
2024-04-09  5:02 ` [PATCH v2 21/28] linux-user/i386: Return boolean success from xrstor_sigcontext Richard Henderson
2024-04-09  5:02 ` [PATCH v2 22/28] linux-user/i386: Fix allocation and alignment of fp state Richard Henderson
2024-04-09  5:02 ` [PATCH v2 23/28] target/i386: Honor xfeatures in xrstor_sigcontext Richard Henderson
2024-04-09  7:44   ` Paolo Bonzini
2024-04-09 18:09     ` Richard Henderson [this message]
2024-04-10  0:27       ` Richard Henderson
2024-04-09  5:02 ` [PATCH v2 24/28] target/i386: Convert do_xsave to X86Access Richard Henderson
2024-04-09  5:02 ` [PATCH v2 25/28] target/i386: Convert do_xrstor " Richard Henderson
2024-04-09  5:03 ` [PATCH v2 26/28] target/i386: Pass host pointer and size to cpu_x86_{fsave, frstor} Richard Henderson
2024-04-09  5:03 ` [PATCH v2 27/28] target/i386: Pass host pointer and size to cpu_x86_{fxsave, fxrstor} Richard Henderson
2024-04-09  5:03 ` [PATCH v2 28/28] target/i386: Pass host pointer and size to cpu_x86_{xsave, xrstor} Richard Henderson
2024-04-09  7:52 ` [PATCH for-9.1 v2 00/28] linux-user/i386: Properly align signal frame Paolo Bonzini

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f67bf3a0-d684-4fe7-af05-bf2c1f273e42@linaro.org \
    --to=richard.henderson@linaro.org \
    --cc=pbonzini@redhat.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).