[PATCH 0/2] Fixes and cleanups in PTRACE_SETOPTIONS

[PATCH 0/2] Fixes and cleanups in PTRACE_SETOPTIONS

[Denys Vlasenko]

This patchset has two changes.

First patch makes ptrace(PTRACE_SETOPTIONS, pid, 0, <bogus value>)
to error out *without* changing any task->ptrace bits.

Second patch is a cleanup: PTRACE_SETOPTIONS code
clears all option bits in task->ptrace, then sets bits one-by-one.
This is butt-ugly.

-- 
vda

[PATCH 1/2] ptrace: don't modify flags on PTRACE_SETOPTIONS failure

[Denys Vlasenko]

ptrace: don't modify flags on PTRACE_SETOPTIONS failure

On ptrace(PTRACE_SETOPTIONS, pid, 0, <opts>), we used to set
those option bits which are known, and then fail with -EINVAL
if there are some unknown bits in <opts>.

This in inconsistent with typical error handling, which
does not change any state if input is invalid.

This patch changes PTRACE_SETOPTIONS behavior so that
in this case, we return -EINVAL and don't change any bits
in task->ptrace.

It's very unlikely that there is userspace code in the wild which
will be affected by this change: it should have the form

    ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_BOGUSOPT)

where PTRACE_O_BOGUSOPT is a constant unknown to the kernel.
But kernel headers, naturally, don't contain any
PTRACE_O_BOGUSOPTs, thus the only way userspace can use one
if it defines one itself. I can't see why anyone would do such
a thing deliberately.

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 9de3ecf..02df3c1 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -509,6 +509,9 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds
 
 static int ptrace_setoptions(struct task_struct *child, unsigned long data)
 {
+	if (data & ~(unsigned long)PTRACE_O_MASK)
+		return -EINVAL;
+
 	child->ptrace &= ~PT_TRACE_MASK;
 
 	if (data & PTRACE_O_TRACESYSGOOD)
@@ -532,7 +535,7 @@ static int ptrace_setoptions(struct task_struct *child, unsigned long data)
 	if (data & PTRACE_O_TRACEEXIT)
 		child->ptrace |= PT_TRACE_EXIT;
 
-	return (data & ~PTRACE_O_MASK) ? -EINVAL : 0;
+	return 0;
 }
 
 static int ptrace_getsiginfo(struct task_struct *child, siginfo_t *info)

[PATCH 2/2] ptrace: simplify PTRACE_foo constants and PTRACE_SETOPTIONS code

[Denys Vlasenko]

ptrace: simplify PTRACE_foo constants and PTRACE_SETOPTIONS code

Exchange PT_TRACESYSGOOD and PT_PTRACE_CAP bit positions, which makes
PT_option bits contiguous and therefore makes code in ptrace_setoptions()
much simpler.

Every PTRACE_O_TRACEevent is defined to (1 << PTRACE_EVENT_event)
instead of using explicit numeric constants, to ensure we don't
mess up relationship between bit positions and event ids.

PT_EVENT_FLAG_SHIFT was not particularly useful, PT_OPT_FLAG_SHIFT with
value of PT_EVENT_FLAG_SHIFT-1 is easier to use.

PT_TRACE_MASK constant is nuked, the only its use is replaced by
(PTRACE_O_MASK << PT_OPT_FLAG_SHIFT).

Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>
---

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 800f113..0911100 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -54,17 +54,6 @@
 /* flags in @data for PTRACE_SEIZE */
 #define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */
 
-/* options set using PTRACE_SETOPTIONS */
-#define PTRACE_O_TRACESYSGOOD	0x00000001
-#define PTRACE_O_TRACEFORK	0x00000002
-#define PTRACE_O_TRACEVFORK	0x00000004
-#define PTRACE_O_TRACECLONE	0x00000008
-#define PTRACE_O_TRACEEXEC	0x00000010
-#define PTRACE_O_TRACEVFORKDONE	0x00000020
-#define PTRACE_O_TRACEEXIT	0x00000040
-
-#define PTRACE_O_MASK		0x0000007f
-
 /* Wait extended result codes for the above trace options.  */
 #define PTRACE_EVENT_FORK	1
 #define PTRACE_EVENT_VFORK	2
