diff mbox series

linux-user/i386: Properly align signal frame

Message ID 20230524054647.1093758-1-richard.henderson@linaro.org
State New
Headers show
Series linux-user/i386: Properly align signal frame | expand

Commit Message

Richard Henderson May 24, 2023, 5:46 a.m. UTC
The beginning of the structure, with pretaddr, should be just below
16-byte alignment.  Disconnect fpstate from sigframe, just like the
kernel does.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 linux-user/i386/signal.c         | 104 +++++++++++++++++--------------
 tests/tcg/x86_64/sigstack.c      |  33 ++++++++++
 tests/tcg/x86_64/Makefile.target |   1 +
 3 files changed, 90 insertions(+), 48 deletions(-)
 create mode 100644 tests/tcg/x86_64/sigstack.c

Comments

Richard Henderson May 24, 2023, 5:50 a.m. UTC | #1
On 5/23/23 22:46, Richard Henderson wrote:
> The beginning of the structure, with pretaddr, should be just below
> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
> kernel does.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/i386/signal.c         | 104 +++++++++++++++++--------------
>   tests/tcg/x86_64/sigstack.c      |  33 ++++++++++
>   tests/tcg/x86_64/Makefile.target |   1 +
>   3 files changed, 90 insertions(+), 48 deletions(-)
>   create mode 100644 tests/tcg/x86_64/sigstack.c

Oops, meant to add

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1648

r~

