compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
instead using those in ptrace_request(). The compat variant should
read a compat_sigset_t from userspace instead of ptrace_request()s
sigset_t.
While compat_sigset_t is the same size as sigset_t, it is defined as
2xu32, instead of a single u64. On a big-endian CPU this means that
compat_sigset_t is passed to user-space using middle-endianness,
where the least-significant u32 is written most significant byte
first.
If ptrace_request()s code is used userspace will read the most
significant u32 where it expected the least significant.
Instead of duplicating ptrace_request()s code as a special case in
the arch code, handle it here.
CC: Yury Norov <[email protected]>
CC: Andrey Vagin <[email protected]>
Reported-by: Zhou Chengming <[email protected]>
Signed-off-by: James Morse <[email protected]>
Fixes: 29000caecbe87 ("ptrace: add ability to get/set signal-blocked mask")
---
LTP test case here:
https://lists.linux.it/pipermail/ltp/2017-June/004932.html
kernel/ptrace.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
1 file changed, 40 insertions(+), 12 deletions(-)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 8d2c10714530..a5bebb6713e8 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -843,6 +843,22 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
EXPORT_SYMBOL_GPL(task_user_regset_view);
#endif
+static int ptrace_setsigmask(struct task_struct *child, sigset_t *new_set)
+{
+ 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;
+}
+
int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data)
{
@@ -914,18 +930,7 @@ int ptrace_request(struct task_struct *child, long request,
break;
}
- 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);
-
- ret = 0;
+ ret = ptrace_setsigmask(child, &new_set);
break;
}
@@ -1149,7 +1154,9 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
compat_ulong_t addr, compat_ulong_t data)
{
compat_ulong_t __user *datap = compat_ptr(data);
+ compat_sigset_t set32;
compat_ulong_t word;
+ sigset_t new_set;
siginfo_t siginfo;
int ret;
@@ -1189,6 +1196,27 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
else
ret = ptrace_setsiginfo(child, &siginfo);
break;
+ case PTRACE_GETSIGMASK:
+ if (addr != sizeof(compat_sigset_t))
+ return -EINVAL;
+
+ sigset_to_compat(&set32, &child->blocked);
+
+ if (copy_to_user(datap, &set32, sizeof(set32)))
+ return -EFAULT;
+
+ ret = 0;
+ break;
+ case PTRACE_SETSIGMASK:
+ if (addr != sizeof(compat_sigset_t))
+ return -EINVAL;
+
+ if (copy_from_user(&set32, datap, sizeof(compat_sigset_t)))
+ return -EFAULT;
+
+ sigset_from_compat(&new_set, &set32);
+ ret = ptrace_setsigmask(child, &new_set);
+ break;
#ifdef CONFIG_HAVE_ARCH_TRACEHOOK
case PTRACE_GETREGSET:
case PTRACE_SETREGSET:
--
2.11.0
On Thu, Jun 29, 2017 at 05:26:37PM +0100, James Morse wrote:
> compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
> instead using those in ptrace_request(). The compat variant should
> read a compat_sigset_t from userspace instead of ptrace_request()s
> sigset_t.
>
> While compat_sigset_t is the same size as sigset_t, it is defined as
> 2xu32, instead of a single u64. On a big-endian CPU this means that
> compat_sigset_t is passed to user-space using middle-endianness,
> where the least-significant u32 is written most significant byte
> first.
>
> If ptrace_request()s code is used userspace will read the most
> significant u32 where it expected the least significant.
>
> Instead of duplicating ptrace_request()s code as a special case in
> the arch code, handle it here.
>
Acked-by: Andrei Vagin <[email protected]>
> CC: Yury Norov <[email protected]>
> CC: Andrey Vagin <[email protected]>
> Reported-by: Zhou Chengming <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> Fixes: 29000caecbe87 ("ptrace: add ability to get/set signal-blocked mask")
> ---
> LTP test case here:
> https://lists.linux.it/pipermail/ltp/2017-June/004932.html
>
> kernel/ptrace.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 8d2c10714530..a5bebb6713e8 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -843,6 +843,22 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> EXPORT_SYMBOL_GPL(task_user_regset_view);
> #endif
>
> +static int ptrace_setsigmask(struct task_struct *child, sigset_t *new_set)
> +{
> + 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;
> +}
> +
> int ptrace_request(struct task_struct *child, long request,
> unsigned long addr, unsigned long data)
> {
> @@ -914,18 +930,7 @@ int ptrace_request(struct task_struct *child, long request,
> break;
> }
>
> - 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);
> -
> - ret = 0;
> + ret = ptrace_setsigmask(child, &new_set);
> break;
> }
>
> @@ -1149,7 +1154,9 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
> compat_ulong_t addr, compat_ulong_t data)
> {
> compat_ulong_t __user *datap = compat_ptr(data);
> + compat_sigset_t set32;
> compat_ulong_t word;
> + sigset_t new_set;
> siginfo_t siginfo;
> int ret;
>
> @@ -1189,6 +1196,27 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
> else
> ret = ptrace_setsiginfo(child, &siginfo);
> break;
> + case PTRACE_GETSIGMASK:
> + if (addr != sizeof(compat_sigset_t))
> + return -EINVAL;
> +
> + sigset_to_compat(&set32, &child->blocked);
> +
> + if (copy_to_user(datap, &set32, sizeof(set32)))
> + return -EFAULT;
> +
> + ret = 0;
> + break;
> + case PTRACE_SETSIGMASK:
> + if (addr != sizeof(compat_sigset_t))
> + return -EINVAL;
> +
> + if (copy_from_user(&set32, datap, sizeof(compat_sigset_t)))
> + return -EFAULT;
> +
> + sigset_from_compat(&new_set, &set32);
> + ret = ptrace_setsigmask(child, &new_set);
> + break;
> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> case PTRACE_GETREGSET:
> case PTRACE_SETREGSET:
> --
> 2.11.0
>
On Thu, Jun 29, 2017 at 05:26:37PM +0100, James Morse wrote:
> compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
> instead using those in ptrace_request(). The compat variant should
> read a compat_sigset_t from userspace instead of ptrace_request()s
> sigset_t.
>
> While compat_sigset_t is the same size as sigset_t, it is defined as
> 2xu32, instead of a single u64. On a big-endian CPU this means that
> compat_sigset_t is passed to user-space using middle-endianness,
> where the least-significant u32 is written most significant byte
> first.
>
> If ptrace_request()s code is used userspace will read the most
> significant u32 where it expected the least significant.
>
> Instead of duplicating ptrace_request()s code as a special case in
> the arch code, handle it here.
Hi James,
I tested arm64/ilp32 on top of, and everything is fine.
Yury
Acked-by: Yury Norov <[email protected]>
> CC: Yury Norov <[email protected]>
> CC: Andrey Vagin <[email protected]>
> Reported-by: Zhou Chengming <[email protected]>
> Signed-off-by: James Morse <[email protected]>
> Fixes: 29000caecbe87 ("ptrace: add ability to get/set signal-blocked mask")
> ---
> LTP test case here:
> https://lists.linux.it/pipermail/ltp/2017-June/004932.html
>
> kernel/ptrace.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
> 1 file changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 8d2c10714530..a5bebb6713e8 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -843,6 +843,22 @@ static int ptrace_regset(struct task_struct *task, int req, unsigned int type,
> EXPORT_SYMBOL_GPL(task_user_regset_view);
> #endif
>
> +static int ptrace_setsigmask(struct task_struct *child, sigset_t *new_set)
> +{
> + 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;
> +}
> +
> int ptrace_request(struct task_struct *child, long request,
> unsigned long addr, unsigned long data)
> {
> @@ -914,18 +930,7 @@ int ptrace_request(struct task_struct *child, long request,
> break;
> }
>
> - 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);
> -
> - ret = 0;
> + ret = ptrace_setsigmask(child, &new_set);
> break;
> }
>
> @@ -1149,7 +1154,9 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
> compat_ulong_t addr, compat_ulong_t data)
> {
> compat_ulong_t __user *datap = compat_ptr(data);
> + compat_sigset_t set32;
> compat_ulong_t word;
> + sigset_t new_set;
> siginfo_t siginfo;
> int ret;
>
> @@ -1189,6 +1196,27 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
> else
> ret = ptrace_setsiginfo(child, &siginfo);
> break;
> + case PTRACE_GETSIGMASK:
> + if (addr != sizeof(compat_sigset_t))
> + return -EINVAL;
> +
> + sigset_to_compat(&set32, &child->blocked);
> +
> + if (copy_to_user(datap, &set32, sizeof(set32)))
> + return -EFAULT;
> +
> + ret = 0;
> + break;
> + case PTRACE_SETSIGMASK:
> + if (addr != sizeof(compat_sigset_t))
> + return -EINVAL;
> +
> + if (copy_from_user(&set32, datap, sizeof(compat_sigset_t)))
> + return -EFAULT;
> +
> + sigset_from_compat(&new_set, &set32);
> + ret = ptrace_setsigmask(child, &new_set);
> + break;
> #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
> case PTRACE_GETREGSET:
> case PTRACE_SETREGSET:
> --
> 2.11.0
On 06/29, James Morse wrote:
>
> compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
> instead using those in ptrace_request(). The compat variant should
> read a compat_sigset_t from userspace instead of ptrace_request()s
> sigset_t.
Acked-by: Oleg Nesterov <[email protected]>
James Morse <[email protected]> writes:
> compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
> instead using those in ptrace_request(). The compat variant should
> read a compat_sigset_t from userspace instead of ptrace_request()s
> sigset_t.
>
> While compat_sigset_t is the same size as sigset_t, it is defined as
> 2xu32, instead of a single u64. On a big-endian CPU this means that
> compat_sigset_t is passed to user-space using middle-endianness,
> where the least-significant u32 is written most significant byte
> first.
>
> If ptrace_request()s code is used userspace will read the most
> significant u32 where it expected the least significant.
But that's what the code has done since 2013.
So won't changing this break userspace that has been written to work
around that bug? Or do we think nothing actually uses it in the wild and
we can get away with it?
cheers
Hi Michael,
On 17/07/17 11:17, Michael Ellerman wrote:
> James Morse <[email protected]> writes:
>> compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
>> instead using those in ptrace_request(). The compat variant should
>> read a compat_sigset_t from userspace instead of ptrace_request()s
>> sigset_t.
>>
>> While compat_sigset_t is the same size as sigset_t, it is defined as
>> 2xu32, instead of a single u64. On a big-endian CPU this means that
>> compat_sigset_t is passed to user-space using middle-endianness,
>> where the least-significant u32 is written most significant byte
>> first.
>>
>> If ptrace_request()s code is used userspace will read the most
>> significant u32 where it expected the least significant.
>
> But that's what the code has done since 2013.
> So won't changing this break userspace that has been written to work
> around that bug?
Wouldn't the same program then be broken when run natively instead? To work
around it userspace would have to know it was running under compat instead of
natively.
This only affects this exotic ptrace API for big-endian compat users. I think
there are so few users that no-one has noticed its broken.
I'm only aware of CRIU using this[0], and it doesn't look like powerpc has to
support compat-criu users:
https://github.com/xemul/criu/tree/master/compel/arch
only has a ppc64 entry, for which
https://github.com/xemul/criu/blob/master/compel/arch/ppc64/plugins/include/asm/syscall-types.h
puts 'bits per word' as 64, I don't think it supports ppc32, which is where this
bug would be seen.
> Or do we think nothing actually uses it in the wild and
> we can get away with it?
I think only Zhou Chengming has hit this, and there is no 'in the wild' code
that actually inspects the buffer returned by the call.
Zhou, were you using criu on big-endian ilp32 when you found this? Or was it
some other project that uses this API..
(ilp32 is a second user of compat on arm64)
Thanks,
James
[0]
The commit message that added this code points to CRIU and GDB. GDB doesn't use
this API. Debian's codesearch (which obviously isn't exhaustive) only finds CRIU
and systemtap making these calls.
It looks like criu just save/restores the sigset_t as a blob:
https://sources.debian.net/src/criu/2.12.1-2/criu/parasite-syscall.c/?hl=92#L92
It's sigset_t helpers aren't aware of this bug:
https://github.com/xemul/criu/blob/master/compel/include/uapi/ksigset.h
Systemtap just makes some calls as part of a self test:
https://sources.debian.net/src/systemtap/3.1-2/testsuite/systemtap.syscall/ptrace.c/?hl=198#L198
James Morse <[email protected]> writes:
> Hi Michael,
>
> On 17/07/17 11:17, Michael Ellerman wrote:
>> James Morse <[email protected]> writes:
>>> compat_ptrace_request() lacks handlers for PTRACE_{G,S}ETSIGMASK,
>>> instead using those in ptrace_request(). The compat variant should
>>> read a compat_sigset_t from userspace instead of ptrace_request()s
>>> sigset_t.
>>>
>>> While compat_sigset_t is the same size as sigset_t, it is defined as
>>> 2xu32, instead of a single u64. On a big-endian CPU this means that
>>> compat_sigset_t is passed to user-space using middle-endianness,
>>> where the least-significant u32 is written most significant byte
>>> first.
>>>
>>> If ptrace_request()s code is used userspace will read the most
>>> significant u32 where it expected the least significant.
>>
>> But that's what the code has done since 2013.
>
>> So won't changing this break userspace that has been written to work
>> around that bug?
>
> Wouldn't the same program then be broken when run natively instead? To work
> around it userspace would have to know it was running under compat instead of
> natively.
True, it would be a mess to make it work in all cases. But that doesn't
mean someone hasn't done it :)
> This only affects this exotic ptrace API for big-endian compat users. I think
> there are so few users that no-one has noticed its broken.
>
> I'm only aware of CRIU using this[0], and it doesn't look like powerpc has to
> support compat-criu users:
> https://github.com/xemul/criu/tree/master/compel/arch
> only has a ppc64 entry, for which
> https://github.com/xemul/criu/blob/master/compel/arch/ppc64/plugins/include/asm/syscall-types.h
> puts 'bits per word' as 64, I don't think it supports ppc32, which is where this
> bug would be seen.
>
>
>> Or do we think nothing actually uses it in the wild and
>> we can get away with it?
>
> I think only Zhou Chengming has hit this, and there is no 'in the wild' code
> that actually inspects the buffer returned by the call.
>
> Zhou, were you using criu on big-endian ilp32 when you found this? Or was it
> some other project that uses this API..
> (ilp32 is a second user of compat on arm64)
...
>
> [0]
> The commit message that added this code points to CRIU and GDB. GDB doesn't use
> this API. Debian's codesearch (which obviously isn't exhaustive) only finds CRIU
> and systemtap making these calls.
>
> It looks like criu just save/restores the sigset_t as a blob:
> https://sources.debian.net/src/criu/2.12.1-2/criu/parasite-syscall.c/?hl=92#L92
>
> It's sigset_t helpers aren't aware of this bug:
> https://github.com/xemul/criu/blob/master/compel/include/uapi/ksigset.h
>
> Systemtap just makes some calls as part of a self test:
> https://sources.debian.net/src/systemtap/3.1-2/testsuite/systemtap.syscall/ptrace.c/?hl=198#L198
OK, that's pretty comprehensive.
You should just mention in the changelog that yes this breaks ABI but we
don't believe there are any users that will be affected, and the broken
behaviour is not easy to workaround in userspace.
cheers