Introduce new flags that defines which ABI to use on creating sigframe.
Those flags one may set from the userspace, or kernel will set them
according to syscall, which sets handler for a signal.
So that will drop the dependency on TIF_IA32/TIF_X32 flags on syscall deliver.
Those flags will be used only under CONFIG_COMPAT.
The same way ARM uses sa_flags to differ in which mode deliver signal
for 26-bit applications (look at SA_THIRYTWO).
Cc: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry Safonov <[email protected]>
---
arch/x86/ia32/ia32_signal.c | 2 +-
arch/x86/include/asm/fpu/signal.h | 6 ++++++
arch/x86/include/uapi/asm/signal.h | 6 +++++-
arch/x86/kernel/signal.c | 19 ++++++++++---------
arch/x86/kernel/signal_compat.c | 33 ++++++++++++++++++++++++++++++---
kernel/signal.c | 5 +++++
6 files changed, 57 insertions(+), 14 deletions(-)
diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
index 0552884da18d..faf1871300cb 100644
--- a/arch/x86/ia32/ia32_signal.c
+++ b/arch/x86/ia32/ia32_signal.c
@@ -378,7 +378,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
put_user_ex(*((u64 *)&code), (u64 __user *)frame->retcode);
} put_user_catch(err);
- err |= copy_siginfo_to_user32(&frame->info, &ksig->info);
+ err |= __copy_siginfo_to_user32(&frame->info, &ksig->info, false);
err |= ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
regs, set->sig[0]);
err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
index 0e970d00dfcd..20a1fbf7fe4e 100644
--- a/arch/x86/include/asm/fpu/signal.h
+++ b/arch/x86/include/asm/fpu/signal.h
@@ -19,6 +19,12 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
# define ia32_setup_rt_frame __setup_rt_frame
#endif
+#ifdef CONFIG_COMPAT
+int __copy_siginfo_to_user32(compat_siginfo_t __user *to,
+ const siginfo_t *from, bool x32_ABI);
+#endif
+
+
extern void convert_from_fxsr(struct user_i387_ia32_struct *env,
struct task_struct *tsk);
extern void convert_to_fxsr(struct task_struct *tsk,
diff --git a/arch/x86/include/uapi/asm/signal.h b/arch/x86/include/uapi/asm/signal.h
index 8264f47cf53e..9c663b6fc023 100644
--- a/arch/x86/include/uapi/asm/signal.h
+++ b/arch/x86/include/uapi/asm/signal.h
@@ -70,6 +70,8 @@ typedef unsigned long sigset_t;
* SA_RESETHAND clears the handler when the signal is delivered.
* SA_NOCLDWAIT flag on SIGCHLD to inhibit zombies.
* SA_NODEFER prevents the current signal from being masked in the handler.
+ * SA_IA32_ABI/SA_X32_ABI indicates ABI for a signal frame,
+ * if neither is set, the kernel will set them according to a syscall ABI
*
* SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single
* Unix names RESETHAND and NODEFER respectively.
@@ -85,7 +87,9 @@ typedef unsigned long sigset_t;
#define SA_NOMASK SA_NODEFER
#define SA_ONESHOT SA_RESETHAND
-#define SA_RESTORER 0x04000000
+#define SA_RESTORER 0x04000000u
+#define SA_IA32_ABI 0x02000000u
+#define SA_X32_ABI 0x01000000u
#define MINSIGSTKSZ 2048
#define SIGSTKSZ 8192
diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
index aa31265aa61d..8d13556f7df2 100644
--- a/arch/x86/kernel/signal.c
+++ b/arch/x86/kernel/signal.c
@@ -548,7 +548,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
return -EFAULT;
if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
- if (copy_siginfo_to_user32(&frame->info, &ksig->info))
+ if (__copy_siginfo_to_user32(&frame->info, &ksig->info, true))
return -EFAULT;
}
@@ -661,20 +661,21 @@ badframe:
return 0;
}
-static inline int is_ia32_compat_frame(void)
+static inline int is_ia32_compat_frame(struct ksignal *ksig)
{
return config_enabled(CONFIG_IA32_EMULATION) &&
- test_thread_flag(TIF_IA32);
+ ksig->ka.sa.sa_flags & SA_IA32_ABI;
}
-static inline int is_ia32_frame(void)
+static inline int is_ia32_frame(struct ksignal *ksig)
{
- return config_enabled(CONFIG_X86_32) || is_ia32_compat_frame();
+ return config_enabled(CONFIG_X86_32) || is_ia32_compat_frame(ksig);
}
-static inline int is_x32_frame(void)
+static inline int is_x32_frame(struct ksignal *ksig)
{
- return config_enabled(CONFIG_X86_X32_ABI) && test_thread_flag(TIF_X32);
+ return config_enabled(CONFIG_X86_X32_ABI) &&
+ ksig->ka.sa.sa_flags & SA_X32_ABI;
}
static int
@@ -685,12 +686,12 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
compat_sigset_t *cset = (compat_sigset_t *) set;
/* Set up the stack frame */
- if (is_ia32_frame()) {
+ if (is_ia32_frame(ksig)) {
if (ksig->ka.sa.sa_flags & SA_SIGINFO)
return ia32_setup_rt_frame(usig, ksig, cset, regs);
else
return ia32_setup_frame(usig, ksig, cset, regs);
- } else if (is_x32_frame()) {
+ } else if (is_x32_frame(ksig)) {
return x32_setup_rt_frame(ksig, cset, regs);
} else {
return __setup_rt_frame(ksig->sig, ksig, set, regs);
diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
index dc3c0b1c816f..7f2d7c784a47 100644
--- a/arch/x86/kernel/signal_compat.c
+++ b/arch/x86/kernel/signal_compat.c
@@ -1,10 +1,31 @@
#include <linux/compat.h>
#include <linux/uaccess.h>
+#include <linux/ptrace.h>
-int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
+void sigaction_compat_abi(struct k_sigaction *act)
+{
+ if (!act)
+ return;
+
+ /* don't let to set both ABI flags */
+ if (act->sa.sa_flags & SA_IA32_ABI && act->sa.sa_flags & SA_X32_ABI)
+ act->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI);
+
+ if (user_64bit_mode(current_pt_regs()))
+ return;
+
+ if (!(act->sa.sa_flags & (SA_IA32_ABI | SA_X32_ABI))) {
+ if (in_ia32_syscall())
+ act->sa.sa_flags |= SA_IA32_ABI;
+ if (in_x32_syscall())
+ act->sa.sa_flags |= SA_X32_ABI;
+ }
+}
+
+int __copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from,
+ bool x32_ABI)
{
int err = 0;
- bool ia32 = test_thread_flag(TIF_IA32);
if (!access_ok(VERIFY_WRITE, to, sizeof(compat_siginfo_t)))
return -EFAULT;
@@ -38,7 +59,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
put_user_ex(from->si_arch, &to->si_arch);
break;
case __SI_CHLD >> 16:
- if (ia32) {
+ if (!x32_ABI) {
put_user_ex(from->si_utime, &to->si_utime);
put_user_ex(from->si_stime, &to->si_stime);
} else {
@@ -72,6 +93,12 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
return err;
}
+/* from syscall's path, where we know the ABI */
+int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
+{
+ return __copy_siginfo_to_user32(to, from, in_x32_syscall());
+}
+
int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
{
int err = 0;
diff --git a/kernel/signal.c b/kernel/signal.c
index aa9bf00749c1..d0309f2a36f0 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -3048,6 +3048,10 @@ void kernel_sigaction(int sig, __sighandler_t action)
}
EXPORT_SYMBOL(kernel_sigaction);
+void __weak sigaction_compat_abi(struct k_sigaction *act)
+{
+}
+
int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
{
struct task_struct *p = current, *t;
@@ -3058,6 +3062,7 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
return -EINVAL;
k = &p->sighand->action[sig-1];
+ sigaction_compat_abi(act);
spin_lock_irq(&p->sighand->siglock);
if (oact)
--
2.8.0
As we have here core registers, use them to determine application's
mode and sizes of register set and elf_prstatus.
Cc: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Alexander Viro <[email protected]>
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Dmitry Safonov <[email protected]>
---
arch/x86/include/asm/compat.h | 8 ++++----
fs/binfmt_elf.c | 18 ++++++++++--------
2 files changed, 14 insertions(+), 12 deletions(-)
diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
index 5a3b2c119ed0..d0b517fc77ff 100644
--- a/arch/x86/include/asm/compat.h
+++ b/arch/x86/include/asm/compat.h
@@ -264,10 +264,10 @@ struct compat_shmid64_ds {
#ifdef CONFIG_X86_X32_ABI
typedef struct user_regs_struct compat_elf_gregset_t;
-#define PR_REG_SIZE(S) (test_thread_flag(TIF_IA32) ? 68 : 216)
-#define PRSTATUS_SIZE(S) (test_thread_flag(TIF_IA32) ? 144 : 296)
-#define SET_PR_FPVALID(S,V) \
- do { *(int *) (((void *) &((S)->pr_reg)) + PR_REG_SIZE(0)) = (V); } \
+#define PR_REG_SIZE(S, R) (!user_64bit_mode(R) ? 68 : 216)
+#define PRSTATUS_SIZE(S, R) (!user_64bit_mode(R) ? 144 : 296)
+#define SET_PR_FPVALID(S, V, R) \
+ do { *(int *) (((void *) &((S)->pr_reg)) + PR_REG_SIZE(0, R)) = (V); } \
while (0)
#define COMPAT_USE_64BIT_TIME \
diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index 81381cc0dd17..6be281ef34f9 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1621,11 +1621,11 @@ static void do_thread_regset_writeback(struct task_struct *task,
}
#ifndef PR_REG_SIZE
-#define PR_REG_SIZE(S) sizeof(S)
+#define PR_REG_SIZE(S, R) sizeof(S)
#endif
#ifndef PRSTATUS_SIZE
-#define PRSTATUS_SIZE(S) sizeof(S)
+#define PRSTATUS_SIZE(S, R) sizeof(S)
#endif
#ifndef PR_REG_PTR
@@ -1633,12 +1633,13 @@ static void do_thread_regset_writeback(struct task_struct *task,
#endif
#ifndef SET_PR_FPVALID
-#define SET_PR_FPVALID(S, V) ((S)->pr_fpvalid = (V))
+#define SET_PR_FPVALID(S, V, R) ((S)->pr_fpvalid = (V))
#endif
static int fill_thread_core_info(struct elf_thread_core_info *t,
const struct user_regset_view *view,
- long signr, size_t *total)
+ long signr, size_t *total,
+ struct pt_regs *regs __maybe_unused)
{
unsigned int i;
@@ -1650,11 +1651,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
*/
fill_prstatus(&t->prstatus, t->task, signr);
(void) view->regsets[0].get(t->task, &view->regsets[0],
- 0, PR_REG_SIZE(t->prstatus.pr_reg),
+ 0, PR_REG_SIZE(t->prstatus.pr_reg, regs),
PR_REG_PTR(&t->prstatus), NULL);
fill_note(&t->notes[0], "CORE", NT_PRSTATUS,
- PRSTATUS_SIZE(t->prstatus), &t->prstatus);
+ PRSTATUS_SIZE(t->prstatus, regs), &t->prstatus);
*total += notesize(&t->notes[0]);
do_thread_regset_writeback(t->task, &view->regsets[0]);
@@ -1684,7 +1685,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
regset->core_note_type,
size, data);
else {
- SET_PR_FPVALID(&t->prstatus, 1);
+ SET_PR_FPVALID(&t->prstatus, 1, regs);
fill_note(&t->notes[i], "CORE",
NT_PRFPREG, size, data);
}
@@ -1770,7 +1771,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
* Now fill in each thread's information.
*/
for (t = info->thread; t != NULL; t = t->next)
- if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
+ if (!fill_thread_core_info(t, view, siginfo->si_signo,
+ &info->size, regs))
return 0;
/*
--
2.8.0
As the task isn't executing at the moment of {GET,SET}REGS,
return regset that corresponds to code selector.
So, for i386 elf binary that changed it's CS to __USER_CS
it will return full x86_64 register set.
That will change ABI: i.e, strace uses returned register size
to determine, in which mode the application is.
With the current ABI that way is buggy:
int main(int argc, char **argv, char **envp)
{
printf("Here we exit\n");
fflush(stdout);
asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
printf("After exit\n");
return 0;
}
This program will confuse strace:
[tst]$ strace ./confuse 2>&1 | tail
brk(0x1ca1000) = 0x1ca1000
write(1, "Here we exit\n", 13Here we exit
) = 13
exit(1) = ?
<... exit resumed> strace: _exit returned!
) = ?
write(1, "After exit\n", 11After exit
) = 11
exit_group(0) = ?
+++ exited with 0 +++
So this ABI change should make PTRACE_GETREGSET more reliable and
this will be another step to drop TIF_{IA32,X32} flags.
Cc: Andy Lutomirski <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Thomas Gleixner <[email protected]>
Cc: "H. Peter Anvin" <[email protected]>
Cc: Oleg Nesterov <[email protected]>
Cc: [email protected]
Signed-off-by: Dmitry Safonov <[email protected]>
---
arch/x86/kernel/ptrace.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
index 0f4d2a5df2dc..d7d72f2f8b46 100644
--- a/arch/x86/kernel/ptrace.c
+++ b/arch/x86/kernel/ptrace.c
@@ -1387,7 +1387,7 @@ void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
const struct user_regset_view *task_user_regset_view(struct task_struct *task)
{
#ifdef CONFIG_IA32_EMULATION
- if (test_tsk_thread_flag(task, TIF_IA32))
+ if (!user_64bit_mode(task_pt_regs(task)))
#endif
#if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
return &user_x86_32_view;
--
2.8.0
On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]> wrote:
> As we have here core registers, use them to determine application's
> mode and sizes of register set and elf_prstatus.
Looks fine to me, but I'm not that familiar with this code. Oleg?
>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Alexander Viro <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> arch/x86/include/asm/compat.h | 8 ++++----
> fs/binfmt_elf.c | 18 ++++++++++--------
> 2 files changed, 14 insertions(+), 12 deletions(-)
>
> diff --git a/arch/x86/include/asm/compat.h b/arch/x86/include/asm/compat.h
> index 5a3b2c119ed0..d0b517fc77ff 100644
> --- a/arch/x86/include/asm/compat.h
> +++ b/arch/x86/include/asm/compat.h
> @@ -264,10 +264,10 @@ struct compat_shmid64_ds {
> #ifdef CONFIG_X86_X32_ABI
> typedef struct user_regs_struct compat_elf_gregset_t;
>
> -#define PR_REG_SIZE(S) (test_thread_flag(TIF_IA32) ? 68 : 216)
> -#define PRSTATUS_SIZE(S) (test_thread_flag(TIF_IA32) ? 144 : 296)
> -#define SET_PR_FPVALID(S,V) \
> - do { *(int *) (((void *) &((S)->pr_reg)) + PR_REG_SIZE(0)) = (V); } \
> +#define PR_REG_SIZE(S, R) (!user_64bit_mode(R) ? 68 : 216)
> +#define PRSTATUS_SIZE(S, R) (!user_64bit_mode(R) ? 144 : 296)
> +#define SET_PR_FPVALID(S, V, R) \
> + do { *(int *) (((void *) &((S)->pr_reg)) + PR_REG_SIZE(0, R)) = (V); } \
> while (0)
>
> #define COMPAT_USE_64BIT_TIME \
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 81381cc0dd17..6be281ef34f9 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -1621,11 +1621,11 @@ static void do_thread_regset_writeback(struct task_struct *task,
> }
>
> #ifndef PR_REG_SIZE
> -#define PR_REG_SIZE(S) sizeof(S)
> +#define PR_REG_SIZE(S, R) sizeof(S)
> #endif
>
> #ifndef PRSTATUS_SIZE
> -#define PRSTATUS_SIZE(S) sizeof(S)
> +#define PRSTATUS_SIZE(S, R) sizeof(S)
> #endif
>
> #ifndef PR_REG_PTR
> @@ -1633,12 +1633,13 @@ static void do_thread_regset_writeback(struct task_struct *task,
> #endif
>
> #ifndef SET_PR_FPVALID
> -#define SET_PR_FPVALID(S, V) ((S)->pr_fpvalid = (V))
> +#define SET_PR_FPVALID(S, V, R) ((S)->pr_fpvalid = (V))
> #endif
>
> static int fill_thread_core_info(struct elf_thread_core_info *t,
> const struct user_regset_view *view,
> - long signr, size_t *total)
> + long signr, size_t *total,
> + struct pt_regs *regs __maybe_unused)
> {
> unsigned int i;
>
> @@ -1650,11 +1651,11 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
> */
> fill_prstatus(&t->prstatus, t->task, signr);
> (void) view->regsets[0].get(t->task, &view->regsets[0],
> - 0, PR_REG_SIZE(t->prstatus.pr_reg),
> + 0, PR_REG_SIZE(t->prstatus.pr_reg, regs),
> PR_REG_PTR(&t->prstatus), NULL);
>
> fill_note(&t->notes[0], "CORE", NT_PRSTATUS,
> - PRSTATUS_SIZE(t->prstatus), &t->prstatus);
> + PRSTATUS_SIZE(t->prstatus, regs), &t->prstatus);
> *total += notesize(&t->notes[0]);
>
> do_thread_regset_writeback(t->task, &view->regsets[0]);
> @@ -1684,7 +1685,7 @@ static int fill_thread_core_info(struct elf_thread_core_info *t,
> regset->core_note_type,
> size, data);
> else {
> - SET_PR_FPVALID(&t->prstatus, 1);
> + SET_PR_FPVALID(&t->prstatus, 1, regs);
> fill_note(&t->notes[i], "CORE",
> NT_PRFPREG, size, data);
> }
> @@ -1770,7 +1771,8 @@ static int fill_note_info(struct elfhdr *elf, int phdrs,
> * Now fill in each thread's information.
> */
> for (t = info->thread; t != NULL; t = t->next)
> - if (!fill_thread_core_info(t, view, siginfo->si_signo, &info->size))
> + if (!fill_thread_core_info(t, view, siginfo->si_signo,
> + &info->size, regs))
> return 0;
>
> /*
> --
> 2.8.0
>
--
Andy Lutomirski
AMA Capital Management, LLC
On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]> wrote:
> As the task isn't executing at the moment of {GET,SET}REGS,
> return regset that corresponds to code selector.
> So, for i386 elf binary that changed it's CS to __USER_CS
> it will return full x86_64 register set.
>
> That will change ABI: i.e, strace uses returned register size
> to determine, in which mode the application is.
> With the current ABI that way is buggy:
Oleg, any comment here?
>
> int main(int argc, char **argv, char **envp)
> {
> printf("Here we exit\n");
> fflush(stdout);
> asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
> printf("After exit\n");
>
> return 0;
> }
>
> This program will confuse strace:
>
> [tst]$ strace ./confuse 2>&1 | tail
> brk(0x1ca1000) = 0x1ca1000
> write(1, "Here we exit\n", 13Here we exit
> ) = 13
> exit(1) = ?
> <... exit resumed> strace: _exit returned!
> ) = ?
> write(1, "After exit\n", 11After exit
> ) = 11
> exit_group(0) = ?
> +++ exited with 0 +++
>
> So this ABI change should make PTRACE_GETREGSET more reliable and
> this will be another step to drop TIF_{IA32,X32} flags.
Does strace start working again with this change? I suspect that
we'll eventually have to expose syscall_get_arch directly through
ptrace, but that's a project for another day.
I think this patch is fine, but I'm not a ptrace expert.
--Andy
>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: Oleg Nesterov <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> arch/x86/kernel/ptrace.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
> index 0f4d2a5df2dc..d7d72f2f8b46 100644
> --- a/arch/x86/kernel/ptrace.c
> +++ b/arch/x86/kernel/ptrace.c
> @@ -1387,7 +1387,7 @@ void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
> const struct user_regset_view *task_user_regset_view(struct task_struct *task)
> {
> #ifdef CONFIG_IA32_EMULATION
> - if (test_tsk_thread_flag(task, TIF_IA32))
> + if (!user_64bit_mode(task_pt_regs(task)))
> #endif
> #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
> return &user_x86_32_view;
> --
> 2.8.0
>
--
Andy Lutomirski
AMA Capital Management, LLC
On 04/25/2016 07:53 PM, Andy Lutomirski wrote:
> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]> wrote:
>> As the task isn't executing at the moment of {GET,SET}REGS,
>> return regset that corresponds to code selector.
>> So, for i386 elf binary that changed it's CS to __USER_CS
>> it will return full x86_64 register set.
>>
>> That will change ABI: i.e, strace uses returned register size
>> to determine, in which mode the application is.
>> With the current ABI that way is buggy:
> Oleg, any comment here?
>
>> int main(int argc, char **argv, char **envp)
>> {
>> printf("Here we exit\n");
>> fflush(stdout);
>> asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
>> printf("After exit\n");
>>
>> return 0;
>> }
>>
>> This program will confuse strace:
>>
>> [tst]$ strace ./confuse 2>&1 | tail
>> brk(0x1ca1000) = 0x1ca1000
>> write(1, "Here we exit\n", 13Here we exit
>> ) = 13
>> exit(1) = ?
>> <... exit resumed> strace: _exit returned!
>> ) = ?
>> write(1, "After exit\n", 11After exit
>> ) = 11
>> exit_group(0) = ?
>> +++ exited with 0 +++
>>
>> So this ABI change should make PTRACE_GETREGSET more reliable and
>> this will be another step to drop TIF_{IA32,X32} flags.
> Does strace start working again with this change? I suspect that
> we'll eventually have to expose syscall_get_arch directly through
> ptrace, but that's a project for another day.
Oh, crap, not yet - seems like, I failed with my test.
I'll resend this patch as will get it fixed, sorry.
> I think this patch is fine, but I'm not a ptrace expert.
>
>> Cc: Andy Lutomirski <[email protected]>
>> Cc: Ingo Molnar <[email protected]>
>> Cc: Thomas Gleixner <[email protected]>
>> Cc: "H. Peter Anvin" <[email protected]>
>> Cc: Oleg Nesterov <[email protected]>
>> Cc: [email protected]
>> Signed-off-by: Dmitry Safonov <[email protected]>
>> ---
>> arch/x86/kernel/ptrace.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
>> index 0f4d2a5df2dc..d7d72f2f8b46 100644
>> --- a/arch/x86/kernel/ptrace.c
>> +++ b/arch/x86/kernel/ptrace.c
>> @@ -1387,7 +1387,7 @@ void update_regset_xstate_info(unsigned int size, u64 xstate_mask)
>> const struct user_regset_view *task_user_regset_view(struct task_struct *task)
>> {
>> #ifdef CONFIG_IA32_EMULATION
>> - if (test_tsk_thread_flag(task, TIF_IA32))
>> + if (!user_64bit_mode(task_pt_regs(task)))
>> #endif
>> #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
>> return &user_x86_32_view;
>> --
>> 2.8.0
>>
--
Regards,
Dmitry Safonov
On 04/25/2016 08:14 PM, Dmitry Safonov wrote:
> On 04/25/2016 07:53 PM, Andy Lutomirski wrote:
>> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov
>> <[email protected]> wrote:
>>> As the task isn't executing at the moment of {GET,SET}REGS,
>>> return regset that corresponds to code selector.
>>> So, for i386 elf binary that changed it's CS to __USER_CS
>>> it will return full x86_64 register set.
>>>
>>> That will change ABI: i.e, strace uses returned register size
>>> to determine, in which mode the application is.
>>> With the current ABI that way is buggy:
>> Oleg, any comment here?
>>
>>> int main(int argc, char **argv, char **envp)
>>> {
>>> printf("Here we exit\n");
>>> fflush(stdout);
>>> asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
>>> printf("After exit\n");
>>>
>>> return 0;
>>> }
>>>
>>> This program will confuse strace:
>>>
>>> [tst]$ strace ./confuse 2>&1 | tail
>>> brk(0x1ca1000) = 0x1ca1000
>>> write(1, "Here we exit\n", 13Here we exit
>>> ) = 13
>>> exit(1) = ?
>>> <... exit resumed> strace: _exit returned!
>>> ) = ?
>>> write(1, "After exit\n", 11After exit
>>> ) = 11
>>> exit_group(0) = ?
>>> +++ exited with 0 +++
>>>
>>> So this ABI change should make PTRACE_GETREGSET more reliable and
>>> this will be another step to drop TIF_{IA32,X32} flags.
>> Does strace start working again with this change? I suspect that
>> we'll eventually have to expose syscall_get_arch directly through
>> ptrace, but that's a project for another day.
>
> Oh, crap, not yet - seems like, I failed with my test.
> I'll resend this patch as will get it fixed, sorry.
I find out, what I have changed (and broke test):
> if (!user_64bit_mode(task_pt_regs(task)))
was
> if (task_thread_info(task)->status & TS_COMPAT)
That way the test runs now:
> brk(NULL) = 0x1145000
> brk(0x1167000) = 0x1167000
> write(1, "Here we exit\n", 13Here we exit
> ) = 13
> strace: [ Process PID=1608 runs in 32 bit mode. ]
> umask(0) = 022
> strace: [ Process PID=1608 runs in 64 bit mode. ]
> write(1, "After exit\n", 11After exit
> ) = 11
> exit_group(0) = ?
> +++ exited with 0 +++
But I changed on signal patch rebase and now I'm
thinking: should it be
> if (task_thread_info(task)->status & TS_COMPAT ||
!user_64bit_mode(task_pt_regs(task)))
or what?
Should we count program that does compat syscall
as compatible even if it's in 64-bit mode?
>
>> I think this patch is fine, but I'm not a ptrace expert.
>>
>>> Cc: Andy Lutomirski <[email protected]>
>>> Cc: Ingo Molnar <[email protected]>
>>> Cc: Thomas Gleixner <[email protected]>
>>> Cc: "H. Peter Anvin" <[email protected]>
>>> Cc: Oleg Nesterov <[email protected]>
>>> Cc: [email protected]
>>> Signed-off-by: Dmitry Safonov <[email protected]>
>>> ---
>>> arch/x86/kernel/ptrace.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/arch/x86/kernel/ptrace.c b/arch/x86/kernel/ptrace.c
>>> index 0f4d2a5df2dc..d7d72f2f8b46 100644
>>> --- a/arch/x86/kernel/ptrace.c
>>> +++ b/arch/x86/kernel/ptrace.c
>>> @@ -1387,7 +1387,7 @@ void update_regset_xstate_info(unsigned int
>>> size, u64 xstate_mask)
>>> const struct user_regset_view *task_user_regset_view(struct
>>> task_struct *task)
>>> {
>>> #ifdef CONFIG_IA32_EMULATION
>>> - if (test_tsk_thread_flag(task, TIF_IA32))
>>> + if (!user_64bit_mode(task_pt_regs(task)))
>>> #endif
>>> #if defined CONFIG_X86_32 || defined CONFIG_IA32_EMULATION
>>> return &user_x86_32_view;
>>> --
>>> 2.8.0
>>>
>
--
Regards,
Dmitry Safonov
On 04/25/2016 09:09 PM, Dmitry Safonov wrote:
> On 04/25/2016 08:14 PM, Dmitry Safonov wrote:
>> On 04/25/2016 07:53 PM, Andy Lutomirski wrote:
>>> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov
>>> <[email protected]> wrote:
>>>> As the task isn't executing at the moment of {GET,SET}REGS,
>>>> return regset that corresponds to code selector.
>>>> So, for i386 elf binary that changed it's CS to __USER_CS
>>>> it will return full x86_64 register set.
>>>>
>>>> That will change ABI: i.e, strace uses returned register size
>>>> to determine, in which mode the application is.
>>>> With the current ABI that way is buggy:
>>> Oleg, any comment here?
>>>
>>>> int main(int argc, char **argv, char **envp)
>>>> {
>>>> printf("Here we exit\n");
>>>> fflush(stdout);
>>>> asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
>>>> printf("After exit\n");
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> This program will confuse strace:
>>>>
>>>> [tst]$ strace ./confuse 2>&1 | tail
>>>> brk(0x1ca1000) = 0x1ca1000
>>>> write(1, "Here we exit\n", 13Here we exit
>>>> ) = 13
>>>> exit(1) = ?
>>>> <... exit resumed> strace: _exit returned!
>>>> ) = ?
>>>> write(1, "After exit\n", 11After exit
>>>> ) = 11
>>>> exit_group(0) = ?
>>>> +++ exited with 0 +++
>>>>
>>>> So this ABI change should make PTRACE_GETREGSET more reliable and
>>>> this will be another step to drop TIF_{IA32,X32} flags.
>>> Does strace start working again with this change? I suspect that
>>> we'll eventually have to expose syscall_get_arch directly through
>>> ptrace, but that's a project for another day.
>>
>> Oh, crap, not yet - seems like, I failed with my test.
>> I'll resend this patch as will get it fixed, sorry.
>
> I find out, what I have changed (and broke test):
> > if (!user_64bit_mode(task_pt_regs(task)))
> was
> > if (task_thread_info(task)->status & TS_COMPAT)
>
> That way the test runs now:
>> brk(NULL) = 0x1145000
>> brk(0x1167000) = 0x1167000
>> write(1, "Here we exit\n", 13Here we exit
>> ) = 13
>> strace: [ Process PID=1608 runs in 32 bit mode. ]
>> umask(0) = 022
And that seems to be right as __NR_exit for amd64
is 60, which is the same as __NR_umask for i386.
>> strace: [ Process PID=1608 runs in 64 bit mode. ]
>> write(1, "After exit\n", 11After exit
>> ) = 11
>> exit_group(0) = ?
>> +++ exited with 0 +++
>
> But I changed on signal patch rebase and now I'm
> thinking: should it be
> > if (task_thread_info(task)->status & TS_COMPAT ||
> !user_64bit_mode(task_pt_regs(task)))
> or what?
> Should we count program that does compat syscall
> as compatible even if it's in 64-bit mode?
>
--
Regards,
Dmitry Safonov
On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]> wrote:
> Introduce new flags that defines which ABI to use on creating sigframe.
> Those flags one may set from the userspace, or kernel will set them
> according to syscall, which sets handler for a signal.
> So that will drop the dependency on TIF_IA32/TIF_X32 flags on syscall deliver.
> Those flags will be used only under CONFIG_COMPAT.
>
> The same way ARM uses sa_flags to differ in which mode deliver signal
> for 26-bit applications (look at SA_THIRYTWO).
Hmm. Do we want to make these user-visible at all, or should it be
purely an in-kernel thing?
--Andy
>
> Cc: Andy Lutomirski <[email protected]>
> Cc: Ingo Molnar <[email protected]>
> Cc: Thomas Gleixner <[email protected]>
> Cc: "H. Peter Anvin" <[email protected]>
> Cc: [email protected]
> Signed-off-by: Dmitry Safonov <[email protected]>
> ---
> arch/x86/ia32/ia32_signal.c | 2 +-
> arch/x86/include/asm/fpu/signal.h | 6 ++++++
> arch/x86/include/uapi/asm/signal.h | 6 +++++-
> arch/x86/kernel/signal.c | 19 ++++++++++---------
> arch/x86/kernel/signal_compat.c | 33 ++++++++++++++++++++++++++++++---
> kernel/signal.c | 5 +++++
> 6 files changed, 57 insertions(+), 14 deletions(-)
>
> diff --git a/arch/x86/ia32/ia32_signal.c b/arch/x86/ia32/ia32_signal.c
> index 0552884da18d..faf1871300cb 100644
> --- a/arch/x86/ia32/ia32_signal.c
> +++ b/arch/x86/ia32/ia32_signal.c
> @@ -378,7 +378,7 @@ int ia32_setup_rt_frame(int sig, struct ksignal *ksig,
> put_user_ex(*((u64 *)&code), (u64 __user *)frame->retcode);
> } put_user_catch(err);
>
> - err |= copy_siginfo_to_user32(&frame->info, &ksig->info);
> + err |= __copy_siginfo_to_user32(&frame->info, &ksig->info, false);
> err |= ia32_setup_sigcontext(&frame->uc.uc_mcontext, fpstate,
> regs, set->sig[0]);
> err |= __copy_to_user(&frame->uc.uc_sigmask, set, sizeof(*set));
> diff --git a/arch/x86/include/asm/fpu/signal.h b/arch/x86/include/asm/fpu/signal.h
> index 0e970d00dfcd..20a1fbf7fe4e 100644
> --- a/arch/x86/include/asm/fpu/signal.h
> +++ b/arch/x86/include/asm/fpu/signal.h
> @@ -19,6 +19,12 @@ int ia32_setup_frame(int sig, struct ksignal *ksig,
> # define ia32_setup_rt_frame __setup_rt_frame
> #endif
>
> +#ifdef CONFIG_COMPAT
> +int __copy_siginfo_to_user32(compat_siginfo_t __user *to,
> + const siginfo_t *from, bool x32_ABI);
> +#endif
> +
> +
> extern void convert_from_fxsr(struct user_i387_ia32_struct *env,
> struct task_struct *tsk);
> extern void convert_to_fxsr(struct task_struct *tsk,
> diff --git a/arch/x86/include/uapi/asm/signal.h b/arch/x86/include/uapi/asm/signal.h
> index 8264f47cf53e..9c663b6fc023 100644
> --- a/arch/x86/include/uapi/asm/signal.h
> +++ b/arch/x86/include/uapi/asm/signal.h
> @@ -70,6 +70,8 @@ typedef unsigned long sigset_t;
> * SA_RESETHAND clears the handler when the signal is delivered.
> * SA_NOCLDWAIT flag on SIGCHLD to inhibit zombies.
> * SA_NODEFER prevents the current signal from being masked in the handler.
> + * SA_IA32_ABI/SA_X32_ABI indicates ABI for a signal frame,
> + * if neither is set, the kernel will set them according to a syscall ABI
> *
> * SA_ONESHOT and SA_NOMASK are the historical Linux names for the Single
> * Unix names RESETHAND and NODEFER respectively.
> @@ -85,7 +87,9 @@ typedef unsigned long sigset_t;
> #define SA_NOMASK SA_NODEFER
> #define SA_ONESHOT SA_RESETHAND
>
> -#define SA_RESTORER 0x04000000
> +#define SA_RESTORER 0x04000000u
> +#define SA_IA32_ABI 0x02000000u
> +#define SA_X32_ABI 0x01000000u
>
> #define MINSIGSTKSZ 2048
> #define SIGSTKSZ 8192
> diff --git a/arch/x86/kernel/signal.c b/arch/x86/kernel/signal.c
> index aa31265aa61d..8d13556f7df2 100644
> --- a/arch/x86/kernel/signal.c
> +++ b/arch/x86/kernel/signal.c
> @@ -548,7 +548,7 @@ static int x32_setup_rt_frame(struct ksignal *ksig,
> return -EFAULT;
>
> if (ksig->ka.sa.sa_flags & SA_SIGINFO) {
> - if (copy_siginfo_to_user32(&frame->info, &ksig->info))
> + if (__copy_siginfo_to_user32(&frame->info, &ksig->info, true))
> return -EFAULT;
> }
>
> @@ -661,20 +661,21 @@ badframe:
> return 0;
> }
>
> -static inline int is_ia32_compat_frame(void)
> +static inline int is_ia32_compat_frame(struct ksignal *ksig)
> {
> return config_enabled(CONFIG_IA32_EMULATION) &&
> - test_thread_flag(TIF_IA32);
> + ksig->ka.sa.sa_flags & SA_IA32_ABI;
> }
>
> -static inline int is_ia32_frame(void)
> +static inline int is_ia32_frame(struct ksignal *ksig)
> {
> - return config_enabled(CONFIG_X86_32) || is_ia32_compat_frame();
> + return config_enabled(CONFIG_X86_32) || is_ia32_compat_frame(ksig);
> }
>
> -static inline int is_x32_frame(void)
> +static inline int is_x32_frame(struct ksignal *ksig)
> {
> - return config_enabled(CONFIG_X86_X32_ABI) && test_thread_flag(TIF_X32);
> + return config_enabled(CONFIG_X86_X32_ABI) &&
> + ksig->ka.sa.sa_flags & SA_X32_ABI;
> }
>
> static int
> @@ -685,12 +686,12 @@ setup_rt_frame(struct ksignal *ksig, struct pt_regs *regs)
> compat_sigset_t *cset = (compat_sigset_t *) set;
>
> /* Set up the stack frame */
> - if (is_ia32_frame()) {
> + if (is_ia32_frame(ksig)) {
> if (ksig->ka.sa.sa_flags & SA_SIGINFO)
> return ia32_setup_rt_frame(usig, ksig, cset, regs);
> else
> return ia32_setup_frame(usig, ksig, cset, regs);
> - } else if (is_x32_frame()) {
> + } else if (is_x32_frame(ksig)) {
> return x32_setup_rt_frame(ksig, cset, regs);
> } else {
> return __setup_rt_frame(ksig->sig, ksig, set, regs);
> diff --git a/arch/x86/kernel/signal_compat.c b/arch/x86/kernel/signal_compat.c
> index dc3c0b1c816f..7f2d7c784a47 100644
> --- a/arch/x86/kernel/signal_compat.c
> +++ b/arch/x86/kernel/signal_compat.c
> @@ -1,10 +1,31 @@
> #include <linux/compat.h>
> #include <linux/uaccess.h>
> +#include <linux/ptrace.h>
>
> -int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> +void sigaction_compat_abi(struct k_sigaction *act)
> +{
> + if (!act)
> + return;
> +
> + /* don't let to set both ABI flags */
> + if (act->sa.sa_flags & SA_IA32_ABI && act->sa.sa_flags & SA_X32_ABI)
> + act->sa.sa_flags &= ~(SA_IA32_ABI | SA_X32_ABI);
> +
> + if (user_64bit_mode(current_pt_regs()))
> + return;
> +
> + if (!(act->sa.sa_flags & (SA_IA32_ABI | SA_X32_ABI))) {
> + if (in_ia32_syscall())
> + act->sa.sa_flags |= SA_IA32_ABI;
> + if (in_x32_syscall())
> + act->sa.sa_flags |= SA_X32_ABI;
> + }
> +}
> +
> +int __copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from,
> + bool x32_ABI)
> {
> int err = 0;
> - bool ia32 = test_thread_flag(TIF_IA32);
>
> if (!access_ok(VERIFY_WRITE, to, sizeof(compat_siginfo_t)))
> return -EFAULT;
> @@ -38,7 +59,7 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> put_user_ex(from->si_arch, &to->si_arch);
> break;
> case __SI_CHLD >> 16:
> - if (ia32) {
> + if (!x32_ABI) {
> put_user_ex(from->si_utime, &to->si_utime);
> put_user_ex(from->si_stime, &to->si_stime);
> } else {
> @@ -72,6 +93,12 @@ int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> return err;
> }
>
> +/* from syscall's path, where we know the ABI */
> +int copy_siginfo_to_user32(compat_siginfo_t __user *to, const siginfo_t *from)
> +{
> + return __copy_siginfo_to_user32(to, from, in_x32_syscall());
> +}
> +
> int copy_siginfo_from_user32(siginfo_t *to, compat_siginfo_t __user *from)
> {
> int err = 0;
> diff --git a/kernel/signal.c b/kernel/signal.c
> index aa9bf00749c1..d0309f2a36f0 100644
> --- a/kernel/signal.c
> +++ b/kernel/signal.c
> @@ -3048,6 +3048,10 @@ void kernel_sigaction(int sig, __sighandler_t action)
> }
> EXPORT_SYMBOL(kernel_sigaction);
>
> +void __weak sigaction_compat_abi(struct k_sigaction *act)
> +{
> +}
> +
> int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
> {
> struct task_struct *p = current, *t;
> @@ -3058,6 +3062,7 @@ int do_sigaction(int sig, struct k_sigaction *act, struct k_sigaction *oact)
> return -EINVAL;
>
> k = &p->sighand->action[sig-1];
> + sigaction_compat_abi(act);
>
> spin_lock_irq(&p->sighand->siglock);
> if (oact)
> --
> 2.8.0
>
--
Andy Lutomirski
AMA Capital Management, LLC
On Mon, Apr 25, 2016 at 11:09 AM, Dmitry Safonov <[email protected]> wrote:
> On 04/25/2016 08:14 PM, Dmitry Safonov wrote:
>>
>> On 04/25/2016 07:53 PM, Andy Lutomirski wrote:
>>>
>>> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]>
>>> wrote:
>>>>
>>>> As the task isn't executing at the moment of {GET,SET}REGS,
>>>> return regset that corresponds to code selector.
>>>> So, for i386 elf binary that changed it's CS to __USER_CS
>>>> it will return full x86_64 register set.
>>>>
>>>> That will change ABI: i.e, strace uses returned register size
>>>> to determine, in which mode the application is.
>>>> With the current ABI that way is buggy:
>>>
>>> Oleg, any comment here?
>>>
>>>> int main(int argc, char **argv, char **envp)
>>>> {
>>>> printf("Here we exit\n");
>>>> fflush(stdout);
>>>> asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
>>>> printf("After exit\n");
>>>>
>>>> return 0;
>>>> }
>>>>
>>>> This program will confuse strace:
>>>>
>>>> [tst]$ strace ./confuse 2>&1 | tail
>>>> brk(0x1ca1000) = 0x1ca1000
>>>> write(1, "Here we exit\n", 13Here we exit
>>>> ) = 13
>>>> exit(1) = ?
>>>> <... exit resumed> strace: _exit returned!
>>>> ) = ?
>>>> write(1, "After exit\n", 11After exit
>>>> ) = 11
>>>> exit_group(0) = ?
>>>> +++ exited with 0 +++
>>>>
>>>> So this ABI change should make PTRACE_GETREGSET more reliable and
>>>> this will be another step to drop TIF_{IA32,X32} flags.
>>>
>>> Does strace start working again with this change? I suspect that
>>> we'll eventually have to expose syscall_get_arch directly through
>>> ptrace, but that's a project for another day.
>>
>>
>> Oh, crap, not yet - seems like, I failed with my test.
>> I'll resend this patch as will get it fixed, sorry.
>
>
> I find out, what I have changed (and broke test):
>> if (!user_64bit_mode(task_pt_regs(task)))
> was
>> if (task_thread_info(task)->status & TS_COMPAT)
>
> That way the test runs now:
>>
>> brk(NULL) = 0x1145000
>> brk(0x1167000) = 0x1167000
>> write(1, "Here we exit\n", 13Here we exit
>> ) = 13
>> strace: [ Process PID=1608 runs in 32 bit mode. ]
>> umask(0) = 022
>> strace: [ Process PID=1608 runs in 64 bit mode. ]
>> write(1, "After exit\n", 11After exit
>> ) = 11
>> exit_group(0) = ?
>> +++ exited with 0 +++
>
>
> But I changed on signal patch rebase and now I'm
> thinking: should it be
>> if (task_thread_info(task)->status & TS_COMPAT ||
>> !user_64bit_mode(task_pt_regs(task)))
> or what?
> Should we count program that does compat syscall
> as compatible even if it's in 64-bit mode?
I think we should report 64-bit regs if the app is running in 64-bit
mode. Then (not necessarily as part of your series), we should have a
way for ptrace users to query the *syscall* arch.
IOW, this is totally screwed up right now. I think the goal of your
patch should be to stop using TIF_IA32 without breaking it any worse
than it already is.
--Andy
2016-04-25 22:20 GMT+03:00 Andy Lutomirski <[email protected]>:
> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]> wrote:
>> Introduce new flags that defines which ABI to use on creating sigframe.
>> Those flags one may set from the userspace, or kernel will set them
>> according to syscall, which sets handler for a signal.
>> So that will drop the dependency on TIF_IA32/TIF_X32 flags on syscall deliver.
>> Those flags will be used only under CONFIG_COMPAT.
>>
>> The same way ARM uses sa_flags to differ in which mode deliver signal
>> for 26-bit applications (look at SA_THIRYTWO).
>
> Hmm. Do we want to make these user-visible at all, or should it be
> purely an in-kernel thing?
Yes, I'll rework it to not expose to userspace.
I thought about it as a bonus when did it, but yeah, it's better
not reveal a new interfaces until they really needed.
But anyway, I did it for RFC, and I don't know what's better
for hidden flag: reuse sa_flags or invent in ksig a new hidden
member only for the kernel?
2016-04-25 22:33 GMT+03:00 Andy Lutomirski <[email protected]>:
> On Mon, Apr 25, 2016 at 11:09 AM, Dmitry Safonov <[email protected]> wrote:
>> On 04/25/2016 08:14 PM, Dmitry Safonov wrote:
>>>
>>> On 04/25/2016 07:53 PM, Andy Lutomirski wrote:
>>>>
>>>> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]>
>>>> wrote:
>>>>>
>>>>> As the task isn't executing at the moment of {GET,SET}REGS,
>>>>> return regset that corresponds to code selector.
>>>>> So, for i386 elf binary that changed it's CS to __USER_CS
>>>>> it will return full x86_64 register set.
>>>>>
>>>>> That will change ABI: i.e, strace uses returned register size
>>>>> to determine, in which mode the application is.
>>>>> With the current ABI that way is buggy:
>>>>
>>>> Oleg, any comment here?
>>>>
>>>>> int main(int argc, char **argv, char **envp)
>>>>> {
>>>>> printf("Here we exit\n");
>>>>> fflush(stdout);
>>>>> asm volatile ("int $0x80" : : "a" (__NR_exit), "D" (1));
>>>>> printf("After exit\n");
>>>>>
>>>>> return 0;
>>>>> }
>>>>>
>>>>> This program will confuse strace:
>>>>>
>>>>> [tst]$ strace ./confuse 2>&1 | tail
>>>>> brk(0x1ca1000) = 0x1ca1000
>>>>> write(1, "Here we exit\n", 13Here we exit
>>>>> ) = 13
>>>>> exit(1) = ?
>>>>> <... exit resumed> strace: _exit returned!
>>>>> ) = ?
>>>>> write(1, "After exit\n", 11After exit
>>>>> ) = 11
>>>>> exit_group(0) = ?
>>>>> +++ exited with 0 +++
>>>>>
>>>>> So this ABI change should make PTRACE_GETREGSET more reliable and
>>>>> this will be another step to drop TIF_{IA32,X32} flags.
>>>>
>>>> Does strace start working again with this change? I suspect that
>>>> we'll eventually have to expose syscall_get_arch directly through
>>>> ptrace, but that's a project for another day.
>>>
>>>
>>> Oh, crap, not yet - seems like, I failed with my test.
>>> I'll resend this patch as will get it fixed, sorry.
>>
>>
>> I find out, what I have changed (and broke test):
>>> if (!user_64bit_mode(task_pt_regs(task)))
>> was
>>> if (task_thread_info(task)->status & TS_COMPAT)
>>
>> That way the test runs now:
>>>
>>> brk(NULL) = 0x1145000
>>> brk(0x1167000) = 0x1167000
>>> write(1, "Here we exit\n", 13Here we exit
>>> ) = 13
>>> strace: [ Process PID=1608 runs in 32 bit mode. ]
>>> umask(0) = 022
>>> strace: [ Process PID=1608 runs in 64 bit mode. ]
>>> write(1, "After exit\n", 11After exit
>>> ) = 11
>>> exit_group(0) = ?
>>> +++ exited with 0 +++
>>
>>
>> But I changed on signal patch rebase and now I'm
>> thinking: should it be
>>> if (task_thread_info(task)->status & TS_COMPAT ||
>>> !user_64bit_mode(task_pt_regs(task)))
>> or what?
>> Should we count program that does compat syscall
>> as compatible even if it's in 64-bit mode?
>
> I think we should report 64-bit regs if the app is running in 64-bit
> mode. Then (not necessarily as part of your series), we should have a
> way for ptrace users to query the *syscall* arch.
>
> IOW, this is totally screwed up right now. I think the goal of your
> patch should be to stop using TIF_IA32 without breaking it any worse
> than it already is.
Ok, so that's what that patch does.
Big thanks, Andy -- I appreciate your help so much with this patches.
--
Regards,
Safonov Dmitry.
On Mon, Apr 25, 2016 at 1:34 PM, Dmitry Safonov <[email protected]> wrote:
> 2016-04-25 22:20 GMT+03:00 Andy Lutomirski <[email protected]>:
>> On Mon, Apr 25, 2016 at 9:12 AM, Dmitry Safonov <[email protected]> wrote:
>>> Introduce new flags that defines which ABI to use on creating sigframe.
>>> Those flags one may set from the userspace, or kernel will set them
>>> according to syscall, which sets handler for a signal.
>>> So that will drop the dependency on TIF_IA32/TIF_X32 flags on syscall deliver.
>>> Those flags will be used only under CONFIG_COMPAT.
>>>
>>> The same way ARM uses sa_flags to differ in which mode deliver signal
>>> for 26-bit applications (look at SA_THIRYTWO).
>>
>> Hmm. Do we want to make these user-visible at all, or should it be
>> purely an in-kernel thing?
>
> Yes, I'll rework it to not expose to userspace.
> I thought about it as a bonus when did it, but yeah, it's better
> not reveal a new interfaces until they really needed.
> But anyway, I did it for RFC, and I don't know what's better
> for hidden flag: reuse sa_flags or invent in ksig a new hidden
> member only for the kernel?
Either is fine with me. If you hide it in sa_flags, make sure to mask
it off in the syscalls.
--
Andy Lutomirski
AMA Capital Management, LLC