> 
> diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
> index 60fa07d6f9..c49467de78 100644
> --- a/linux-user/i386/signal.c
> +++ b/linux-user/i386/signal.c
> @@ -191,16 +191,7 @@ struct sigframe {
>       struct target_fpstate fpstate_unused;
>       abi_ulong extramask[TARGET_NSIG_WORDS-1];
>       char retcode[8];
> -
> -    /*
> -     * This field will be 16-byte aligned in memory.  Applying QEMU_ALIGNED
> -     * to it ensures that the base of the frame has an appropriate alignment
> -     * too.
> -     */
> -    struct target_fpstate fpstate QEMU_ALIGNED(8);
>   };
> -#define TARGET_SIGFRAME_FXSAVE_OFFSET (                                    \
> -    offsetof(struct sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>   
>   struct rt_sigframe {
>       abi_ulong pretcode;
> @@ -210,27 +201,21 @@ struct rt_sigframe {
>       struct target_siginfo info;
>       struct target_ucontext uc;
>       char retcode[8];
> -    struct target_fpstate fpstate QEMU_ALIGNED(8);
>   };
> -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
> -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>   #else
> -
>   struct rt_sigframe {
>       abi_ulong pretcode;
>       struct target_ucontext uc;
>       struct target_siginfo info;
> -    struct target_fpstate fpstate QEMU_ALIGNED(16);
>   };
> -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
> -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>   #endif
>   
>   /*
>    * Set up a signal frame.
>    */
>   
> -static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
> +static void xsave_sigcontext(CPUX86State *env,
> +                             struct target_fpstate_fxsave *fxsave,
>                                abi_ulong fxsave_addr)
>   {
>       if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> @@ -266,8 +251,9 @@ static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxs
>   }
>   
>   static void setup_sigcontext(struct target_sigcontext *sc,
> -        struct target_fpstate *fpstate, CPUX86State *env, abi_ulong mask,
> -        abi_ulong fpstate_addr)
> +                             struct target_fpstate *fpstate,
> +                             CPUX86State *env, abi_ulong mask,
> +                             abi_ulong fpstate_addr)
>   {
>       CPUState *cs = env_cpu(env);
>   #ifndef TARGET_X86_64
> @@ -347,10 +333,11 @@ static void setup_sigcontext(struct target_sigcontext *sc,
>    * Determine which stack to use..
>    */
>   
> -static inline abi_ulong
> -get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset)
> +static abi_ulong get_sigframe(struct target_sigaction *ka, CPUX86State *env,
> +                              size_t *frame_size, abi_ulong *fpsave_addr)
>   {
> -    unsigned long esp;
> +    abi_ulong esp, orig;
> +    size_t fpsave_size;
>   
>       /* Default to using normal stack */
>       esp = get_sp_from_cpustate(env);
> @@ -371,16 +358,23 @@ get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset
>           }
>   #endif
>       }
> +    orig = esp;
>   
> -    if (!(env->features[FEAT_1_EDX] & CPUID_FXSR)) {
> -        return (esp - (fxsave_offset + TARGET_FXSAVE_SIZE)) & -8ul;
> -    } else if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> -        return ((esp - TARGET_FXSAVE_SIZE) & -16ul) - fxsave_offset;
> +    if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> +        fpsave_size = TARGET_FXSAVE_SIZE;
> +        esp = ROUND_DOWN(esp - fpsave_size, 16);
>       } else {
> -        size_t xstate_size =
> -               xsave_area_size(env->xcr0, false) + TARGET_FP_XSTATE_MAGIC2_SIZE;
> -        return ((esp - xstate_size) & -64ul) - fxsave_offset;
> +        fpsave_size = xsave_area_size(env->xcr0, false)
> +                      + TARGET_FP_XSTATE_MAGIC2_SIZE;
> +        esp = ROUND_DOWN(esp - fpsave_size, 64);
>       }
> +    *fpsave_addr = esp;
> +
> +    esp = esp - *frame_size + sizeof(abi_ulong);
> +    esp = ROUND_DOWN(esp, 16) - sizeof(abi_ulong);
> +
> +    *frame_size = orig - esp;
> +    return esp;
>   }
>   
>   #ifndef TARGET_X86_64
> @@ -405,26 +399,34 @@ void setup_frame(int sig, struct target_sigaction *ka,
>                    target_sigset_t *set, CPUX86State *env)
>   {
>       abi_ulong frame_addr;
> +    abi_ulong fpstate_addr;
> +    size_t frame_size;
>       struct sigframe *frame;
> +    struct target_fpstate *fpstate;
>       int i;
>   
> -    frame_addr = get_sigframe(ka, env, TARGET_SIGFRAME_FXSAVE_OFFSET);
> +    frame_size = sizeof(struct sigframe);
> +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
>       trace_user_setup_frame(env, frame_addr);
>   
> -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
> +    if (!frame) {
>           goto give_sigsegv;
> +    }
> +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
>   
>       __put_user(sig, &frame->sig);
>   
> -    setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0],
> -            frame_addr + offsetof(struct sigframe, fpstate));
> +    setup_sigcontext(&frame->sc, fpstate, env, set->sig[0], fpstate_addr);
>   
> -    for(i = 1; i < TARGET_NSIG_WORDS; i++) {
> +    for (i = 1; i < TARGET_NSIG_WORDS; i++) {
>           __put_user(set->sig[i], &frame->extramask[i - 1]);
>       }
>   
> -    /* Set up to return from userspace.  If provided, use a stub
> -       already in userspace.  */
> +    /*
> +     * Set up to return from userspace.
> +     * If provided, use a stub already in userspace.
> +     */
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
>           __put_user(ka->sa_restorer, &frame->pretcode);
>       } else {
> @@ -443,11 +445,10 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       cpu_x86_load_seg(env, R_CS, __USER_CS);
>       env->eflags &= ~TF_MASK;
>   
> -    unlock_user_struct(frame, frame_addr, 1);
> -
> +    unlock_user(frame, frame_addr, frame_size);
>       return;
>   
> -give_sigsegv:
> + give_sigsegv:
>       force_sigsegv(sig);
>   }
>   #endif
> @@ -458,17 +459,24 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>                       target_sigset_t *set, CPUX86State *env)
>   {
>       abi_ulong frame_addr;
> +    abi_ulong fpstate_addr;
> +    size_t frame_size;
>   #ifndef TARGET_X86_64
>       abi_ulong addr;
>   #endif
>       struct rt_sigframe *frame;
> +    struct target_fpstate *fpstate;
>       int i;
>   
> -    frame_addr = get_sigframe(ka, env, TARGET_RT_SIGFRAME_FXSAVE_OFFSET);
> +    frame_size = sizeof(struct rt_sigframe);
> +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
>       trace_user_setup_rt_frame(env, frame_addr);
>   
> -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
> +    if (!frame) {
>           goto give_sigsegv;
> +    }
> +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
>   
>       /* These fields are only in rt_sigframe on 32 bit */
>   #ifndef TARGET_X86_64
> @@ -490,10 +498,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       }
>       __put_user(0, &frame->uc.tuc_link);
>       target_save_altstack(&frame->uc.tuc_stack, env);
> -    setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env,
> -            set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate));
> +    setup_sigcontext(&frame->uc.tuc_mcontext, fpstate, env,
> +                     set->sig[0], fpstate_addr);
>   
> -    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
> +    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
>           __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
>       }
>   
> @@ -533,15 +541,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       cpu_x86_load_seg(env, R_SS, __USER_DS);
>       env->eflags &= ~TF_MASK;
>   
> -    unlock_user_struct(frame, frame_addr, 1);
> -
> +    unlock_user(frame, frame_addr, frame_size);
>       return;
>   
> -give_sigsegv:
> + give_sigsegv:
>       force_sigsegv(sig);
>   }
>   
> -static int xrstor_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
> +static int xrstor_sigcontext(CPUX86State *env,
> +                             struct target_fpstate_fxsave *fxsave,
>                                abi_ulong fxsave_addr)
>   {
>       if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
> diff --git a/tests/tcg/x86_64/sigstack.c b/tests/tcg/x86_64/sigstack.c
> new file mode 100644
> index 0000000000..06cb847569
> --- /dev/null
> +++ b/tests/tcg/x86_64/sigstack.c
> @@ -0,0 +1,33 @@
> +#include <stdlib.h>
> +#include <assert.h>
> +#include <signal.h>
> +#include <stdint.h>
> +
> +void __attribute__((noinline)) bar(void)
> +{
> +    exit(EXIT_SUCCESS);
> +}
> +
> +void __attribute__((noinline, ms_abi)) foo(void)
> +{
> +    /*
> +     * With ms_abi, there are call-saved xmm registers, which are forced
> +     * to the stack around the call to sysv_abi bar().  If the signal
> +     * stack frame is not properly aligned, movaps will raise #GP.
> +     */
> +    bar();
> +}
> +
> +void sighandler(int num)
> +{
> +    void* sp = __builtin_dwarf_cfa();
> +    assert((uintptr_t)sp % 16 == 0);
> +    foo();
> +}
> +
> +int main(void)
> +{
> +    signal(SIGUSR1, sighandler);
> +    raise(SIGUSR1);
> +    abort();
> +}
> diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
> index e64aab1b81..d961599f64 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -13,6 +13,7 @@ X86_64_TESTS += vsyscall
>   X86_64_TESTS += noexec
>   X86_64_TESTS += cmpxchg
>   X86_64_TESTS += adox
> +X86_64_TESTS += sigstack
>   TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
>   else
>   TESTS=$(MULTIARCH_TESTS)
Xingtao Yao (Fujitsu)" via May 26, 2023, 2:56 a.m. UTC | #2
"The beginning of the structure, with pretaddr, should be just below 16-byte alignment."

It is incorrect! The beginning of the structure, with pretaddr not aligned as 16-byte!
On x86-64, It aligned as (16n - sizeof(void*)) because of instruction "call" !





> -----原始邮件-----
> 发件人: "Richard Henderson" <richard.henderson@linaro.org>
> 发送时间: 2023-05-24 13:50:43 (星期三)
> 收件人: qemu-devel@nongnu.org
> 抄送: fanwj@mail.ustc.edu.cn, laurent@vivier.eu
> 主题: Re: [PATCH] linux-user/i386: Properly align signal frame
> 
> On 5/23/23 22:46, Richard Henderson wrote:
> > The beginning of the structure, with pretaddr, should be just below
> > 16-byte alignment.  Disconnect fpstate from sigframe, just like the
> > kernel does.
> > 
> > Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> > ---
> >   linux-user/i386/signal.c         | 104 +++++++++++++++++--------------
> >   tests/tcg/x86_64/sigstack.c      |  33 ++++++++++
> >   tests/tcg/x86_64/Makefile.target |   1 +
> >   3 files changed, 90 insertions(+), 48 deletions(-)
> >   create mode 100644 tests/tcg/x86_64/sigstack.c
> 
> Oops, meant to add
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1648
> 
> r~
> 
> > 
> > diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
> > index 60fa07d6f9..c49467de78 100644
> > --- a/linux-user/i386/signal.c
> > +++ b/linux-user/i386/signal.c
> > @@ -191,16 +191,7 @@ struct sigframe {
> >       struct target_fpstate fpstate_unused;
> >       abi_ulong extramask[TARGET_NSIG_WORDS-1];
> >       char retcode[8];
> > -
> > -    /*
> > -     * This field will be 16-byte aligned in memory.  Applying QEMU_ALIGNED
> > -     * to it ensures that the base of the frame has an appropriate alignment
> > -     * too.
> > -     */
> > -    struct target_fpstate fpstate QEMU_ALIGNED(8);
> >   };
> > -#define TARGET_SIGFRAME_FXSAVE_OFFSET (                                    \
> > -    offsetof(struct sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
> >   
> >   struct rt_sigframe {
> >       abi_ulong pretcode;
> > @@ -210,27 +201,21 @@ struct rt_sigframe {
> >       struct target_siginfo info;
> >       struct target_ucontext uc;
> >       char retcode[8];
> > -    struct target_fpstate fpstate QEMU_ALIGNED(8);
> >   };
> > -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
> > -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
> >   #else
> > -
> >   struct rt_sigframe {
> >       abi_ulong pretcode;
> >       struct target_ucontext uc;
> >       struct target_siginfo info;
> > -    struct target_fpstate fpstate QEMU_ALIGNED(16);
> >   };
> > -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
> > -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
> >   #endif
> >   
> >   /*
> >    * Set up a signal frame.
> >    */
> >   
> > -static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
> > +static void xsave_sigcontext(CPUX86State *env,
> > +                             struct target_fpstate_fxsave *fxsave,
> >                                abi_ulong fxsave_addr)
> >   {
> >       if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> > @@ -266,8 +251,9 @@ static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxs
> >   }
> >   
> >   static void setup_sigcontext(struct target_sigcontext *sc,
> > -        struct target_fpstate *fpstate, CPUX86State *env, abi_ulong mask,
> > -        abi_ulong fpstate_addr)
> > +                             struct target_fpstate *fpstate,
> > +                             CPUX86State *env, abi_ulong mask,
> > +                             abi_ulong fpstate_addr)
> >   {
> >       CPUState *cs = env_cpu(env);
> >   #ifndef TARGET_X86_64
> > @@ -347,10 +333,11 @@ static void setup_sigcontext(struct target_sigcontext *sc,
> >    * Determine which stack to use..
> >    */
> >   
> > -static inline abi_ulong
> > -get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset)
> > +static abi_ulong get_sigframe(struct target_sigaction *ka, CPUX86State *env,
> > +                              size_t *frame_size, abi_ulong *fpsave_addr)
> >   {
> > -    unsigned long esp;
> > +    abi_ulong esp, orig;
> > +    size_t fpsave_size;
> >   
> >       /* Default to using normal stack */
> >       esp = get_sp_from_cpustate(env);
> > @@ -371,16 +358,23 @@ get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset
> >           }
> >   #endif
> >       }
> > +    orig = esp;
> >   
> > -    if (!(env->features[FEAT_1_EDX] & CPUID_FXSR)) {
> > -        return (esp - (fxsave_offset + TARGET_FXSAVE_SIZE)) & -8ul;
> > -    } else if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> > -        return ((esp - TARGET_FXSAVE_SIZE) & -16ul) - fxsave_offset;
> > +    if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> > +        fpsave_size = TARGET_FXSAVE_SIZE;
> > +        esp = ROUND_DOWN(esp - fpsave_size, 16);
> >       } else {
> > -        size_t xstate_size =
> > -               xsave_area_size(env->xcr0, false) + TARGET_FP_XSTATE_MAGIC2_SIZE;
> > -        return ((esp - xstate_size) & -64ul) - fxsave_offset;
> > +        fpsave_size = xsave_area_size(env->xcr0, false)
> > +                      + TARGET_FP_XSTATE_MAGIC2_SIZE;
> > +        esp = ROUND_DOWN(esp - fpsave_size, 64);
> >       }
> > +    *fpsave_addr = esp;
> > +
> > +    esp = esp - *frame_size + sizeof(abi_ulong);
> > +    esp = ROUND_DOWN(esp, 16) - sizeof(abi_ulong);
> > +
> > +    *frame_size = orig - esp;
> > +    return esp;
> >   }
> >   
> >   #ifndef TARGET_X86_64
> > @@ -405,26 +399,34 @@ void setup_frame(int sig, struct target_sigaction *ka,
> >                    target_sigset_t *set, CPUX86State *env)
> >   {
> >       abi_ulong frame_addr;
> > +    abi_ulong fpstate_addr;
> > +    size_t frame_size;
> >       struct sigframe *frame;
> > +    struct target_fpstate *fpstate;
> >       int i;
> >   
> > -    frame_addr = get_sigframe(ka, env, TARGET_SIGFRAME_FXSAVE_OFFSET);
> > +    frame_size = sizeof(struct sigframe);
> > +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
> >       trace_user_setup_frame(env, frame_addr);
> >   
> > -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> > +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
> > +    if (!frame) {
> >           goto give_sigsegv;
> > +    }
> > +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
> >   
> >       __put_user(sig, &frame->sig);
> >   
> > -    setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0],
> > -            frame_addr + offsetof(struct sigframe, fpstate));
> > +    setup_sigcontext(&frame->sc, fpstate, env, set->sig[0], fpstate_addr);
> >   
> > -    for(i = 1; i < TARGET_NSIG_WORDS; i++) {
> > +    for (i = 1; i < TARGET_NSIG_WORDS; i++) {
> >           __put_user(set->sig[i], &frame->extramask[i - 1]);
> >       }
> >   
> > -    /* Set up to return from userspace.  If provided, use a stub
> > -       already in userspace.  */
> > +    /*
> > +     * Set up to return from userspace.
> > +     * If provided, use a stub already in userspace.
> > +     */
> >       if (ka->sa_flags & TARGET_SA_RESTORER) {
> >           __put_user(ka->sa_restorer, &frame->pretcode);
> >       } else {
> > @@ -443,11 +445,10 @@ void setup_frame(int sig, struct target_sigaction *ka,
> >       cpu_x86_load_seg(env, R_CS, __USER_CS);
> >       env->eflags &= ~TF_MASK;
> >   
> > -    unlock_user_struct(frame, frame_addr, 1);
> > -
> > +    unlock_user(frame, frame_addr, frame_size);
> >       return;
> >   
> > -give_sigsegv:
> > + give_sigsegv:
> >       force_sigsegv(sig);
> >   }
> >   #endif
> > @@ -458,17 +459,24 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
> >                       target_sigset_t *set, CPUX86State *env)
> >   {
> >       abi_ulong frame_addr;
> > +    abi_ulong fpstate_addr;
> > +    size_t frame_size;
> >   #ifndef TARGET_X86_64
> >       abi_ulong addr;
> >   #endif
> >       struct rt_sigframe *frame;
> > +    struct target_fpstate *fpstate;
> >       int i;
> >   
> > -    frame_addr = get_sigframe(ka, env, TARGET_RT_SIGFRAME_FXSAVE_OFFSET);
> > +    frame_size = sizeof(struct rt_sigframe);
> > +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
> >       trace_user_setup_rt_frame(env, frame_addr);
> >   
> > -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> > +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
> > +    if (!frame) {
> >           goto give_sigsegv;
> > +    }
> > +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
> >   
> >       /* These fields are only in rt_sigframe on 32 bit */
> >   #ifndef TARGET_X86_64
> > @@ -490,10 +498,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
> >       }
> >       __put_user(0, &frame->uc.tuc_link);
> >       target_save_altstack(&frame->uc.tuc_stack, env);
> > -    setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env,
> > -            set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate));
> > +    setup_sigcontext(&frame->uc.tuc_mcontext, fpstate, env,
> > +                     set->sig[0], fpstate_addr);
> >   
> > -    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
> > +    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
> >           __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
> >       }
> >   
> > @@ -533,15 +541,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
> >       cpu_x86_load_seg(env, R_SS, __USER_DS);
> >       env->eflags &= ~TF_MASK;
> >   
> > -    unlock_user_struct(frame, frame_addr, 1);
> > -
> > +    unlock_user(frame, frame_addr, frame_size);
> >       return;
> >   
> > -give_sigsegv:
> > + give_sigsegv:
> >       force_sigsegv(sig);
> >   }
> >   
> > -static int xrstor_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
> > +static int xrstor_sigcontext(CPUX86State *env,
> > +                             struct target_fpstate_fxsave *fxsave,
> >                                abi_ulong fxsave_addr)
> >   {
> >       if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
> > diff --git a/tests/tcg/x86_64/sigstack.c b/tests/tcg/x86_64/sigstack.c
> > new file mode 100644
> > index 0000000000..06cb847569
> > --- /dev/null
> > +++ b/tests/tcg/x86_64/sigstack.c
> > @@ -0,0 +1,33 @@
> > +#include <stdlib.h>
> > +#include <assert.h>
> > +#include <signal.h>
> > +#include <stdint.h>
> > +
> > +void __attribute__((noinline)) bar(void)
> > +{
> > +    exit(EXIT_SUCCESS);
> > +}
> > +
> > +void __attribute__((noinline, ms_abi)) foo(void)
> > +{
> > +    /*
> > +     * With ms_abi, there are call-saved xmm registers, which are forced
> > +     * to the stack around the call to sysv_abi bar().  If the signal
> > +     * stack frame is not properly aligned, movaps will raise #GP.
> > +     */
> > +    bar();
> > +}
> > +
> > +void sighandler(int num)
> > +{
> > +    void* sp = __builtin_dwarf_cfa();
> > +    assert((uintptr_t)sp % 16 == 0);
> > +    foo();
> > +}
> > +
> > +int main(void)
> > +{
> > +    signal(SIGUSR1, sighandler);
> > +    raise(SIGUSR1);
> > +    abort();
> > +}
> > diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
> > index e64aab1b81..d961599f64 100644
> > --- a/tests/tcg/x86_64/Makefile.target
> > +++ b/tests/tcg/x86_64/Makefile.target
> > @@ -13,6 +13,7 @@ X86_64_TESTS += vsyscall
> >   X86_64_TESTS += noexec
> >   X86_64_TESTS += cmpxchg
> >   X86_64_TESTS += adox
> > +X86_64_TESTS += sigstack
> >   TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
> >   else
> >   TESTS=$(MULTIARCH_TESTS)
>
Richard Henderson May 26, 2023, 2:27 p.m. UTC | #3
On 5/25/23 19:56, fanwj@mail.ustc.edu.cn wrote:
> 
> "The beginning of the structure, with pretaddr, should be just below 16-byte alignment."
> 
> It is incorrect! The beginning of the structure, with pretaddr not aligned as 16-byte!
> On x86-64, It aligned as (16n - sizeof(void*)) because of instruction "call" !

Exactly: 16n - sizeof(void*) is why I mean by "just below 16-byte alignment".
Which is exactly what I have done...

>>> +    esp = esp - *frame_size + sizeof(abi_ulong);
>>> +    esp = ROUND_DOWN(esp, 16) - sizeof(abi_ulong);

... here.


r~
Richard Henderson June 20, 2023, 1:26 p.m. UTC | #4
Ping.

On 5/24/23 07:46, Richard Henderson wrote:
> The beginning of the structure, with pretaddr, should be just below
> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
> kernel does.
> 
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   linux-user/i386/signal.c         | 104 +++++++++++++++++--------------
>   tests/tcg/x86_64/sigstack.c      |  33 ++++++++++
>   tests/tcg/x86_64/Makefile.target |   1 +
>   3 files changed, 90 insertions(+), 48 deletions(-)
>   create mode 100644 tests/tcg/x86_64/sigstack.c
> 
> diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
> index 60fa07d6f9..c49467de78 100644
> --- a/linux-user/i386/signal.c
> +++ b/linux-user/i386/signal.c
> @@ -191,16 +191,7 @@ struct sigframe {
>       struct target_fpstate fpstate_unused;
>       abi_ulong extramask[TARGET_NSIG_WORDS-1];
>       char retcode[8];
> -
> -    /*
> -     * This field will be 16-byte aligned in memory.  Applying QEMU_ALIGNED
> -     * to it ensures that the base of the frame has an appropriate alignment
> -     * too.
> -     */
> -    struct target_fpstate fpstate QEMU_ALIGNED(8);
>   };
> -#define TARGET_SIGFRAME_FXSAVE_OFFSET (                                    \
> -    offsetof(struct sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>   
>   struct rt_sigframe {
>       abi_ulong pretcode;
> @@ -210,27 +201,21 @@ struct rt_sigframe {
>       struct target_siginfo info;
>       struct target_ucontext uc;
>       char retcode[8];
> -    struct target_fpstate fpstate QEMU_ALIGNED(8);
>   };
> -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
> -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>   #else
> -
>   struct rt_sigframe {
>       abi_ulong pretcode;
>       struct target_ucontext uc;
>       struct target_siginfo info;
> -    struct target_fpstate fpstate QEMU_ALIGNED(16);
>   };
> -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
> -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>   #endif
>   
>   /*
>    * Set up a signal frame.
>    */
>   
> -static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
> +static void xsave_sigcontext(CPUX86State *env,
> +                             struct target_fpstate_fxsave *fxsave,
>                                abi_ulong fxsave_addr)
>   {
>       if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> @@ -266,8 +251,9 @@ static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxs
>   }
>   
>   static void setup_sigcontext(struct target_sigcontext *sc,
> -        struct target_fpstate *fpstate, CPUX86State *env, abi_ulong mask,
> -        abi_ulong fpstate_addr)
> +                             struct target_fpstate *fpstate,
> +                             CPUX86State *env, abi_ulong mask,
> +                             abi_ulong fpstate_addr)
>   {
>       CPUState *cs = env_cpu(env);
>   #ifndef TARGET_X86_64
> @@ -347,10 +333,11 @@ static void setup_sigcontext(struct target_sigcontext *sc,
>    * Determine which stack to use..
>    */
>   
> -static inline abi_ulong
> -get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset)
> +static abi_ulong get_sigframe(struct target_sigaction *ka, CPUX86State *env,
> +                              size_t *frame_size, abi_ulong *fpsave_addr)
>   {
> -    unsigned long esp;
> +    abi_ulong esp, orig;
> +    size_t fpsave_size;
>   
>       /* Default to using normal stack */
>       esp = get_sp_from_cpustate(env);
> @@ -371,16 +358,23 @@ get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset
>           }
>   #endif
>       }
> +    orig = esp;
>   
> -    if (!(env->features[FEAT_1_EDX] & CPUID_FXSR)) {
> -        return (esp - (fxsave_offset + TARGET_FXSAVE_SIZE)) & -8ul;
> -    } else if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> -        return ((esp - TARGET_FXSAVE_SIZE) & -16ul) - fxsave_offset;
> +    if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
> +        fpsave_size = TARGET_FXSAVE_SIZE;
> +        esp = ROUND_DOWN(esp - fpsave_size, 16);
>       } else {
> -        size_t xstate_size =
> -               xsave_area_size(env->xcr0, false) + TARGET_FP_XSTATE_MAGIC2_SIZE;
> -        return ((esp - xstate_size) & -64ul) - fxsave_offset;
> +        fpsave_size = xsave_area_size(env->xcr0, false)
> +                      + TARGET_FP_XSTATE_MAGIC2_SIZE;
> +        esp = ROUND_DOWN(esp - fpsave_size, 64);
>       }
> +    *fpsave_addr = esp;
> +
> +    esp = esp - *frame_size + sizeof(abi_ulong);
> +    esp = ROUND_DOWN(esp, 16) - sizeof(abi_ulong);
> +
> +    *frame_size = orig - esp;
> +    return esp;
>   }
>   
>   #ifndef TARGET_X86_64
> @@ -405,26 +399,34 @@ void setup_frame(int sig, struct target_sigaction *ka,
>                    target_sigset_t *set, CPUX86State *env)
>   {
>       abi_ulong frame_addr;
> +    abi_ulong fpstate_addr;
> +    size_t frame_size;
>       struct sigframe *frame;
> +    struct target_fpstate *fpstate;
>       int i;
>   
> -    frame_addr = get_sigframe(ka, env, TARGET_SIGFRAME_FXSAVE_OFFSET);
> +    frame_size = sizeof(struct sigframe);
> +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
>       trace_user_setup_frame(env, frame_addr);
>   
> -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
> +    if (!frame) {
>           goto give_sigsegv;
> +    }
> +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
>   
>       __put_user(sig, &frame->sig);
>   
> -    setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0],
> -            frame_addr + offsetof(struct sigframe, fpstate));
> +    setup_sigcontext(&frame->sc, fpstate, env, set->sig[0], fpstate_addr);
>   
> -    for(i = 1; i < TARGET_NSIG_WORDS; i++) {
> +    for (i = 1; i < TARGET_NSIG_WORDS; i++) {
>           __put_user(set->sig[i], &frame->extramask[i - 1]);
>       }
>   
> -    /* Set up to return from userspace.  If provided, use a stub
> -       already in userspace.  */
> +    /*
> +     * Set up to return from userspace.
> +     * If provided, use a stub already in userspace.
> +     */
>       if (ka->sa_flags & TARGET_SA_RESTORER) {
>           __put_user(ka->sa_restorer, &frame->pretcode);
>       } else {
> @@ -443,11 +445,10 @@ void setup_frame(int sig, struct target_sigaction *ka,
>       cpu_x86_load_seg(env, R_CS, __USER_CS);
>       env->eflags &= ~TF_MASK;
>   
> -    unlock_user_struct(frame, frame_addr, 1);
> -
> +    unlock_user(frame, frame_addr, frame_size);
>       return;
>   
> -give_sigsegv:
> + give_sigsegv:
>       force_sigsegv(sig);
>   }
>   #endif
> @@ -458,17 +459,24 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>                       target_sigset_t *set, CPUX86State *env)
>   {
>       abi_ulong frame_addr;
> +    abi_ulong fpstate_addr;
> +    size_t frame_size;
>   #ifndef TARGET_X86_64
>       abi_ulong addr;
>   #endif
>       struct rt_sigframe *frame;
> +    struct target_fpstate *fpstate;
>       int i;
>   
> -    frame_addr = get_sigframe(ka, env, TARGET_RT_SIGFRAME_FXSAVE_OFFSET);
> +    frame_size = sizeof(struct rt_sigframe);
> +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
>       trace_user_setup_rt_frame(env, frame_addr);
>   
> -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
> +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
> +    if (!frame) {
>           goto give_sigsegv;
> +    }
> +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
>   
>       /* These fields are only in rt_sigframe on 32 bit */
>   #ifndef TARGET_X86_64
> @@ -490,10 +498,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       }
>       __put_user(0, &frame->uc.tuc_link);
>       target_save_altstack(&frame->uc.tuc_stack, env);
> -    setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env,
> -            set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate));
> +    setup_sigcontext(&frame->uc.tuc_mcontext, fpstate, env,
> +                     set->sig[0], fpstate_addr);
>   
> -    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
> +    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
>           __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
>       }
>   
> @@ -533,15 +541,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>       cpu_x86_load_seg(env, R_SS, __USER_DS);
>       env->eflags &= ~TF_MASK;
>   
> -    unlock_user_struct(frame, frame_addr, 1);
> -
> +    unlock_user(frame, frame_addr, frame_size);
>       return;
>   
> -give_sigsegv:
> + give_sigsegv:
>       force_sigsegv(sig);
>   }
>   
> -static int xrstor_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
> +static int xrstor_sigcontext(CPUX86State *env,
> +                             struct target_fpstate_fxsave *fxsave,
>                                abi_ulong fxsave_addr)
>   {
>       if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
> diff --git a/tests/tcg/x86_64/sigstack.c b/tests/tcg/x86_64/sigstack.c
> new file mode 100644
> index 0000000000..06cb847569
> --- /dev/null
> +++ b/tests/tcg/x86_64/sigstack.c
> @@ -0,0 +1,33 @@
> +#include <stdlib.h>
> +#include <assert.h>
> +#include <signal.h>
> +#include <stdint.h>
> +
> +void __attribute__((noinline)) bar(void)
> +{
> +    exit(EXIT_SUCCESS);
> +}
> +
> +void __attribute__((noinline, ms_abi)) foo(void)
> +{
> +    /*
> +     * With ms_abi, there are call-saved xmm registers, which are forced
> +     * to the stack around the call to sysv_abi bar().  If the signal
> +     * stack frame is not properly aligned, movaps will raise #GP.
> +     */
> +    bar();
> +}
> +
> +void sighandler(int num)
> +{
> +    void* sp = __builtin_dwarf_cfa();
> +    assert((uintptr_t)sp % 16 == 0);
> +    foo();
> +}
> +
> +int main(void)
> +{
> +    signal(SIGUSR1, sighandler);
> +    raise(SIGUSR1);
> +    abort();
> +}
> diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
> index e64aab1b81..d961599f64 100644
> --- a/tests/tcg/x86_64/Makefile.target
> +++ b/tests/tcg/x86_64/Makefile.target
> @@ -13,6 +13,7 @@ X86_64_TESTS += vsyscall
>   X86_64_TESTS += noexec
>   X86_64_TESTS += cmpxchg
>   X86_64_TESTS += adox
> +X86_64_TESTS += sigstack
>   TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
>   else
>   TESTS=$(MULTIARCH_TESTS)
Richard Henderson June 30, 2023, 5:53 p.m. UTC | #5
Ping 2.

On 6/20/23 15:26, Richard Henderson wrote:
> Ping.
> 
> On 5/24/23 07:46, Richard Henderson wrote:
>> The beginning of the structure, with pretaddr, should be just below
>> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
>> kernel does.
>>
>> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>   linux-user/i386/signal.c         | 104 +++++++++++++++++--------------
>>   tests/tcg/x86_64/sigstack.c      |  33 ++++++++++
>>   tests/tcg/x86_64/Makefile.target |   1 +
>>   3 files changed, 90 insertions(+), 48 deletions(-)
>>   create mode 100644 tests/tcg/x86_64/sigstack.c
>>
>> diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
>> index 60fa07d6f9..c49467de78 100644
>> --- a/linux-user/i386/signal.c
>> +++ b/linux-user/i386/signal.c
>> @@ -191,16 +191,7 @@ struct sigframe {
>>       struct target_fpstate fpstate_unused;
>>       abi_ulong extramask[TARGET_NSIG_WORDS-1];
>>       char retcode[8];
>> -
>> -    /*
>> -     * This field will be 16-byte aligned in memory.  Applying QEMU_ALIGNED
>> -     * to it ensures that the base of the frame has an appropriate alignment
>> -     * too.
>> -     */
>> -    struct target_fpstate fpstate QEMU_ALIGNED(8);
>>   };
>> -#define TARGET_SIGFRAME_FXSAVE_OFFSET (                                    \
>> -    offsetof(struct sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>>   struct rt_sigframe {
>>       abi_ulong pretcode;
>> @@ -210,27 +201,21 @@ struct rt_sigframe {
>>       struct target_siginfo info;
>>       struct target_ucontext uc;
>>       char retcode[8];
>> -    struct target_fpstate fpstate QEMU_ALIGNED(8);
>>   };
>> -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
>> -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>>   #else
>> -
>>   struct rt_sigframe {
>>       abi_ulong pretcode;
>>       struct target_ucontext uc;
>>       struct target_siginfo info;
>> -    struct target_fpstate fpstate QEMU_ALIGNED(16);
>>   };
>> -#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
>> -    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
>>   #endif
>>   /*
>>    * Set up a signal frame.
>>    */
>> -static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
>> +static void xsave_sigcontext(CPUX86State *env,
>> +                             struct target_fpstate_fxsave *fxsave,
>>                                abi_ulong fxsave_addr)
>>   {
>>       if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
>> @@ -266,8 +251,9 @@ static void xsave_sigcontext(CPUX86State *env, struct 
>> target_fpstate_fxsave *fxs
>>   }
>>   static void setup_sigcontext(struct target_sigcontext *sc,
>> -        struct target_fpstate *fpstate, CPUX86State *env, abi_ulong mask,
>> -        abi_ulong fpstate_addr)
>> +                             struct target_fpstate *fpstate,
>> +                             CPUX86State *env, abi_ulong mask,
>> +                             abi_ulong fpstate_addr)
>>   {
>>       CPUState *cs = env_cpu(env);
>>   #ifndef TARGET_X86_64
>> @@ -347,10 +333,11 @@ static void setup_sigcontext(struct target_sigcontext *sc,
>>    * Determine which stack to use..
>>    */
>> -static inline abi_ulong
>> -get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset)
>> +static abi_ulong get_sigframe(struct target_sigaction *ka, CPUX86State *env,
>> +                              size_t *frame_size, abi_ulong *fpsave_addr)
>>   {
>> -    unsigned long esp;
>> +    abi_ulong esp, orig;
>> +    size_t fpsave_size;
>>       /* Default to using normal stack */
>>       esp = get_sp_from_cpustate(env);
>> @@ -371,16 +358,23 @@ get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t 
>> fxsave_offset
>>           }
>>   #endif
>>       }
>> +    orig = esp;
>> -    if (!(env->features[FEAT_1_EDX] & CPUID_FXSR)) {
>> -        return (esp - (fxsave_offset + TARGET_FXSAVE_SIZE)) & -8ul;
>> -    } else if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
>> -        return ((esp - TARGET_FXSAVE_SIZE) & -16ul) - fxsave_offset;
>> +    if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
>> +        fpsave_size = TARGET_FXSAVE_SIZE;
>> +        esp = ROUND_DOWN(esp - fpsave_size, 16);
>>       } else {
>> -        size_t xstate_size =
>> -               xsave_area_size(env->xcr0, false) + TARGET_FP_XSTATE_MAGIC2_SIZE;
>> -        return ((esp - xstate_size) & -64ul) - fxsave_offset;
>> +        fpsave_size = xsave_area_size(env->xcr0, false)
>> +                      + TARGET_FP_XSTATE_MAGIC2_SIZE;
>> +        esp = ROUND_DOWN(esp - fpsave_size, 64);
>>       }
>> +    *fpsave_addr = esp;
>> +
>> +    esp = esp - *frame_size + sizeof(abi_ulong);
>> +    esp = ROUND_DOWN(esp, 16) - sizeof(abi_ulong);
>> +
>> +    *frame_size = orig - esp;
>> +    return esp;
>>   }
>>   #ifndef TARGET_X86_64
>> @@ -405,26 +399,34 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>                    target_sigset_t *set, CPUX86State *env)
>>   {
>>       abi_ulong frame_addr;
>> +    abi_ulong fpstate_addr;
>> +    size_t frame_size;
>>       struct sigframe *frame;
>> +    struct target_fpstate *fpstate;
>>       int i;
>> -    frame_addr = get_sigframe(ka, env, TARGET_SIGFRAME_FXSAVE_OFFSET);
>> +    frame_size = sizeof(struct sigframe);
>> +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
>>       trace_user_setup_frame(env, frame_addr);
>> -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
>> +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
>> +    if (!frame) {
>>           goto give_sigsegv;
>> +    }
>> +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
>>       __put_user(sig, &frame->sig);
>> -    setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0],
>> -            frame_addr + offsetof(struct sigframe, fpstate));
>> +    setup_sigcontext(&frame->sc, fpstate, env, set->sig[0], fpstate_addr);
>> -    for(i = 1; i < TARGET_NSIG_WORDS; i++) {
>> +    for (i = 1; i < TARGET_NSIG_WORDS; i++) {
>>           __put_user(set->sig[i], &frame->extramask[i - 1]);
>>       }
>> -    /* Set up to return from userspace.  If provided, use a stub
>> -       already in userspace.  */
>> +    /*
>> +     * Set up to return from userspace.
>> +     * If provided, use a stub already in userspace.
>> +     */
>>       if (ka->sa_flags & TARGET_SA_RESTORER) {
>>           __put_user(ka->sa_restorer, &frame->pretcode);
>>       } else {
>> @@ -443,11 +445,10 @@ void setup_frame(int sig, struct target_sigaction *ka,
>>       cpu_x86_load_seg(env, R_CS, __USER_CS);
>>       env->eflags &= ~TF_MASK;
>> -    unlock_user_struct(frame, frame_addr, 1);
>> -
>> +    unlock_user(frame, frame_addr, frame_size);
>>       return;
>> -give_sigsegv:
>> + give_sigsegv:
>>       force_sigsegv(sig);
>>   }
>>   #endif
>> @@ -458,17 +459,24 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>>                       target_sigset_t *set, CPUX86State *env)
>>   {
>>       abi_ulong frame_addr;
>> +    abi_ulong fpstate_addr;
>> +    size_t frame_size;
>>   #ifndef TARGET_X86_64
>>       abi_ulong addr;
>>   #endif
>>       struct rt_sigframe *frame;
>> +    struct target_fpstate *fpstate;
>>       int i;
>> -    frame_addr = get_sigframe(ka, env, TARGET_RT_SIGFRAME_FXSAVE_OFFSET);
>> +    frame_size = sizeof(struct rt_sigframe);
>> +    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
>>       trace_user_setup_rt_frame(env, frame_addr);
>> -    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
>> +    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
>> +    if (!frame) {
>>           goto give_sigsegv;
>> +    }
>> +    fpstate = (void *)frame + (fpstate_addr - frame_addr);
>>       /* These fields are only in rt_sigframe on 32 bit */
>>   #ifndef TARGET_X86_64
>> @@ -490,10 +498,10 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>>       }
>>       __put_user(0, &frame->uc.tuc_link);
>>       target_save_altstack(&frame->uc.tuc_stack, env);
>> -    setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env,
>> -            set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate));
>> +    setup_sigcontext(&frame->uc.tuc_mcontext, fpstate, env,
>> +                     set->sig[0], fpstate_addr);
>> -    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
>> +    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
>>           __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
>>       }
>> @@ -533,15 +541,15 @@ void setup_rt_frame(int sig, struct target_sigaction *ka,
>>       cpu_x86_load_seg(env, R_SS, __USER_DS);
>>       env->eflags &= ~TF_MASK;
>> -    unlock_user_struct(frame, frame_addr, 1);
>> -
>> +    unlock_user(frame, frame_addr, frame_size);
>>       return;
>> -give_sigsegv:
>> + give_sigsegv:
>>       force_sigsegv(sig);
>>   }
>> -static int xrstor_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
>> +static int xrstor_sigcontext(CPUX86State *env,
>> +                             struct target_fpstate_fxsave *fxsave,
>>                                abi_ulong fxsave_addr)
>>   {
>>       if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
>> diff --git a/tests/tcg/x86_64/sigstack.c b/tests/tcg/x86_64/sigstack.c
>> new file mode 100644
>> index 0000000000..06cb847569
>> --- /dev/null
>> +++ b/tests/tcg/x86_64/sigstack.c
>> @@ -0,0 +1,33 @@
>> +#include <stdlib.h>
>> +#include <assert.h>
>> +#include <signal.h>
>> +#include <stdint.h>
>> +
>> +void __attribute__((noinline)) bar(void)
>> +{
>> +    exit(EXIT_SUCCESS);
>> +}
>> +
>> +void __attribute__((noinline, ms_abi)) foo(void)
>> +{
>> +    /*
>> +     * With ms_abi, there are call-saved xmm registers, which are forced
>> +     * to the stack around the call to sysv_abi bar().  If the signal
>> +     * stack frame is not properly aligned, movaps will raise #GP.
>> +     */
>> +    bar();
>> +}
>> +
>> +void sighandler(int num)
>> +{
>> +    void* sp = __builtin_dwarf_cfa();
>> +    assert((uintptr_t)sp % 16 == 0);
>> +    foo();
>> +}
>> +
>> +int main(void)
>> +{
>> +    signal(SIGUSR1, sighandler);
>> +    raise(SIGUSR1);
>> +    abort();
>> +}
>> diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
>> index e64aab1b81..d961599f64 100644
>> --- a/tests/tcg/x86_64/Makefile.target
>> +++ b/tests/tcg/x86_64/Makefile.target
>> @@ -13,6 +13,7 @@ X86_64_TESTS += vsyscall
>>   X86_64_TESTS += noexec
>>   X86_64_TESTS += cmpxchg
>>   X86_64_TESTS += adox
>> +X86_64_TESTS += sigstack
>>   TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
>>   else
>>   TESTS=$(MULTIARCH_TESTS)
>
Michael Tokarev July 2, 2023, 6:41 p.m. UTC | #6
30.06.2023 20:53, Richard Henderson wrote:
> Ping 2.
> 
> On 6/20/23 15:26, Richard Henderson wrote:
>> Ping.
>>
>> On 5/24/23 07:46, Richard Henderson wrote:
>>> The beginning of the structure, with pretaddr, should be just below
>>> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
>>> kernel does.

This smells like a -stable material too (I tagged it "stable" in my inbox
a month ago already), as it is fixing real bug which people are hitting.

/mjt
Michael Tokarev Aug. 5, 2023, 9:56 p.m. UTC | #7
30.06.2023 20:53, Richard Henderson wrote:
> Ping 2.
> 
> On 6/20/23 15:26, Richard Henderson wrote:
>> Ping.
>>
>> On 5/24/23 07:46, Richard Henderson wrote:
>>> The beginning of the structure, with pretaddr, should be just below
>>> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
>>> kernel does.

Whom we're pinging here? :)
Ping 3 ;)

