On Thu, 19 Mar 2026 at 03:30, Richard Henderson
<[email protected]> 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: [email protected]
> > Signed-off-by: Peter Maydell <[email protected]>
> 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
