Our definition of the target_fpstate_32 struct doesn't match the
kernel's version.  We only use this struct definition in the
definition of 'struct sigframe', where it is used in a field that is
present only for legacy reasons to retain the offset of the following
'extramask' field.  So really all that matters is its length, and we
do get that right; but our previous definition using
X86LegacySaveArea implicitly added an extra alignment constraint
(because X86LegacySaveArea is tagged as 16-aligned) which the real
target_fpstate_32 does not have.  Because we allocate and use a
'struct sigframe' on the guest's stack with the guest's alignment
requirements, this resulted in the undefined-behaviour sanitizer
complaining during 'make check-tcg' for i386-linux-user:

../../linux-user/i386/signal.c:471:35: runtime error: member access within misaligned address 0x1000c07f75ec for type 'struct sigframe', which requires 16 byte alignment
0x1000c07f75ec: note: pointer points here
  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
              ^

../../linux-user/i386/signal.c:808:5: runtime error: member access within misaligned address 0x1000c07f75f4 for type 'struct target_sigcontext_32', which requires 8 byte alignment
0x1000c07f75f4: note: pointer points here
  0a 00 00 00 33 00 00 00  00 00 00 00 2b 00 00 00  2b 00 00 00 40 05 80 40  f4 7f 10 08 58 05 80 40
              ^

and various similar errors.

Replace the use of X86LegacyXSaveArea with a set of fields that match
the kernel _fpstate_32 struct, and assert that the length is correct.
We could equally have used
   uint8_t legacy_area[512];
but following the kernel is probably less confusing overall.

Since in target/i386/cpu.h we assert that X86LegacySaveArea is 512
bytes, and in linux-user/i386/signal.c we assert that
target_fregs_state is (32 + 80) bytes, the new assertion confirms
that we didn't change the size of target_fpstate_32 here, only its
alignment requirements.

Cc: qemu-stable@nongnu.org
Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 linux-user/i386/signal.c | 25 ++++++++++++++++++++++++-
 1 file changed, 24 insertions(+), 1 deletion(-)

diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
index 0f11dba831..b646fde431 100644
--- a/linux-user/i386/signal.c
+++ b/linux-user/i386/signal.c
@@ -60,10 +60,33 @@ struct target_fpx_sw_bytes {
 };
 QEMU_BUILD_BUG_ON(sizeof(struct target_fpx_sw_bytes) != 12*4);
 
+struct fpxreg {
+    uint16_t significand[4];
+    uint16_t exponent;
+    uint16_t padding[3];
+};
+
+struct xmmreg {
+    uint32_t element[4];
+};
+
+/*
+ * This corresponds to the kernel's _fpstate_32. Since we
+ * only use it for the fpstate_unused padding section in
+ * the target sigcontext, it doesn't actually matter what fields
+ * we define here as long as we get the size right.
+ */
 struct target_fpstate_32 {
     struct target_fregs_state fpstate;
-    X86LegacyXSaveArea fxstate;
+    uint32_t fxsr_env[6];
+    uint32_t mxcsr;
+    uint32_t reserved;
+    struct fpxreg fxsr_st[8];
+    struct xmmreg xmm[8];
+    uint32_t padding1[44];
+    uint32_t padding2[12]; /* aka sw_reserved */
 };