@@ -74,6 +63,17 @@
 #define PTRACE_EVENT_EXIT	6
 #define PTRACE_EVENT_STOP	7
 
+/* options set using PTRACE_SETOPTIONS */
+#define PTRACE_O_TRACESYSGOOD	1
+#define PTRACE_O_TRACEFORK	(1 << PTRACE_EVENT_FORK)
+#define PTRACE_O_TRACEVFORK	(1 << PTRACE_EVENT_VFORK)
+#define PTRACE_O_TRACECLONE	(1 << PTRACE_EVENT_CLONE)
+#define PTRACE_O_TRACEEXEC	(1 << PTRACE_EVENT_EXEC)
+#define PTRACE_O_TRACEVFORKDONE	(1 << PTRACE_EVENT_VFORK_DONE)
+#define PTRACE_O_TRACEEXIT	(1 << PTRACE_EVENT_EXIT)
+
+#define PTRACE_O_MASK		0x0000007f
+
 #include <asm/ptrace.h>
 
 #ifdef __KERNEL__
@@ -88,13 +88,12 @@
 #define PT_SEIZED	0x00010000	/* SEIZE used, enable new behavior */
 #define PT_PTRACED	0x00000001
 #define PT_DTRACE	0x00000002	/* delayed trace (used on m68k, i386) */
-#define PT_TRACESYSGOOD	0x00000004
-#define PT_PTRACE_CAP	0x00000008	/* ptracer can follow suid-exec */
+#define PT_PTRACE_CAP	0x00000004	/* ptracer can follow suid-exec */
 
+#define PT_OPT_FLAG_SHIFT	3
 /* PT_TRACE_* event enable flags */
-#define PT_EVENT_FLAG_SHIFT	4
-#define PT_EVENT_FLAG(event)	(1 << (PT_EVENT_FLAG_SHIFT + (event) - 1))
-
+#define PT_EVENT_FLAG(event)	(1 << (PT_OPT_FLAG_SHIFT + (event)))
+#define PT_TRACESYSGOOD		PT_EVENT_FLAG(0)
 #define PT_TRACE_FORK		PT_EVENT_FLAG(PTRACE_EVENT_FORK)
 #define PT_TRACE_VFORK		PT_EVENT_FLAG(PTRACE_EVENT_VFORK)
 #define PT_TRACE_CLONE		PT_EVENT_FLAG(PTRACE_EVENT_CLONE)
@@ -102,8 +101,6 @@
 #define PT_TRACE_VFORK_DONE	PT_EVENT_FLAG(PTRACE_EVENT_VFORK_DONE)
 #define PT_TRACE_EXIT		PT_EVENT_FLAG(PTRACE_EVENT_EXIT)
 
-#define PT_TRACE_MASK	0x000003f4
-
 /* single stepping state bits (used on ARM and PA-RISC) */
 #define PT_SINGLESTEP_BIT	31
 #define PT_SINGLESTEP		(1<<PT_SINGLESTEP_BIT)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 02df3c1..6664923 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -243,7 +243,7 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * Protect exec's credential calculations against our interference;
-	 * interference; SUID, SGID and LSM creds get determined differently
+	 * SUID, SGID and LSM creds get determined differently
 	 * under ptrace.
 	 */
 	retval = -ERESTARTNOINTR;
