| Message ID | 20170604120009.342-19-ynorov@caviumnetworks.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Yury, On 04/06/17 13:00, Yury Norov wrote: > ILP32 has context-related structures different from both aarch32 and > aarch64/lp64. In this patch compat_arch_ptrace() renamed to > compat_a32_ptrace(), and compat_arch_ptrace() only makes choice between > compat_a32_ptrace() and new compat_ilp32_ptrace() handler. > > compat_ilp32_ptrace() calls generic compat_ptrace_request() for all > requests except PTRACE_GETSIGMASK and PTRACE_SETSIGMASK, which need > special handling. Can you elaborate on this special handling? How come we don't need to wrap PTRACE_{G,S}ETSIGMASK for aarch32 compat? From kernel/signal32.c that uses compat_sigset_t too. It looks like aarch64, ilp32 and aarch32 all use the same size sigset_t, so doesn't compat_ptrace_request() already do everything we need? ... Is this fixing an endian problem? If so, can we document it as such. Do we already have the same bug for aarch32 compat? Thanks, James
On Fri, Jun 23, 2017 at 06:03:37PM +0100, James Morse wrote: > Hi Yury, > > On 04/06/17 13:00, Yury Norov wrote: > > ILP32 has context-related structures different from both aarch32 and > > aarch64/lp64. In this patch compat_arch_ptrace() renamed to > > compat_a32_ptrace(), and compat_arch_ptrace() only makes choice between > > compat_a32_ptrace() and new compat_ilp32_ptrace() handler. > > > > compat_ilp32_ptrace() calls generic compat_ptrace_request() for all > > requests except PTRACE_GETSIGMASK and PTRACE_SETSIGMASK, which need > > special handling. > > Can you elaborate on this special handling? > > How come we don't need to wrap PTRACE_{G,S}ETSIGMASK for aarch32 compat? > >From kernel/signal32.c that uses compat_sigset_t too. > > It looks like aarch64, ilp32 and aarch32 all use the same size sigset_t, > so doesn't compat_ptrace_request() already do everything we need? > > ... > > Is this fixing an endian problem? If so, can we document it as such. Do we > already have the same bug for aarch32 compat? Originally, the problem was found by Zhou Chengming: https://lkml.org/lkml/2016/6/27/18 But I think you right, this is the fix for endian. It lookd like aarch32 is buggy, but IIUC to confirm it, the BE arm64 machine is needed. I use qemu and AFAIR it has no BE support. Zhou, can you test it on your machine and if the bug will be reproduced, send the patch for aarch32? Yury
Hi Yury, Zhou, On 23/06/17 23:28, Yury Norov wrote: > On Fri, Jun 23, 2017 at 06:03:37PM +0100, James Morse wrote: >> Hi Yury, >> >> On 04/06/17 13:00, Yury Norov wrote: >>> ILP32 has context-related structures different from both aarch32 and >>> aarch64/lp64. In this patch compat_arch_ptrace() renamed to >>> compat_a32_ptrace(), and compat_arch_ptrace() only makes choice between >>> compat_a32_ptrace() and new compat_ilp32_ptrace() handler. >>> >>> compat_ilp32_ptrace() calls generic compat_ptrace_request() for all >>> requests except PTRACE_GETSIGMASK and PTRACE_SETSIGMASK, which need >>> special handling. >> >> Can you elaborate on this special handling? >> >> How come we don't need to wrap PTRACE_{G,S}ETSIGMASK for aarch32 compat? >> >From kernel/signal32.c that uses compat_sigset_t too. >> >> It looks like aarch64, ilp32 and aarch32 all use the same size sigset_t, >> so doesn't compat_ptrace_request() already do everything we need? >> >> ... >> >> Is this fixing an endian problem? If so, can we document it as such. Do we >> already have the same bug for aarch32 compat? > > Originally, the problem was found by Zhou Chengming: https://lkml.org/lkml/2016/6/27/18 > But I think you right, this is the fix for endian. > > It lookd like aarch32 is buggy, but IIUC to confirm it, the BE arm64 > machine is needed. I use qemu and AFAIR it has no BE support. > > Zhou, can you test it on your machine and if the bug will be reproduced, > send the patch for aarch32? I've reproduced this on big endian compat-aarch32: yes its broken. I will respin Zhou's patch as a fix. Thanks, James
diff --git a/arch/arm64/kernel/ptrace.c b/arch/arm64/kernel/ptrace.c index e2b7c040bf84..28f96765e8cc 100644 --- a/arch/arm64/kernel/ptrace.c +++ b/arch/arm64/kernel/ptrace.c @@ -765,9 +765,11 @@ static const struct user_regset_view user_aarch64_view = { .regsets = aarch64_regsets, .n = ARRAY_SIZE(aarch64_regsets) }; -#ifdef CONFIG_AARCH32_EL0 +#ifdef CONFIG_COMPAT #include <linux/compat.h> +#endif +#ifdef CONFIG_AARCH32_EL0 enum compat_regset { REGSET_COMPAT_GPR, REGSET_COMPAT_VFP, @@ -1223,7 +1225,7 @@ static int compat_ptrace_sethbpregs(struct task_struct *tsk, compat_long_t num, } #endif /* CONFIG_HAVE_HW_BREAKPOINT */ -long compat_arch_ptrace(struct task_struct *child, compat_long_t request, +static long compat_a32_ptrace(struct task_struct *child, compat_long_t request, compat_ulong_t caddr, compat_ulong_t cdata) { unsigned long addr = caddr; @@ -1300,8 +1302,67 @@ long compat_arch_ptrace(struct task_struct *child, compat_long_t request, return ret; } + +#else +#define compat_a32_ptrace(child, request, caddr, cdata) (0) #endif /* CONFIG_AARCH32_EL0 */ +#ifdef CONFIG_ARM64_ILP32 +#include <asm/signal32_common.h> + +static long compat_ilp32_ptrace(struct task_struct *child, compat_long_t request, + compat_ulong_t caddr, compat_ulong_t cdata) +{ + sigset_t new_set; + + switch (request) { + case PTRACE_GETSIGMASK: + if (caddr != sizeof(compat_sigset_t)) + return -EINVAL; + + return put_sigset_t((compat_sigset_t __user *) (u64) cdata, + &child->blocked); + + case PTRACE_SETSIGMASK: + if (caddr != sizeof(compat_sigset_t)) + return -EINVAL; + + if (get_sigset_t(&new_set, (compat_sigset_t __user *) (u64) cdata)) + return -EFAULT; + + sigdelsetmask(&new_set, sigmask(SIGKILL)|sigmask(SIGSTOP)); + + /* + * Every thread does recalc_sigpending() after resume, so + * retarget_shared_pending() and recalc_sigpending() are not + * called here. + */ + spin_lock_irq(&child->sighand->siglock); + child->blocked = new_set; + spin_unlock_irq(&child->sighand->siglock); + + return 0; + + default: + return compat_ptrace_request(child, request, caddr, cdata); + } +} + +#else +#define compat_ilp32_ptrace(child, request, caddr, cdata) (0) +#endif + +#ifdef CONFIG_COMPAT +long compat_arch_ptrace(struct task_struct *child, compat_long_t request, + compat_ulong_t caddr, compat_ulong_t cdata) +{ + if (is_a32_compat_task()) + return compat_a32_ptrace(child, request, caddr, cdata); + + return compat_ilp32_ptrace(child, request, caddr, cdata); +} +#endif + const struct user_regset_view *task_user_regset_view(struct task_struct *task) { #ifdef CONFIG_AARCH32_EL0