[PATCH 0/5] ptrace tweaks

[PATCH 0/5] ptrace tweaks

[Denys Vlasenko]

This patch set fixes a minor bug in PTRACE_SETOPTIONS,
slightly extends PTRACE_SEIZE (now it can take ptrace options),
changes PTRACE_EVENT_STOP value,
and enables PTRACE_SEIZE for non-development use.

Ptrace git tree on kernel.org is not restored yet,
so this patch set can't go through it.

Andrew, please consider taking this patch set.

Denys Vlasenko (5):
  ptrace: don't modify flags on PTRACE_SETOPTIONS failure
  ptrace: simplify PTRACE_foo constants and PTRACE_SETOPTIONS code
  ptrace: make PTRACE_SEIZE set ptrace options specified in 'data' parameter
  ptrace: renumber PTRACE_EVENT_STOP so that future new options and
    events can match
  ptrace: remove PTRACE_SEIZE_DEVEL bit

 include/linux/ptrace.h |   39 ++++++++++++----------------
 kernel/ptrace.c        |   64 ++++++++++++++++++------------------------------
 2 files changed, 41 insertions(+), 62 deletions(-)

-- 
1.7.7.6

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

[Denys Vlasenko]

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 <vda.linux@googlemail.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 kernel/ptrace.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 00ab2ca..273f56e 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -528,6 +528,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)
@@ -551,7 +554,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)
-- 
1.7.7.6

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

[Denys Vlasenko]

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 <vda.linux@googlemail.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 include/linux/ptrace.h |   33 +++++++++++++++------------------
 kernel/ptrace.c        |   31 ++++++++-----------------------
 2 files changed, 23 insertions(+), 41 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index c2f1f6a..06be6a3 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 273f56e..9acd07a 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -262,7 +262,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;
@@ -528,31 +528,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;
 }
-- 
1.7.7.6

[PATCH 3/5] ptrace: make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Denys Vlasenko]

This can be used to close a few corner cases in strace where we get
unwanted racy behavior after attach, but before we have a chance
to set options (the notorious post-execve SIGTRAP comes to mind),
and removes the need to track "did we set opts for this task" state
in strace internals.

While we are at it:

Make it possible to extend SEIZE in the future with more functionality
by passing non-zero 'addr' parameter.
To that end, error out if 'addr' is non-zero.
PTRACE_ATTACH did not (and still does not) have such check,
and users (strace) do pass garbage there... let's avoid repeating
this mistake with SEIZE.

Set all task->ptrace bits in one operation - before this change,
we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT ops.
This was probably ok (not a bug), but let's be on a safer side.