@@ -509,31 +509,16 @@ int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned long ds
 
 static int ptrace_setoptions(struct task_struct *child, unsigned long data)
 {
+	unsigned flags;
+
 	if (data & ~(unsigned long)PTRACE_O_MASK)
 		return -EINVAL;
 
-	child->ptrace &= ~PT_TRACE_MASK;
-
-	if (data & PTRACE_O_TRACESYSGOOD)
-		child->ptrace |= PT_TRACESYSGOOD;
-
-	if (data & PTRACE_O_TRACEFORK)
-		child->ptrace |= PT_TRACE_FORK;
-
-	if (data & PTRACE_O_TRACEVFORK)
-		child->ptrace |= PT_TRACE_VFORK;
-
-	if (data & PTRACE_O_TRACECLONE)
-		child->ptrace |= PT_TRACE_CLONE;
-
-	if (data & PTRACE_O_TRACEEXEC)
-		child->ptrace |= PT_TRACE_EXEC;
-
-	if (data & PTRACE_O_TRACEVFORKDONE)
-		child->ptrace |= PT_TRACE_VFORK_DONE;
-
-	if (data & PTRACE_O_TRACEEXIT)
-		child->ptrace |= PT_TRACE_EXIT;
+	/* Avoid intermediate state when all opts are cleared */
+	flags = child->ptrace;
+	flags &= ~(PTRACE_O_MASK << PT_OPT_FLAG_SHIFT);
+	flags |= (data << PT_OPT_FLAG_SHIFT);
+	child->ptrace = flags;
 
 	return 0;
 }

Re: [PATCH 1/2] ptrace: don't modify flags on PTRACE_SETOPTIONS failure

[Indan Zupancic]

Hello,

On Fri, September 9, 2011 08:24, Denys Vlasenko wrote:
> It's very unlikely that there is userspace code in the wild which
> will be affected by this change: it should have the form
>
>     ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_BOGUSOPT)
>
> where PTRACE_O_BOGUSOPT is a constant unknown to the kernel.
> But kernel headers, naturally, don't contain any
> PTRACE_O_BOGUSOPTs, thus the only way userspace can use one
> if it defines one itself. I can't see why anyone would do such
> a thing deliberately.

The only realistic case is when a program compiled on a newer
kernel is run on an older kernel, when it does things like

#ifndef PTRACE_O_TRACEFORK
	opts |= PTRACE_O_TRACEFORK;
#endif

and happened to work on older kernels because it didn't check
the return value.

With the current options the program has to change its behaviour
depending whether the options are available or not, so no problems
should happen in practice, because if a program counts on any
options that didn't exist before, it won't work anyway.

Code that cares about backward compatibility will check at runtime
if the PTRACE_SETOPTIONS fails, and do a new one without the unknown
options.

So it's best to fix this behaviour before introducing new options
that are harmless if the setting of them fails.

Greetings,

Indan

Re: [PATCH 1/2] ptrace: don't modify flags on PTRACE_SETOPTIONS failure

[Denys Vlasenko]

On Fri, 2011-09-09 at 14:44 +0200, Indan Zupancic wrote:
> Hello,
> 
> On Fri, September 9, 2011 08:24, Denys Vlasenko wrote:
> > It's very unlikely that there is userspace code in the wild which
> > will be affected by this change: it should have the form
> >
> >     ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_BOGUSOPT)
> >
> > where PTRACE_O_BOGUSOPT is a constant unknown to the kernel.
> > But kernel headers, naturally, don't contain any
> > PTRACE_O_BOGUSOPTs, thus the only way userspace can use one
> > if it defines one itself. I can't see why anyone would do such
> > a thing deliberately.
> 
> The only realistic case is when a program compiled on a newer
> kernel is run on an older kernel, when it does things like
> 
> #ifndef PTRACE_O_TRACEFORK
> 	opts |= PTRACE_O_TRACEFORK;
> #endif
> 
> and happened to work on older kernels because it didn't check
> the return value.

Well, older kernels, of course, will have *old* behavior
of SETOPTIONS too! My patch will not magically propagate
back in time and change behavior of old kernels :)

-- 
vda

Re: [PATCH 0/2] Fixes and cleanups in PTRACE_SETOPTIONS

[Oleg Nesterov]

On 09/09, Denys Vlasenko wrote:
>
> This patchset has two changes.
>
> First patch makes ptrace(PTRACE_SETOPTIONS, pid, 0, <bogus value>)
> to error out *without* changing any task->ptrace bits.
>
> Second patch is a cleanup: PTRACE_SETOPTIONS code
> clears all option bits in task->ptrace, then sets bits one-by-one.
> This is butt-ugly.

OK, thanks.

I'll wait a bit to collect the possible acks, and apply.

