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
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)
--
2.34.124.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
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~
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)
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)
>
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
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~
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
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)
"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)
>
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~
© 2016 - 2024 Red Hat, Inc.