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
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]>
Acked-by: Tejun Heo <[email protected]>
---
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
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]>
Acked-by: Tejun Heo <[email protected]>
---
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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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.
On Fri, Feb 10, 2012 at 4:57 PM, Oleg Nesterov <[email protected]> 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
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 <[email protected]>
Acked-by: Tejun Heo <[email protected]>
---
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
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 <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
> ---
> 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
>
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 <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
> ---
> 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
>
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 <[email protected]>
IIRC Tejun agreed with this change too.
Reviewed-by: Oleg Nesterov <[email protected]>
> ---
> 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
>
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 <[email protected]>
> Acked-by: Tejun Heo <[email protected]>
Reviewed-by: Oleg Nesterov <[email protected]>
> ---
> 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
>
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 <[email protected]>
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 <[email protected]>
-
> 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
>
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.
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 <[email protected]>
Tested-by: Indan Zupancic <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: Oleg Nesterov <[email protected]>
---
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
Hi,
Do new auto-attached clones still generate SIGSTOP, or has than been fixed already?
--
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
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.
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
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 <[email protected]>
>
> 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 <[email protected]>
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 <[email protected]>
Thanks!
--
tejun