Changes since v2: use (unsigned long) casts instead of (long) ones,
move PTRACE_SEIZE_DEVEL-related code to separate lines of code.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 kernel/ptrace.c |   29 ++++++++++++++++++++---------
 1 files changed, 20 insertions(+), 9 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 9acd07a..99a18a0 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -231,6 +231,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
 }
 
 static int ptrace_attach(struct task_struct *task, long request,
+			 unsigned long addr,
 			 unsigned long flags)
 {
 	bool seize = (request == PTRACE_SEIZE);
@@ -238,19 +239,29 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * SEIZE will enable new ptrace behaviors which will be implemented
-	 * gradually.  SEIZE_DEVEL is used to prevent applications
+	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
 	 * expecting full SEIZE behaviors trapping on kernel commits which
 	 * are still in the process of implementing them.
 	 *
 	 * Only test programs for new ptrace behaviors being implemented
 	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
 	 *
-	 * Once SEIZE behaviors are completely implemented, this flag and
-	 * the following test will be removed.
+	 * Once SEIZE behaviors are completely implemented, this flag
+	 * will be removed.
 	 */
 	retval = -EIO;
-	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
-		goto out;
+	if (seize) {
+		if (addr != 0)
+			goto out;
+		if (!(flags & PTRACE_SEIZE_DEVEL))
+			goto out;
+		flags &= ~(unsigned long)PTRACE_SEIZE_DEVEL;
+		if (flags & ~(unsigned long)PTRACE_O_MASK)
+			goto out;
+		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
+	} else {
+		flags = PT_PTRACED;
+	}
 
 	audit_ptrace(task);
 
@@ -282,11 +293,11 @@ static int ptrace_attach(struct task_struct *task, long request,
 	if (task->ptrace)
 		goto unlock_tasklist;
 
-	task->ptrace = PT_PTRACED;
 	if (seize)
-		task->ptrace |= PT_SEIZED;
+		flags |= PT_SEIZED;
 	if (ns_capable(task_user_ns(task), CAP_SYS_PTRACE))
-		task->ptrace |= PT_PTRACE_CAP;
+		flags |= PT_PTRACE_CAP;
+	task->ptrace = flags;
 
 	__ptrace_link(task, current);
 
@@ -879,7 +890,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 	}
 
 	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
-		ret = ptrace_attach(child, request, data);
+		ret = ptrace_attach(child, request, addr, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.
-- 
1.7.7.6

[PATCH 4/5] ptrace: renumber PTRACE_EVENT_STOP so that future new options and events can match

[Denys Vlasenko]

PTRACE_EVENT_foo and PTRACE_O_TRACEfoo used to match.

New PTRACE_EVENT_STOP is the first event which has no corresponding
PTRACE_O_TRACE option. If we will ever want to add another such option,
its PTRACE_EVENT's value will collide with PTRACE_EVENT_STOP's value.

This patch changes PTRACE_EVENT_STOP value to prevent this.

While at it, added a comment - the one atop PTRACE_EVENT block,
saying "Wait extended result codes for the above trace options",
is not true for PTRACE_EVENT_STOP.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 include/linux/ptrace.h |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 06be6a3..ec6571c 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -61,7 +61,8 @@
 #define PTRACE_EVENT_EXEC	4
 #define PTRACE_EVENT_VFORK_DONE	5
 #define PTRACE_EVENT_EXIT	6
-#define PTRACE_EVENT_STOP	7
+/* Extended result codes which enabled by means other than options.  */
+#define PTRACE_EVENT_STOP	128
 
 /* options set using PTRACE_SETOPTIONS */
 #define PTRACE_O_TRACESYSGOOD	1
-- 
1.7.7.6

[PATCH 5/5] ptrace: remove PTRACE_SEIZE_DEVEL bit

[Denys Vlasenko]

PTRACE_SEIZE code is tested and ready for production use,
remove the code which requires special bit in data argument
to make PTRACE_SEIZE work.

Strace team prepares for a new release of strace, and we
would like to ship the code which uses PTRACE_SEIZE,
preferably after this change goes into released kernel.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
---
 include/linux/ptrace.h |    5 +----
 kernel/ptrace.c        |   15 ---------------
 2 files changed, 1 insertions(+), 19 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index ec6571c..6dffe18 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -51,9 +51,6 @@
 #define PTRACE_INTERRUPT	0x4207
 #define PTRACE_LISTEN		0x4208
 
-/* flags in @data for PTRACE_SEIZE */
-#define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */
-
 /* Wait extended result codes for the above trace options.  */
 #define PTRACE_EVENT_FORK	1
 #define PTRACE_EVENT_VFORK	2
@@ -64,7 +61,7 @@
 /* Extended result codes which enabled by means other than options.  */
 #define PTRACE_EVENT_STOP	128
 
-/* options set using PTRACE_SETOPTIONS */
+/* Options set using PTRACE_SETOPTIONS or using PTRACE_SEIZE @data param */
 #define PTRACE_O_TRACESYSGOOD	1
 #define PTRACE_O_TRACEFORK	(1 << PTRACE_EVENT_FORK)
 #define PTRACE_O_TRACEVFORK	(1 << PTRACE_EVENT_VFORK)
diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 99a18a0..32846f7 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -237,25 +237,10 @@ static int ptrace_attach(struct task_struct *task, long request,
 	bool seize = (request == PTRACE_SEIZE);
 	int retval;
 
-	/*
-	 * SEIZE will enable new ptrace behaviors which will be implemented
-	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
-	 * expecting full SEIZE behaviors trapping on kernel commits which
-	 * are still in the process of implementing them.
-	 *
-	 * Only test programs for new ptrace behaviors being implemented
-	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
-	 *
-	 * Once SEIZE behaviors are completely implemented, this flag
-	 * will be removed.
-	 */
 	retval = -EIO;
 	if (seize) {
 		if (addr != 0)
 			goto out;
-		if (!(flags & PTRACE_SEIZE_DEVEL))
-			goto out;
-		flags &= ~(unsigned long)PTRACE_SEIZE_DEVEL;
 		if (flags & ~(unsigned long)PTRACE_O_MASK)
 			goto out;
 		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
-- 
1.7.7.6

Re: [PATCH 5/5] ptrace: remove PTRACE_SEIZE_DEVEL bit

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:
>
> PTRACE_SEIZE code is tested and ready for production use,
> remove the code which requires special bit in data argument
> to make PTRACE_SEIZE work.
>
> Strace team prepares for a new release of strace, and we
> would like to ship the code which uses PTRACE_SEIZE,
> preferably after this change goes into released kernel.
>
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

Personally I agree with this change. PTRACE_SEIZE_DEVEL complicates
the testing/development for the user-space.

Tejun, do you agree?

Acked-by: Oleg Nesterov <oleg@redhat.com>

-
>  include/linux/ptrace.h |    5 +----
>  kernel/ptrace.c        |   15 ---------------
>  2 files changed, 1 insertions(+), 19 deletions(-)
> 
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index ec6571c..6dffe18 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -51,9 +51,6 @@
>  #define PTRACE_INTERRUPT	0x4207
>  #define PTRACE_LISTEN		0x4208
>  
> -/* flags in @data for PTRACE_SEIZE */
> -#define PTRACE_SEIZE_DEVEL	0x80000000 /* temp flag for development */
> -
>  /* Wait extended result codes for the above trace options.  */
>  #define PTRACE_EVENT_FORK	1
>  #define PTRACE_EVENT_VFORK	2
> @@ -64,7 +61,7 @@
>  /* Extended result codes which enabled by means other than options.  */
>  #define PTRACE_EVENT_STOP	128
>  
> -/* options set using PTRACE_SETOPTIONS */
> +/* Options set using PTRACE_SETOPTIONS or using PTRACE_SEIZE @data param */
>  #define PTRACE_O_TRACESYSGOOD	1
>  #define PTRACE_O_TRACEFORK	(1 << PTRACE_EVENT_FORK)
>  #define PTRACE_O_TRACEVFORK	(1 << PTRACE_EVENT_VFORK)
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 99a18a0..32846f7 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -237,25 +237,10 @@ static int ptrace_attach(struct task_struct *task, long request,
>  	bool seize = (request == PTRACE_SEIZE);
>  	int retval;
>  
> -	/*
> -	 * SEIZE will enable new ptrace behaviors which will be implemented
> -	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
> -	 * expecting full SEIZE behaviors trapping on kernel commits which
> -	 * are still in the process of implementing them.
> -	 *
> -	 * Only test programs for new ptrace behaviors being implemented
> -	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
> -	 *
> -	 * Once SEIZE behaviors are completely implemented, this flag
> -	 * will be removed.
> -	 */
>  	retval = -EIO;
>  	if (seize) {
>  		if (addr != 0)
>  			goto out;
> -		if (!(flags & PTRACE_SEIZE_DEVEL))
> -			goto out;
> -		flags &= ~(unsigned long)PTRACE_SEIZE_DEVEL;
>  		if (flags & ~(unsigned long)PTRACE_O_MASK)
>  			goto out;
>  		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
> -- 
> 1.7.7.6
> 

Re: [PATCH 5/5] ptrace: remove PTRACE_SEIZE_DEVEL bit

[Pedro Alves]

Hi,

Do new auto-attached clones still generate SIGSTOP, or has than been fixed already?

-- 
Pedro Alves

Re: [PATCH 5/5] ptrace: remove PTRACE_SEIZE_DEVEL bit

[Oleg Nesterov]

On 02/10, Pedro Alves wrote:
>
> Do new auto-attached clones still generate SIGSTOP, or has than been fixed already?

Hopefully fixed by d184d6eb
"ptrace: dont send SIGSTOP on auto-attach if PT_SEIZED"

Oleg.

Re: [PATCH 5/5] ptrace: remove PTRACE_SEIZE_DEVEL bit

[Pedro Alves]

On 02/10/2012 05:42 PM, Oleg Nesterov wrote:
> On 02/10, Pedro Alves wrote:
>>
>> Do new auto-attached clones still generate SIGSTOP, or has than been fixed already?
> 
> Hopefully fixed by d184d6eb
> "ptrace: dont send SIGSTOP on auto-attach if PT_SEIZED"

Awesome, thanks.

-- 
Pedro Alves

Re: [PATCH 5/5] ptrace: remove PTRACE_SEIZE_DEVEL bit

[Tejun Heo]

On Fri, Feb 10, 2012 at 06:24:27PM +0100, Oleg Nesterov wrote:
> On 02/10, Denys Vlasenko wrote:
> >
> > PTRACE_SEIZE code is tested and ready for production use,
> > remove the code which requires special bit in data argument
> > to make PTRACE_SEIZE work.
> >
> > Strace team prepares for a new release of strace, and we
> > would like to ship the code which uses PTRACE_SEIZE,
> > preferably after this change goes into released kernel.
> >
> > Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> 
> Personally I agree with this change. PTRACE_SEIZE_DEVEL complicates
> the testing/development for the user-space.
> 
> Tejun, do you agree?
> 
> Acked-by: Oleg Nesterov <oleg@redhat.com>

Yeah, I was thinking about sending a patch to remove DEVEL flag myself
and other changes seem good to me too.  For the whole series,

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

Thanks!

-- 
tejun

Re: [PATCH 4/5] ptrace: renumber PTRACE_EVENT_STOP so that future new options and events can match

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:
>
> PTRACE_EVENT_foo and PTRACE_O_TRACEfoo used to match.
> 
> New PTRACE_EVENT_STOP is the first event which has no corresponding
> PTRACE_O_TRACE option. If we will ever want to add another such option,
> its PTRACE_EVENT's value will collide with PTRACE_EVENT_STOP's value.
> 
> This patch changes PTRACE_EVENT_STOP value to prevent this.
> 
> While at it, added a comment - the one atop PTRACE_EVENT block,
> saying "Wait extended result codes for the above trace options",
> is not true for PTRACE_EVENT_STOP.
> 
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>

IIRC Tejun agreed with this change too.

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  include/linux/ptrace.h |    3 ++-
>  1 files changed, 2 insertions(+), 1 deletions(-)
> 
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 06be6a3..ec6571c 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -61,7 +61,8 @@
>  #define PTRACE_EVENT_EXEC	4
>  #define PTRACE_EVENT_VFORK_DONE	5
>  #define PTRACE_EVENT_EXIT	6
> -#define PTRACE_EVENT_STOP	7
> +/* Extended result codes which enabled by means other than options.  */
> +#define PTRACE_EVENT_STOP	128
>  
>  /* options set using PTRACE_SETOPTIONS */
>  #define PTRACE_O_TRACESYSGOOD	1
> -- 
> 1.7.7.6
> 

Re: [PATCH 3/5] ptrace: make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:
>
> @@ -879,7 +890,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
>  	}
>
>  	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
> -		ret = ptrace_attach(child, request, data);
> +		ret = ptrace_attach(child, request, addr, data);

There is another caller which should be updated, sys_compat_ptrace().

Otherwise looks good.

Oleg.

Re: [PATCH 3/5] ptrace: make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Denys Vlasenko]

On Fri, Feb 10, 2012 at 4:57 PM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 02/10, Denys Vlasenko wrote:
>>
>> @@ -879,7 +890,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
>>       }
>>
>>       if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
>> -             ret = ptrace_attach(child, request, data);
>> +             ret = ptrace_attach(child, request, addr, data);
>
> There is another caller which should be updated, sys_compat_ptrace().

Drats. I only tested the patch on i386, not on x86-64, and missed it.
Updated patch follows in a minute.

-- 
vda

[PATCH v2 3/5] ptrace: make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Denys Vlasenko]

This can be used to close a few corner cases in strace where we get
unwanted racy behavior after attach, but before we have a chance
to set options (the notorious post-execve SIGTRAP comes to mind),
and removes the need to track "did we set opts for this task" state
in strace internals.

While we are at it:

Make it possible to extend SEIZE in the future with more functionality
by passing non-zero 'addr' parameter.
To that end, error out if 'addr' is non-zero.
PTRACE_ATTACH did not (and still does not) have such check,
and users (strace) do pass garbage there... let's avoid repeating
this mistake with SEIZE.

Set all task->ptrace bits in one operation - before this change,
we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT ops.
This was probably ok (not a bug), but let's be on a safer side.

Changes in v2: update ptrace_attach() call in compat_sys_ptrace() too.

Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
Acked-by: Tejun Heo <tj@kernel.org>
---
 kernel/ptrace.c |   31 +++++++++++++++++++++----------
 1 files changed, 21 insertions(+), 10 deletions(-)

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 9acd07a..4661c5b 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -231,6 +231,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
 }
 
 static int ptrace_attach(struct task_struct *task, long request,
+			 unsigned long addr,
 			 unsigned long flags)
 {
 	bool seize = (request == PTRACE_SEIZE);
@@ -238,19 +239,29 @@ static int ptrace_attach(struct task_struct *task, long request,
 
 	/*
 	 * SEIZE will enable new ptrace behaviors which will be implemented
-	 * gradually.  SEIZE_DEVEL is used to prevent applications
+	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
 	 * expecting full SEIZE behaviors trapping on kernel commits which
 	 * are still in the process of implementing them.
 	 *
 	 * Only test programs for new ptrace behaviors being implemented
 	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
 	 *
-	 * Once SEIZE behaviors are completely implemented, this flag and
-	 * the following test will be removed.
+	 * Once SEIZE behaviors are completely implemented, this flag
+	 * will be removed.
 	 */
 	retval = -EIO;
-	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
-		goto out;
+	if (seize) {
+		if (addr != 0)
+			goto out;
+		if (!(flags & PTRACE_SEIZE_DEVEL))
+			goto out;
+		flags &= ~(unsigned long)PTRACE_SEIZE_DEVEL;
+		if (flags & ~(unsigned long)PTRACE_O_MASK)
+			goto out;
+		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
+	} else {
+		flags = PT_PTRACED;
+	}
 
 	audit_ptrace(task);
 
@@ -282,11 +293,11 @@ static int ptrace_attach(struct task_struct *task, long request,
 	if (task->ptrace)
 		goto unlock_tasklist;
 
-	task->ptrace = PT_PTRACED;
 	if (seize)
-		task->ptrace |= PT_SEIZED;
+		flags |= PT_SEIZED;
 	if (ns_capable(task_user_ns(task), CAP_SYS_PTRACE))
-		task->ptrace |= PT_PTRACE_CAP;
+		flags |= PT_PTRACE_CAP;
+	task->ptrace = flags;
 
 	__ptrace_link(task, current);
 
@@ -879,7 +890,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
 	}
 
 	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
-		ret = ptrace_attach(child, request, data);
+		ret = ptrace_attach(child, request, addr, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.
@@ -1022,7 +1033,7 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
 	}
 
 	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
-		ret = ptrace_attach(child, request, data);
+		ret = ptrace_attach(child, request, addr, data);
 		/*
 		 * Some architectures need to do book-keeping after
 		 * a ptrace attach.
-- 
1.7.7.6

Re: [PATCH v2 3/5] ptrace: make PTRACE_SEIZE set ptrace options specified in 'data' parameter

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:
>
> This can be used to close a few corner cases in strace where we get
> unwanted racy behavior after attach, but before we have a chance
> to set options (the notorious post-execve SIGTRAP comes to mind),
> and removes the need to track "did we set opts for this task" state
> in strace internals.
> 
> While we are at it:
> 
> Make it possible to extend SEIZE in the future with more functionality
> by passing non-zero 'addr' parameter.
> To that end, error out if 'addr' is non-zero.
> PTRACE_ATTACH did not (and still does not) have such check,
> and users (strace) do pass garbage there... let's avoid repeating
> this mistake with SEIZE.
> 
> Set all task->ptrace bits in one operation - before this change,
> we were adding PT_SEIZED and PT_PTRACE_CAP with task->ptrace |= BIT ops.
> This was probably ok (not a bug), but let's be on a safer side.
> 
> Changes in v2: update ptrace_attach() call in compat_sys_ptrace() too.
> 
> Signed-off-by: Denys Vlasenko <vda.linux@googlemail.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  kernel/ptrace.c |   31 +++++++++++++++++++++----------
>  1 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 9acd07a..4661c5b 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -231,6 +231,7 @@ bool ptrace_may_access(struct task_struct *task, unsigned int mode)
>  }
>  
>  static int ptrace_attach(struct task_struct *task, long request,
> +			 unsigned long addr,
>  			 unsigned long flags)
>  {
>  	bool seize = (request == PTRACE_SEIZE);
> @@ -238,19 +239,29 @@ static int ptrace_attach(struct task_struct *task, long request,
>  
>  	/*
>  	 * SEIZE will enable new ptrace behaviors which will be implemented
> -	 * gradually.  SEIZE_DEVEL is used to prevent applications
> +	 * gradually.  SEIZE_DEVEL bit is used to prevent applications
>  	 * expecting full SEIZE behaviors trapping on kernel commits which
>  	 * are still in the process of implementing them.
>  	 *
>  	 * Only test programs for new ptrace behaviors being implemented
>  	 * should set SEIZE_DEVEL.  If unset, SEIZE will fail with -EIO.
>  	 *
> -	 * Once SEIZE behaviors are completely implemented, this flag and
> -	 * the following test will be removed.
> +	 * Once SEIZE behaviors are completely implemented, this flag
> +	 * will be removed.
>  	 */
>  	retval = -EIO;
> -	if (seize && !(flags & PTRACE_SEIZE_DEVEL))
> -		goto out;
> +	if (seize) {
> +		if (addr != 0)
> +			goto out;
> +		if (!(flags & PTRACE_SEIZE_DEVEL))
> +			goto out;
> +		flags &= ~(unsigned long)PTRACE_SEIZE_DEVEL;
> +		if (flags & ~(unsigned long)PTRACE_O_MASK)
> +			goto out;
> +		flags = PT_PTRACED | PT_SEIZED | (flags << PT_OPT_FLAG_SHIFT);
> +	} else {
> +		flags = PT_PTRACED;
> +	}
>  
>  	audit_ptrace(task);
>  
> @@ -282,11 +293,11 @@ static int ptrace_attach(struct task_struct *task, long request,
>  	if (task->ptrace)
>  		goto unlock_tasklist;
>  
> -	task->ptrace = PT_PTRACED;
>  	if (seize)
> -		task->ptrace |= PT_SEIZED;
> +		flags |= PT_SEIZED;
>  	if (ns_capable(task_user_ns(task), CAP_SYS_PTRACE))
> -		task->ptrace |= PT_PTRACE_CAP;
> +		flags |= PT_PTRACE_CAP;
> +	task->ptrace = flags;
>  
>  	__ptrace_link(task, current);
>  
> @@ -879,7 +890,7 @@ SYSCALL_DEFINE4(ptrace, long, request, long, pid, unsigned long, addr,
>  	}
>  
>  	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
> -		ret = ptrace_attach(child, request, data);
> +		ret = ptrace_attach(child, request, addr, data);
>  		/*
>  		 * Some architectures need to do book-keeping after
>  		 * a ptrace attach.
> @@ -1022,7 +1033,7 @@ asmlinkage long compat_sys_ptrace(compat_long_t request, compat_long_t pid,
>  	}
>  
>  	if (request == PTRACE_ATTACH || request == PTRACE_SEIZE) {
> -		ret = ptrace_attach(child, request, data);
> +		ret = ptrace_attach(child, request, addr, data);
>  		/*
>  		 * Some architectures need to do book-keeping after
>  		 * a ptrace attach.
> -- 
> 1.7.7.6
> 

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

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:
>
> 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 <vda.linux@googlemail.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  include/linux/ptrace.h |   33 +++++++++++++++------------------
>  kernel/ptrace.c        |   31 ++++++++-----------------------
>  2 files changed, 23 insertions(+), 41 deletions(-)
> 
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index c2f1f6a..06be6a3 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 273f56e..9acd07a 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -262,7 +262,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;
> @@ -528,31 +528,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;
>  }
> -- 
> 1.7.7.6
> 

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

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:

> 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 <vda.linux@googlemail.com>
> Acked-by: Tejun Heo <tj@kernel.org>

Reviewed-by: Oleg Nesterov <oleg@redhat.com>

> ---
>  kernel/ptrace.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/kernel/ptrace.c b/kernel/ptrace.c
> index 00ab2ca..273f56e 100644
> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -528,6 +528,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)
> @@ -551,7 +554,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)
> -- 
> 1.7.7.6
> 

[PATCH 0/2] more tweaks (Was: ptrace tweaks)

[Oleg Nesterov]

On 02/10, Denys Vlasenko wrote:
>
> This patch set fixes a minor bug in PTRACE_SETOPTIONS,
> slightly extends PTRACE_SEIZE (now it can take ptrace options),
> changes PTRACE_EVENT_STOP value,
> and enables PTRACE_SEIZE for non-development use.
>
> Ptrace git tree on kernel.org is not restored yet,
> so this patch set can't go through it.
>
> Andrew, please consider taking this patch set.

Yes, please...

I was going to send 1-4 to Linus a long ago, but I can't restore
my korg account.

Plus I have a couple of other minor ptrace changes, could you
take them too ?

Chris, iirc you suggested to add PTRACE_O_KILL_TRACEE_IF_DEBUGGER_DIES.
The necessary change is simple, please suggest the good name for the
new option ;)

Oleg.

[PATCH 1/2] ptrace: the killed tracee should not enter the syscall

[Oleg Nesterov]

Another old/known problem. If the tracee is killed after it reports
syscall_entry, it starts the syscall and debugger can't control this.
This confuses the users and this creates the security problems for
ptrace jailers.

Change tracehook_report_syscall_entry() to return non-zero if killed,
this instructs syscall_trace_enter() to abort the syscall.

Reported-by: Chris Evans <scarybeasts@gmail.com>
Tested-by: Indan Zupancic <indan@nul.nu>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/tracehook.h |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/include/linux/tracehook.h b/include/linux/tracehook.h
index a71a292..51bd91d 100644
--- a/include/linux/tracehook.h
+++ b/include/linux/tracehook.h
@@ -54,12 +54,12 @@ struct linux_binprm;
 /*
  * ptrace report for syscall entry and exit looks identical.
  */
-static inline void ptrace_report_syscall(struct pt_regs *regs)
+static inline int ptrace_report_syscall(struct pt_regs *regs)
 {
 	int ptrace = current->ptrace;
 
 	if (!(ptrace & PT_PTRACED))
-		return;
+		return 0;
 
 	ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0));
 
@@ -72,6 +72,8 @@ static inline void ptrace_report_syscall(struct pt_regs *regs)
 		send_sig(current->exit_code, current, 1);
 		current->exit_code = 0;
 	}
+
+	return fatal_signal_pending(current);
 }
 
 /**
@@ -96,8 +98,7 @@ static inline void ptrace_report_syscall(struct pt_regs *regs)
 static inline __must_check int tracehook_report_syscall_entry(
 	struct pt_regs *regs)
 {
-	ptrace_report_syscall(regs);
-	return 0;
+	return ptrace_report_syscall(regs);
 }
 
 /**
-- 
1.5.5.1

[PATCH 2/2] ptrace: don't send SIGTRAP on exec if SEIZED

[Oleg Nesterov]

ptrace_event(PTRACE_EVENT_EXEC) sends SIGTRAP if PT_TRACE_EXEC is
not set. This is because this SIGTRAP predates PTRACE_O_TRACEEXEC
option, we do not need/want this with PT_SEIZED which can set the
options during attach.

Suggested-by: Pedro Alves <palves@redhat.com>
Signed-off-by: Oleg Nesterov <oleg@redhat.com>
---
 include/linux/ptrace.h |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 6dffe18..407c678 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -194,9 +194,10 @@ static inline void ptrace_event(int event, unsigned long message)
 	if (unlikely(ptrace_event_enabled(current, event))) {
 		current->ptrace_message = message;
 		ptrace_notify((event << 8) | SIGTRAP);
-	} else if (event == PTRACE_EVENT_EXEC && unlikely(current->ptrace)) {
+	} else if (event == PTRACE_EVENT_EXEC) {
 		/* legacy EXEC report via SIGTRAP */
-		send_sig(SIGTRAP, current, 0);
+		if ((current->ptrace & (PT_PTRACED|PT_SEIZED)) == PT_PTRACED)
+			send_sig(SIGTRAP, current, 0);
 	}
 }
 
-- 
1.5.5.1

Re: [PATCH 0/2] more tweaks (Was: ptrace tweaks)

[Pedro Alves]

Yay, new bike shed!  ;-)

On 02/10/2012 05:32 PM, Oleg Nesterov wrote:

> Chris, iirc you suggested to add PTRACE_O_KILL_TRACEE_IF_DEBUGGER_DIES.
> The necessary change is simple, please suggest the good name for the
> new option ;)

PTRACE_O_KILL_ON_EXIT .  Cause Windows has had DebugSetProcessKillOnExit(bool) for ages.

-- 
Pedro Alves