(This is https://gitlab.com/qemu-project/qemu/-/issues/1648 and is a
-stable material too, it seems)

/mjt
Richard Henderson Aug. 6, 2023, 2:29 a.m. UTC | #8
On 8/5/23 14:56, Michael Tokarev wrote:
> 30.06.2023 20:53, Richard Henderson wrote:
>> Ping 2.
>>
>> On 6/20/23 15:26, Richard Henderson wrote:
>>> Ping.
>>>
>>> On 5/24/23 07:46, Richard Henderson wrote:
>>>> The beginning of the structure, with pretaddr, should be just below
>>>> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
>>>> kernel does.
> 
> Whom we're pinging here? :)
> Ping 3 ;)
> 
> (This is https://gitlab.com/qemu-project/qemu/-/issues/1648 and is a
> -stable material too, it seems)

In the month since, I've discovered that fpstate needs even more work.
This is extremely fiddly stuff, and we need a complete rewrite to match the kernel and the 
required alignments.


r~
Michael Tokarev Oct. 26, 2023, 6:35 a.m. UTC | #9
24.05.2023 08:46, Richard Henderson:
> The beginning of the structure, with pretaddr, should be just below
> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
> kernel does.

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1648

Ping? Has this been forgotten? It's been 5 months already..

/mjt
Richard Henderson Oct. 27, 2023, 4:07 a.m. UTC | #10
On 10/25/23 23:35, Michael Tokarev wrote:
> 24.05.2023 08:46, Richard Henderson:
>> The beginning of the structure, with pretaddr, should be just below
>> 16-byte alignment.  Disconnect fpstate from sigframe, just like the
>> kernel does.
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/1648
> 
> Ping? Has this been forgotten? It's been 5 months already..

