Fixes and cleanups in PTRACE_SETOPTIONS

PATCH In review Denys Vlasenko 5 participants 9 messages Sep 9, 2011 – Sep 10, 2011 ptrace support

lore

Share & tools

lore

Denys Vlasenko

author

2011-09-09 06:22 UTC #0 lore raw

cover Fixes and cleanups in PTRACE_SETOPTIONS

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.

Signature · 1 line

vda

Denys Vlasenko

author

2011-09-09 06:24 UTC #1 lore raw

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

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 <[email protected]>

---

kernel/ptrace.c +4 -1

index 9de3ecf..02df3c1 100644
@@ -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)

Denys Vlasenko

author

2011-09-09 06:29 UTC #2 lore raw

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

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 <[email protected]>

---

include/linux/ptrace.h +15 ~~-18~~

index 800f113..0911100 100644
@@ -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)

kernel/ptrace.c +8 ~~-23~~

index 02df3c1..6664923 100644
@@ -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;
 }

Indan Zupancic

2011-09-09 12:45 UTC #4 lore raw

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

In reply to Denys Vlasenko

Hello,

Quoted 10 lines · Denys Vlasenko

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

Denys Vlasenko

2011-09-09 12:58 UTC #5 lore raw

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

Quoted 17 lines · Indan Zupancic

Hello,
Quoted 10 lines · Denys Vlasenko
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

Quoted 6 lines

#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 :)

Signature · 1 line

vda

Oleg Nesterov

maintainer

2011-09-09 15:31 UTC #8 lore raw

In reply to Denys Vlasenko

Quoted 9 lines · 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.

OK, thanks.

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

Oleg.

Indan Zupancic

2011-09-09 16:22 UTC #6 lore raw

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

In reply to Denys Vlasenko

Quoted 22 lines · Denys Vlasenko

Quoted 21 lines · Indan Zupancic
Hello,
Quoted 10 lines · Denys Vlasenko
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

Quoted 6 lines

Quoted 2 lines
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

Tejun Heo

Acked-by on v1 1/2

2011-09-10 23:32 UTC #7 lore raw

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

In reply to Denys Vlasenko

Quoted 18 lines · 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)

Quoted 7 lines

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 <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun

Tejun Heo

Acked-by on v1 2/2

2011-09-10 23:32 UTC #3 lore raw

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

In reply to Denys Vlasenko

Quoted 10 lines · 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.

Quoted 7 lines

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 <[email protected]>

Acked-by Tejun Heo <[email protected]>

Thanks.

Signature · 1 line

tejun