+QEMU_BUILD_BUG_ON(sizeof(struct target_fpstate_32) != 32 + 80 + 512);
 
 struct target_sigcontext_32 {
     uint16_t gs, __gsh;
-- 
2.43.0

On 3/6/26 05:17, Peter Maydell wrote:
> Our definition of the target_fpstate_32 struct doesn't match the
> kernel's version.  We only use this struct definition in the
> definition of 'struct sigframe', where it is used in a field that is
> present only for legacy reasons to retain the offset of the following
> 'extramask' field.  So really all that matters is its length, and we
> do get that right; but our previous definition using
> X86LegacySaveArea implicitly added an extra alignment constraint
> (because X86LegacySaveArea is tagged as 16-aligned) which the real
> target_fpstate_32 does not have.  Because we allocate and use a
> 'struct sigframe' on the guest's stack with the guest's alignment
> requirements, this resulted in the undefined-behaviour sanitizer
> complaining during 'make check-tcg' for i386-linux-user:
> 
> ../../linux-user/i386/signal.c:471:35: runtime error: member access within misaligned address 0x1000c07f75ec for type 'struct sigframe', which requires 16 byte alignment
> 0x1000c07f75ec: note: pointer points here
>    00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
>                ^
> 
> ../../linux-user/i386/signal.c:808:5: runtime error: member access within misaligned address 0x1000c07f75f4 for type 'struct target_sigcontext_32', which requires 8 byte alignment
> 0x1000c07f75f4: note: pointer points here
>    0a 00 00 00 33 00 00 00  00 00 00 00 2b 00 00 00  2b 00 00 00 40 05 80 40  f4 7f 10 08 58 05 80 40
>                ^
> 
> and various similar errors.
> 
> Replace the use of X86LegacyXSaveArea with a set of fields that match
> the kernel _fpstate_32 struct, and assert that the length is correct.
> We could equally have used
>     uint8_t legacy_area[512];
> but following the kernel is probably less confusing overall.
> 
> Since in target/i386/cpu.h we assert that X86LegacySaveArea is 512
> bytes, and in linux-user/i386/signal.c we assert that
> target_fregs_state is (32 + 80) bytes, the new assertion confirms
> that we didn't change the size of target_fpstate_32 here, only its
> alignment requirements.
> 
> Cc: qemu-stable@nongnu.org
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>   linux-user/i386/signal.c | 25 ++++++++++++++++++++++++-
>   1 file changed, 24 insertions(+), 1 deletion(-)

I think it's more complicated than that.  I think this is indicative of the alignment 
being wrong.  In fact, I have a vague memory that we have an Issue outstanding for this, 
which I looked into once upon a time, and I think we're well out of spec.

There are two parts: fpregs, which should be 64-byte aligned (for xsave/xrestore), and the 
main signal frame, which must be x % 16 == 16 - sizeof(void *), so that SP, after pushing 
the return address, is 16-byte aligned.

I can't immediately find the issue.  I'll see about writing a test case.


r~

On Thu, 19 Mar 2026 at 03:30, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/6/26 05:17, Peter Maydell wrote:
> > Our definition of the target_fpstate_32 struct doesn't match the
> > kernel's version.  We only use this struct definition in the
> > definition of 'struct sigframe', where it is used in a field that is
> > present only for legacy reasons to retain the offset of the following
> > 'extramask' field.  So really all that matters is its length, and we
> > do get that right; but our previous definition using
> > X86LegacySaveArea implicitly added an extra alignment constraint
> > (because X86LegacySaveArea is tagged as 16-aligned) which the real
> > target_fpstate_32 does not have.  Because we allocate and use a
> > 'struct sigframe' on the guest's stack with the guest's alignment
> > requirements, this resulted in the undefined-behaviour sanitizer
> > complaining during 'make check-tcg' for i386-linux-user:
> >
> > ../../linux-user/i386/signal.c:471:35: runtime error: member access within misaligned address 0x1000c07f75ec for type 'struct sigframe', which requires 16 byte alignment
> > 0x1000c07f75ec: note: pointer points here
> >    00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00  00 00 00 00 00 00 00 00
> >                ^
> >
> > ../../linux-user/i386/signal.c:808:5: runtime error: member access within misaligned address 0x1000c07f75f4 for type 'struct target_sigcontext_32', which requires 8 byte alignment
> > 0x1000c07f75f4: note: pointer points here
> >    0a 00 00 00 33 00 00 00  00 00 00 00 2b 00 00 00  2b 00 00 00 40 05 80 40  f4 7f 10 08 58 05 80 40
> >                ^
> >
> > and various similar errors.
> >
> > Replace the use of X86LegacyXSaveArea with a set of fields that match
> > the kernel _fpstate_32 struct, and assert that the length is correct.
> > We could equally have used
> >     uint8_t legacy_area[512];
> > but following the kernel is probably less confusing overall.
> >
> > Since in target/i386/cpu.h we assert that X86LegacySaveArea is 512
> > bytes, and in linux-user/i386/signal.c we assert that
> > target_fregs_state is (32 + 80) bytes, the new assertion confirms
> > that we didn't change the size of target_fpstate_32 here, only its
> > alignment requirements.
> >
> > Cc: qemu-stable@nongnu.org
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

> I think it's more complicated than that.  I think this is indicative of the alignment
> being wrong.  In fact, I have a vague memory that we have an Issue outstanding for this,
> which I looked into once upon a time, and I think we're well out of spec.

I could definitely believe that I'm missing something subtle here :-)

> There are two parts: fpregs, which should be 64-byte aligned (for xsave/xrestore), and the
> main signal frame, which must be x % 16 == 16 - sizeof(void *), so that SP, after pushing
> the return address, is 16-byte aligned.

The 64-bit x86 signal frame and the 32-bit signal frame alignment
requirements are slightly different, I think (and this problem and
this patch are dealing only with 32-bit x86). In the kernel's
arch/x86/kernel/signal.c:get_sigframe() they calculate the frame
address like this:

        if (ia32_frame)
                /*
                 * Align the stack pointer according to the i386 ABI,
                 * i.e. so that on function entry ((sp + 4) & 15) == 0.
                 */
                sp = ((sp + 4) & -FRAME_ALIGNMENT) - 4;
        else
                sp = round_down(sp, FRAME_ALIGNMENT) - 8;

So x % 16 == 16 - sizeof(void*) is true for 64-bit, but 32-bit is
slightly different.

In QEMU we do:

    /*
     * Align the stack pointer according to the ABI, i.e. so that on
     * function entry ((sp + sizeof(return_addr)) & 15) == 0.
     */
    sp += sizeof(target_ulong);
    sp = ROUND_DOWN(sp, 16);
    sp -= sizeof(target_ulong);

which matches the kernel for 32 bit (it does look like
it gives the wrong answer for 64-bit, but that's not relevant
for this bug).

I also note that for the data structure I am fixing, we never do
an xsave/xrestore on it -- it is purely junk unused padding.
The real xsave/xrestore info is elsewhere, in the dynamic part
of the signal frame.

thanks
-- PMM

On 3/19/26 22:27, Peter Maydell wrote:
> The 64-bit x86 signal frame and the 32-bit signal frame alignment
> requirements are slightly different, I think (and this problem and
> this patch are dealing only with 32-bit x86). In the kernel's
> arch/x86/kernel/signal.c:get_sigframe() they calculate the frame
> address like this:
> 
>          if (ia32_frame)
>                  /*
>                   * Align the stack pointer according to the i386 ABI,
>                   * i.e. so that on function entry ((sp + 4) & 15) == 0.
>                   */
>                  sp = ((sp + 4) & -FRAME_ALIGNMENT) - 4;
>          else
>                  sp = round_down(sp, FRAME_ALIGNMENT) - 8;
> 
> So x % 16 == 16 - sizeof(void*) is true for 64-bit, but 32-bit is
> slightly different.

No, these produce the same alignment, modulo 4 vs 8, i.e. sizeof(void *).

The x86_64 expression is less efficient in the case SP already contains the required 
alignment -- it will waste FRAME_ALIGNMENT bytes of unused stack.

> I also note that for the data structure I am fixing, we never do
> an xsave/xrestore on it -- it is purely junk unused padding.
> The real xsave/xrestore info is elsewhere, in the dynamic part
> of the signal frame.

That is an excellent point.  The structure you're fixing is never really used.

It's only use at present is to be included in the ia32 sigframe structure, and is only 
used for padding.  As an unintended side effect, the alignment of X86LegacyXSaveArea is 
inherited by sigframe.

Perhaps we should simply have

-    struct target_fpstate_32 fpstate_unused;
+    char fpstate_unused[sizeof(struct target_fregs_state) +
+                        sizeof(X86LegacyXSaveArea)];


r~

On Fri, 20 Mar 2026 at 01:15, Richard Henderson
<richard.henderson@linaro.org> wrote:
> On 3/19/26 22:27, Peter Maydell wrote:
> > I also note that for the data structure I am fixing, we never do
> > an xsave/xrestore on it -- it is purely junk unused padding.
> > The real xsave/xrestore info is elsewhere, in the dynamic part
> > of the signal frame.
>
> That is an excellent point.  The structure you're fixing is never really used.
>
> It's only use at present is to be included in the ia32 sigframe structure, and is only
> used for padding.  As an unintended side effect, the alignment of X86LegacyXSaveArea is
> inherited by sigframe.
>
> Perhaps we should simply have
>
> -    struct target_fpstate_32 fpstate_unused;
> +    char fpstate_unused[sizeof(struct target_fregs_state) +
> +                        sizeof(X86LegacyXSaveArea)];

I did consider something like that; from the commit message:

# Replace the use of X86LegacyXSaveArea with a set of fields that match
# the kernel _fpstate_32 struct, and assert that the length is correct.
# We could equally have used
#    uint8_t legacy_area[512];
# but following the kernel is probably less confusing overall.

Either way works, so I guess I don't care very strongly.

thanks
-- PMM

On 3/20/26 22:23, Peter Maydell wrote:
> On Fri, 20 Mar 2026 at 01:15, Richard Henderson
> <richard.henderson@linaro.org> wrote:
>> On 3/19/26 22:27, Peter Maydell wrote:
>>> I also note that for the data structure I am fixing, we never do
>>> an xsave/xrestore on it -- it is purely junk unused padding.
>>> The real xsave/xrestore info is elsewhere, in the dynamic part
>>> of the signal frame.
>>
>> That is an excellent point.  The structure you're fixing is never really used.
>>
>> It's only use at present is to be included in the ia32 sigframe structure, and is only
>> used for padding.  As an unintended side effect, the alignment of X86LegacyXSaveArea is
>> inherited by sigframe.
>>
>> Perhaps we should simply have
>>
>> -    struct target_fpstate_32 fpstate_unused;
>> +    char fpstate_unused[sizeof(struct target_fregs_state) +
>> +                        sizeof(X86LegacyXSaveArea)];
> 
> I did consider something like that; from the commit message:
> 
> # Replace the use of X86LegacyXSaveArea with a set of fields that match
> # the kernel _fpstate_32 struct, and assert that the length is correct.
> # We could equally have used
> #    uint8_t legacy_area[512];
> # but following the kernel is probably less confusing overall.
> 
> Either way works, so I guess I don't care very strongly.

Ok then,

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

On Fri, 20 Mar 2026 at 15:37, Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> On 3/20/26 22:23, Peter Maydell wrote:
> > On Fri, 20 Mar 2026 at 01:15, Richard Henderson
> > <richard.henderson@linaro.org> wrote:
> >> On 3/19/26 22:27, Peter Maydell wrote:
> >>> I also note that for the data structure I am fixing, we never do
> >>> an xsave/xrestore on it -- it is purely junk unused padding.
> >>> The real xsave/xrestore info is elsewhere, in the dynamic part
> >>> of the signal frame.
> >>
> >> That is an excellent point.  The structure you're fixing is never really used.
> >>
> >> It's only use at present is to be included in the ia32 sigframe structure, and is only
> >> used for padding.  As an unintended side effect, the alignment of X86LegacyXSaveArea is
> >> inherited by sigframe.
> >>
> >> Perhaps we should simply have
> >>
> >> -    struct target_fpstate_32 fpstate_unused;
> >> +    char fpstate_unused[sizeof(struct target_fregs_state) +
> >> +                        sizeof(X86LegacyXSaveArea)];
> >
> > I did consider something like that; from the commit message:
> >
> > # Replace the use of X86LegacyXSaveArea with a set of fields that match
> > # the kernel _fpstate_32 struct, and assert that the length is correct.
> > # We could equally have used
> > #    uint8_t legacy_area[512];
> > # but following the kernel is probably less confusing overall.
> >
> > Either way works, so I guess I don't care very strongly.
>
> Ok then,
>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

Thanks; I've applied this to target-arm.next, unless anybody
would prefer to take it some other route.

-- PMM