Oleg.

Re: [PATCH 1/2] ptrace: don't modify flags on PTRACE_SETOPTIONS failure

[Indan Zupancic]

On Fri, September 9, 2011 14:58, Denys Vlasenko wrote:
> On Fri, 2011-09-09 at 14:44 +0200, Indan Zupancic wrote:
>> Hello,
>>
>> On Fri, September 9, 2011 08:24, Denys Vlasenko wrote:
>> > It's very unlikely that there is userspace code in the wild which
>> > will be affected by this change: it should have the form
>> >
>> >     ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_BOGUSOPT)
>> >
>> > where PTRACE_O_BOGUSOPT is a constant unknown to the kernel.
>> > But kernel headers, naturally, don't contain any
>> > PTRACE_O_BOGUSOPTs, thus the only way userspace can use one
>> > if it defines one itself. I can't see why anyone would do such
>> > a thing deliberately.
>>
>> The only realistic case is when a program compiled on a newer
>> kernel is run on an older kernel, when it does things like
>>
>> #ifndef PTRACE_O_TRACEFORK
>> 	opts |= PTRACE_O_TRACEFORK;
>> #endif
>>
>> and happened to work on older kernels because it didn't check
>> the return value.
>
> Well, older kernels, of course, will have *old* behavior
> of SETOPTIONS too! My patch will not magically propagate
> back in time and change behavior of old kernels :)

In that case your patch should be allowed to magically propagate
forward in time and change the behavior of future kernels.

Greetings,

Indan

Re: [PATCH 1/2] ptrace: don't modify flags on PTRACE_SETOPTIONS failure

[Tejun Heo]

On Fri, Sep 09, 2011 at 08:24:37AM +0200, Denys Vlasenko wrote:
> ptrace: don't modify flags on PTRACE_SETOPTIONS failure
> 
> On ptrace(PTRACE_SETOPTIONS, pid, 0, <opts>), we used to set
> those option bits which are known, and then fail with -EINVAL
> if there are some unknown bits in <opts>.
> 
> This in inconsistent with typical error handling, which
> does not change any state if input is invalid.
> 
> This patch changes PTRACE_SETOPTIONS behavior so that
> in this case, we return -EINVAL and don't change any bits
> in task->ptrace.
> 
> It's very unlikely that there is userspace code in the wild which
> will be affected by this change: it should have the form
> 
>     ptrace(PTRACE_SETOPTIONS, pid, 0, PTRACE_O_BOGUSOPT)
> 
> where PTRACE_O_BOGUSOPT is a constant unknown to the kernel.
> But kernel headers, naturally, don't contain any
> PTRACE_O_BOGUSOPTs, thus the only way userspace can use one
> if it defines one itself. I can't see why anyone would do such
> a thing deliberately.
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun

Re: [PATCH 2/2] ptrace: simplify PTRACE_foo constants and PTRACE_SETOPTIONS code

[Tejun Heo]

On Fri, Sep 09, 2011 at 08:29:34AM +0200, Denys Vlasenko wrote:
> ptrace: simplify PTRACE_foo constants and PTRACE_SETOPTIONS code
> 
> Exchange PT_TRACESYSGOOD and PT_PTRACE_CAP bit positions, which makes
> PT_option bits contiguous and therefore makes code in ptrace_setoptions()
> much simpler.
> 
> Every PTRACE_O_TRACEevent is defined to (1 << PTRACE_EVENT_event)
> instead of using explicit numeric constants, to ensure we don't
> mess up relationship between bit positions and event ids.
> 
> PT_EVENT_FLAG_SHIFT was not particularly useful, PT_OPT_FLAG_SHIFT with
> value of PT_EVENT_FLAG_SHIFT-1 is easier to use.
> 
> PT_TRACE_MASK constant is nuked, the only its use is replaced by
> (PTRACE_O_MASK << PT_OPT_FLAG_SHIFT).
> 
> Signed-off-by: Denys Vlasenko <dvlasenk@redhat.com>

Acked-by: Tejun Heo <tj@kernel.org>

Thanks.

-- 
tejun