I have not returned to this problem yet.


r~
diff mbox series

Patch

diff --git a/linux-user/i386/signal.c b/linux-user/i386/signal.c
index 60fa07d6f9..c49467de78 100644
--- a/linux-user/i386/signal.c
+++ b/linux-user/i386/signal.c
@@ -191,16 +191,7 @@  struct sigframe {
     struct target_fpstate fpstate_unused;
     abi_ulong extramask[TARGET_NSIG_WORDS-1];
     char retcode[8];
-
-    /*
-     * This field will be 16-byte aligned in memory.  Applying QEMU_ALIGNED
-     * to it ensures that the base of the frame has an appropriate alignment
-     * too.
-     */
-    struct target_fpstate fpstate QEMU_ALIGNED(8);
 };
-#define TARGET_SIGFRAME_FXSAVE_OFFSET (                                    \
-    offsetof(struct sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
 
 struct rt_sigframe {
     abi_ulong pretcode;
@@ -210,27 +201,21 @@  struct rt_sigframe {
     struct target_siginfo info;
     struct target_ucontext uc;
     char retcode[8];
-    struct target_fpstate fpstate QEMU_ALIGNED(8);
 };
-#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
-    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
 #else
-
 struct rt_sigframe {
     abi_ulong pretcode;
     struct target_ucontext uc;
     struct target_siginfo info;
-    struct target_fpstate fpstate QEMU_ALIGNED(16);
 };
-#define TARGET_RT_SIGFRAME_FXSAVE_OFFSET (                                 \
-    offsetof(struct rt_sigframe, fpstate) + TARGET_FPSTATE_FXSAVE_OFFSET)
 #endif
 
 /*
  * Set up a signal frame.
  */
 
-static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
+static void xsave_sigcontext(CPUX86State *env,
+                             struct target_fpstate_fxsave *fxsave,
                              abi_ulong fxsave_addr)
 {
     if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
@@ -266,8 +251,9 @@  static void xsave_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxs
 }
 
 static void setup_sigcontext(struct target_sigcontext *sc,
-        struct target_fpstate *fpstate, CPUX86State *env, abi_ulong mask,
-        abi_ulong fpstate_addr)
+                             struct target_fpstate *fpstate,
+                             CPUX86State *env, abi_ulong mask,
+                             abi_ulong fpstate_addr)
 {
     CPUState *cs = env_cpu(env);
 #ifndef TARGET_X86_64
@@ -347,10 +333,11 @@  static void setup_sigcontext(struct target_sigcontext *sc,
  * Determine which stack to use..
  */
 
-static inline abi_ulong
-get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset)
+static abi_ulong get_sigframe(struct target_sigaction *ka, CPUX86State *env,
+                              size_t *frame_size, abi_ulong *fpsave_addr)
 {
-    unsigned long esp;
+    abi_ulong esp, orig;
+    size_t fpsave_size;
 
     /* Default to using normal stack */
     esp = get_sp_from_cpustate(env);
@@ -371,16 +358,23 @@  get_sigframe(struct target_sigaction *ka, CPUX86State *env, size_t fxsave_offset
         }
 #endif
     }
+    orig = esp;
 
-    if (!(env->features[FEAT_1_EDX] & CPUID_FXSR)) {
-        return (esp - (fxsave_offset + TARGET_FXSAVE_SIZE)) & -8ul;
-    } else if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
-        return ((esp - TARGET_FXSAVE_SIZE) & -16ul) - fxsave_offset;
+    if (!(env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE)) {
+        fpsave_size = TARGET_FXSAVE_SIZE;
+        esp = ROUND_DOWN(esp - fpsave_size, 16);
     } else {
-        size_t xstate_size =
-               xsave_area_size(env->xcr0, false) + TARGET_FP_XSTATE_MAGIC2_SIZE;
-        return ((esp - xstate_size) & -64ul) - fxsave_offset;
+        fpsave_size = xsave_area_size(env->xcr0, false)
+                      + TARGET_FP_XSTATE_MAGIC2_SIZE;
+        esp = ROUND_DOWN(esp - fpsave_size, 64);
     }
+    *fpsave_addr = esp;
+
+    esp = esp - *frame_size + sizeof(abi_ulong);
+    esp = ROUND_DOWN(esp, 16) - sizeof(abi_ulong);
+
+    *frame_size = orig - esp;
+    return esp;
 }
 
 #ifndef TARGET_X86_64
