[PATCH] ptrace: Add compat PTRACE_{G,S}ETSIGMASK handlers

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 <redacted>
CC: Andrey Vagin <redacted>
Reported-by: Zhou Chengming <redacted>
Signed-off-by: James Morse <james.morse@arm.com>
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 <redacted>

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 <redacted>

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 <oleg@redhat.com>

James Morse [off-list ref] 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 [off-list ref] writes:

quoted

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 [off-list ref] writes:

Hi Michael,

On 17/07/17 11:17, Michael Ellerman wrote:

quoted

James Morse [off-list ref] writes:

quoted

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.

quoted

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.

quoted

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

Hi James, all,

(add linux-api@vger.kernel.org as it is user-visible,
Catalin Marinas and Arnd Bergmann [off-list ref])

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.

CC: Yury Norov <redacted>
CC: Andrey Vagin <redacted>
Reported-by: Zhou Chengming <redacted>
Signed-off-by: James Morse <james.morse@arm.com>
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

This patch relies on sigset_{to,from}_compat() which was proposed to
remove from the kernel recently. The change is in linux-next, and it
breaks the build of the kenel with this patch. Below the updated
version.

I'd like to ask here again, do we need this change? The patch is
correct, but it changes the ptrace API for compat big-endian
architectures. It normally should stop us from pulling it, but there's
seemingly no users of the API in the wild, and so it will
break nothing.

The problem was originally reported by Zhou Chengming for BE arm64/ilp32.
I would like to see arm64/ilp32 working correct in this case, and
developers of other new architectures probably would so.

Regarding arm64/ilp32, we have agreed ABI, and 4.12 and 4.13 kernels
have this change:
https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git/log/?h=staging/ilp32-4.12
https://github.com/norov/linux/tree/ilp32-4.13

So I see 3 ways to proceed with this:
1. Drop the patch and remove it from arm64/ilp32;
2. Apply the patch as is;
3. Introduce new config option like ARCH_PTRACE_COMPAT_BE_SWAP_SIGMASK,
   make it enabled by default and disable explicitly for existing
   compat BE architectures.

I would choose 2 or 3 depending on what maintainers of existing
architectures think.

Yury

Signed-off-by: Yury Norov <redacted>
---
 kernel/ptrace.c | 52 ++++++++++++++++++++++++++++++++++++++++------------
 1 file changed, 40 insertions(+), 12 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 84b1367935e4..1af47a33768e 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c

@@ -880,6 +880,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)
 {

@@ -951,18 +967,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;
 	}
 

@@ -1192,6 +1197,7 @@ int compat_ptrace_request(struct task_struct *child, compat_long_t request,
 {
 	compat_ulong_t __user *datap = compat_ptr(data);
 	compat_ulong_t word;
+	sigset_t new_set;
 	siginfo_t siginfo;
 	int ret;
 

@@ -1233,6 +1239,28 @@ 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;
+
+		ret = put_compat_sigset((compat_sigset_t __user *) datap,
+				&child->blocked, sizeof(compat_sigset_t));
+		if (ret)
+			return ret;
+
+		ret = 0;
+		break;
+	case PTRACE_SETSIGMASK:
+		if (addr != sizeof(compat_sigset_t))
+			return -EINVAL;
+
+		ret = get_compat_sigset(&new_set,
+				(compat_sigset_t __user *) datap);
+		if (ret)
+			break;
+
+		ret = ptrace_setsigmask(child, &new_set);
+		break;
 #ifdef CONFIG_HAVE_ARCH_TRACEHOOK
 	case PTRACE_GETREGSET:
 	case PTRACE_SETREGSET:

-- 
2.11.0