@@ -405,26 +399,34 @@  void setup_frame(int sig, struct target_sigaction *ka,
                  target_sigset_t *set, CPUX86State *env)
 {
     abi_ulong frame_addr;
+    abi_ulong fpstate_addr;
+    size_t frame_size;
     struct sigframe *frame;
+    struct target_fpstate *fpstate;
     int i;
 
-    frame_addr = get_sigframe(ka, env, TARGET_SIGFRAME_FXSAVE_OFFSET);
+    frame_size = sizeof(struct sigframe);
+    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
     trace_user_setup_frame(env, frame_addr);
 
-    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
+    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
+    if (!frame) {
         goto give_sigsegv;
+    }
+    fpstate = (void *)frame + (fpstate_addr - frame_addr);
 
     __put_user(sig, &frame->sig);
 
-    setup_sigcontext(&frame->sc, &frame->fpstate, env, set->sig[0],
-            frame_addr + offsetof(struct sigframe, fpstate));
+    setup_sigcontext(&frame->sc, fpstate, env, set->sig[0], fpstate_addr);
 
-    for(i = 1; i < TARGET_NSIG_WORDS; i++) {
+    for (i = 1; i < TARGET_NSIG_WORDS; i++) {
         __put_user(set->sig[i], &frame->extramask[i - 1]);
     }
 
-    /* Set up to return from userspace.  If provided, use a stub
-       already in userspace.  */
+    /*
+     * Set up to return from userspace.
+     * If provided, use a stub already in userspace.
+     */
     if (ka->sa_flags & TARGET_SA_RESTORER) {
         __put_user(ka->sa_restorer, &frame->pretcode);
     } else {
@@ -443,11 +445,10 @@  void setup_frame(int sig, struct target_sigaction *ka,
     cpu_x86_load_seg(env, R_CS, __USER_CS);
     env->eflags &= ~TF_MASK;
 
-    unlock_user_struct(frame, frame_addr, 1);
-
+    unlock_user(frame, frame_addr, frame_size);
     return;
 
-give_sigsegv:
+ give_sigsegv:
     force_sigsegv(sig);
 }
 #endif
@@ -458,17 +459,24 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
                     target_sigset_t *set, CPUX86State *env)
 {
     abi_ulong frame_addr;
+    abi_ulong fpstate_addr;
+    size_t frame_size;
 #ifndef TARGET_X86_64
     abi_ulong addr;
 #endif
     struct rt_sigframe *frame;
+    struct target_fpstate *fpstate;
     int i;
 
-    frame_addr = get_sigframe(ka, env, TARGET_RT_SIGFRAME_FXSAVE_OFFSET);
+    frame_size = sizeof(struct rt_sigframe);
+    frame_addr = get_sigframe(ka, env, &frame_size, &fpstate_addr);
     trace_user_setup_rt_frame(env, frame_addr);
 
-    if (!lock_user_struct(VERIFY_WRITE, frame, frame_addr, 0))
+    frame = lock_user(VERIFY_WRITE, frame_addr, frame_size, false);
+    if (!frame) {
         goto give_sigsegv;
+    }
+    fpstate = (void *)frame + (fpstate_addr - frame_addr);
 
     /* These fields are only in rt_sigframe on 32 bit */
 #ifndef TARGET_X86_64
@@ -490,10 +498,10 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
     }
     __put_user(0, &frame->uc.tuc_link);
     target_save_altstack(&frame->uc.tuc_stack, env);
-    setup_sigcontext(&frame->uc.tuc_mcontext, &frame->fpstate, env,
-            set->sig[0], frame_addr + offsetof(struct rt_sigframe, fpstate));
+    setup_sigcontext(&frame->uc.tuc_mcontext, fpstate, env,
+                     set->sig[0], fpstate_addr);
 
-    for(i = 0; i < TARGET_NSIG_WORDS; i++) {
+    for (i = 0; i < TARGET_NSIG_WORDS; i++) {
         __put_user(set->sig[i], &frame->uc.tuc_sigmask.sig[i]);
     }
 
@@ -533,15 +541,15 @@  void setup_rt_frame(int sig, struct target_sigaction *ka,
     cpu_x86_load_seg(env, R_SS, __USER_DS);
     env->eflags &= ~TF_MASK;
 
-    unlock_user_struct(frame, frame_addr, 1);
-
+    unlock_user(frame, frame_addr, frame_size);
     return;
 
-give_sigsegv:
+ give_sigsegv:
     force_sigsegv(sig);
 }
 
-static int xrstor_sigcontext(CPUX86State *env, struct target_fpstate_fxsave *fxsave,
+static int xrstor_sigcontext(CPUX86State *env,
+                             struct target_fpstate_fxsave *fxsave,
                              abi_ulong fxsave_addr)
 {
     if (env->features[FEAT_1_ECX] & CPUID_EXT_XSAVE) {
diff --git a/tests/tcg/x86_64/sigstack.c b/tests/tcg/x86_64/sigstack.c
new file mode 100644
index 0000000000..06cb847569
--- /dev/null
+++ b/tests/tcg/x86_64/sigstack.c
@@ -0,0 +1,33 @@ 
+#include <stdlib.h>
+#include <assert.h>
+#include <signal.h>
+#include <stdint.h>
+
+void __attribute__((noinline)) bar(void)
+{
+    exit(EXIT_SUCCESS);
+}
+
+void __attribute__((noinline, ms_abi)) foo(void)
+{
+    /*
+     * With ms_abi, there are call-saved xmm registers, which are forced
+     * to the stack around the call to sysv_abi bar().  If the signal
+     * stack frame is not properly aligned, movaps will raise #GP.
+     */
+    bar();
+}
+
+void sighandler(int num)
+{
+    void* sp = __builtin_dwarf_cfa();
+    assert((uintptr_t)sp % 16 == 0);
+    foo();
+}
+
+int main(void)
+{
+    signal(SIGUSR1, sighandler);
+    raise(SIGUSR1);
+    abort();
+}
diff --git a/tests/tcg/x86_64/Makefile.target b/tests/tcg/x86_64/Makefile.target
index e64aab1b81..d961599f64 100644
--- a/tests/tcg/x86_64/Makefile.target
+++ b/tests/tcg/x86_64/Makefile.target
@@ -13,6 +13,7 @@  X86_64_TESTS += vsyscall
 X86_64_TESTS += noexec
 X86_64_TESTS += cmpxchg
 X86_64_TESTS += adox
+X86_64_TESTS += sigstack
 TESTS=$(MULTIARCH_TESTS) $(X86_64_TESTS) test-x86_64
 else
 TESTS=$(MULTIARCH_TESTS)