The signal a task should continue with after a ptrace stop is
inconsistently read, cleared, and sent. Solve this by reading and
clearing the signal to be sent in ptrace_stop.
In an ideal world everything except ptrace_signal would share a common
implementation of continuing with the signal, so ptracers could count
on the signal they ask to continue with actually being delivered. For
now retain bug compatibility and just return with the signal number
the ptracer requested the code continue with.
Signed-off-by: "Eric W. Biederman" <[email protected]>
---
include/linux/ptrace.h | 12 ++++++------
kernel/signal.c | 31 ++++++++++++++++++-------------
2 files changed, 24 insertions(+), 19 deletions(-)
diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
index 3e6b46e2b7be..15b3d176b6b4 100644
--- a/include/linux/ptrace.h
+++ b/include/linux/ptrace.h
@@ -60,7 +60,7 @@ extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned
extern void ptrace_disable(struct task_struct *);
extern int ptrace_request(struct task_struct *child, long request,
unsigned long addr, unsigned long data);
-extern void ptrace_notify(int exit_code, unsigned long message);
+extern int ptrace_notify(int exit_code, unsigned long message);
extern void __ptrace_link(struct task_struct *child,
struct task_struct *new_parent,
const struct cred *ptracer_cred);
@@ -419,21 +419,21 @@ extern void sigaction_compat_abi(struct k_sigaction *act, struct k_sigaction *oa
static inline int ptrace_report_syscall(unsigned long message)
{
int ptrace = current->ptrace;
+ int signr;
if (!(ptrace & PT_PTRACED))
return 0;
- ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0), message);
+ signr = ptrace_notify(SIGTRAP | ((ptrace & PT_TRACESYSGOOD) ? 0x80 : 0),
+ message);
/*
* this isn't the same as continuing with a signal, but it will do
* for normal use. strace only continues with a signal if the
* stopping signal is not SIGTRAP. -brl
*/
- if (current->exit_code) {
- send_sig(current->exit_code, current, 1);
- current->exit_code = 0;
- }
+ if (signr)
+ send_sig(signr, current, 1);
return fatal_signal_pending(current);
}
diff --git a/kernel/signal.c b/kernel/signal.c
index a49ac7149256..e53ee84b9021 100644
--- a/kernel/signal.c
+++ b/kernel/signal.c
@@ -2188,15 +2188,17 @@ static void do_notify_parent_cldstop(struct task_struct *tsk,
* That makes it a way to test a stopped process for
* being ptrace-stopped vs being job-control-stopped.
*
- * If we actually decide not to stop at all because the tracer
- * is gone, we keep current->exit_code unless clear_code.
+ * Returns the signal the ptracer requested the code resume
+ * with. If the code did not stop because the tracer is gone,
+ * the stop signal remains unchanged unless clear_code.
*/
-static void ptrace_stop(int exit_code, int why, int clear_code,
+static int ptrace_stop(int exit_code, int why, int clear_code,
unsigned long message, kernel_siginfo_t *info)
__releases(¤t->sighand->siglock)
__acquires(¤t->sighand->siglock)
{
bool gstop_done = false;
+ bool read_code = true;
if (arch_ptrace_stop_needed()) {
/*
@@ -2305,8 +2307,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
/* tasklist protects us from ptrace_freeze_traced() */
__set_current_state(TASK_RUNNING);
+ read_code = false;
if (clear_code)
- current->exit_code = 0;
+ exit_code = 0;
read_unlock(&tasklist_lock);
}
@@ -2316,8 +2319,10 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
* any signal-sending on another CPU that wants to examine it.
*/
spin_lock_irq(¤t->sighand->siglock);
+ if (read_code) exit_code = current->exit_code;
current->last_siginfo = NULL;
current->ptrace_message = 0;
+ current->exit_code = 0;
/* LISTENING can be set only during STOP traps, clear it */
current->jobctl &= ~JOBCTL_LISTENING;
@@ -2328,9 +2333,10 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
* This sets TIF_SIGPENDING, but never clears it.
*/
recalc_sigpending_tsk(current);
+ return exit_code;
}
-static void ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
+static int ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
{
kernel_siginfo_t info;
@@ -2341,18 +2347,21 @@ static void ptrace_do_notify(int signr, int exit_code, int why, unsigned long me
info.si_uid = from_kuid_munged(current_user_ns(), current_uid());
/* Let the debugger run. */
- ptrace_stop(exit_code, why, 1, message, &info);
+ return ptrace_stop(exit_code, why, 1, message, &info);
}
-void ptrace_notify(int exit_code, unsigned long message)
+int ptrace_notify(int exit_code, unsigned long message)
{
+ int signr;
+
BUG_ON((exit_code & (0x7f | ~0xffff)) != SIGTRAP);
if (unlikely(task_work_pending(current)))
task_work_run();
spin_lock_irq(¤t->sighand->siglock);
- ptrace_do_notify(SIGTRAP, exit_code, CLD_TRAPPED, message);
+ signr = ptrace_do_notify(SIGTRAP, exit_code, CLD_TRAPPED, message);
spin_unlock_irq(¤t->sighand->siglock);
+ return signr;
}
/**
@@ -2511,7 +2520,6 @@ static void do_jobctl_trap(void)
} else {
WARN_ON_ONCE(!signr);
ptrace_stop(signr, CLD_STOPPED, 0, 0, NULL);
- current->exit_code = 0;
}
}
@@ -2564,15 +2572,12 @@ static int ptrace_signal(int signr, kernel_siginfo_t *info, enum pid_type type)
* comment in dequeue_signal().
*/
current->jobctl |= JOBCTL_STOP_DEQUEUED;
- ptrace_stop(signr, CLD_TRAPPED, 0, 0, info);
+ signr = ptrace_stop(signr, CLD_TRAPPED, 0, 0, info);
/* We're back. Did the debugger cancel the sig? */
- signr = current->exit_code;
if (signr == 0)
return signr;
- current->exit_code = 0;
-
/*
* Update the siginfo structure if the signal has
* changed. If the debugger wanted something
--
2.29.2
Not sure I understand this patch, I can't apply it. I guess I need to
clone your tree first, will do later.
Just one question right now,
On 03/15, Eric W. Biederman wrote:
>
> +static int ptrace_stop(int exit_code, int why, int clear_code,
> unsigned long message, kernel_siginfo_t *info)
> __releases(¤t->sighand->siglock)
> __acquires(¤t->sighand->siglock)
> {
> bool gstop_done = false;
> + bool read_code = true;
>
> if (arch_ptrace_stop_needed()) {
> /*
> @@ -2305,8 +2307,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
>
> /* tasklist protects us from ptrace_freeze_traced() */
> __set_current_state(TASK_RUNNING);
> + read_code = false;
> if (clear_code)
> - current->exit_code = 0;
> + exit_code = 0;
> read_unlock(&tasklist_lock);
> }
>
> @@ -2316,8 +2319,10 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
> * any signal-sending on another CPU that wants to examine it.
> */
> spin_lock_irq(¤t->sighand->siglock);
> + if (read_code) exit_code = current->exit_code;
style ;)
> current->last_siginfo = NULL;
> current->ptrace_message = 0;
> + current->exit_code = 0;
OK, I like it... but can't we remove the ugly "int clear_code" arg?
Oleg.
Oleg Nesterov <[email protected]> writes:
> Not sure I understand this patch, I can't apply it. I guess I need to
> clone your tree first, will do later.
>
> Just one question right now,
>
> On 03/15, Eric W. Biederman wrote:
>>
>> +static int ptrace_stop(int exit_code, int why, int clear_code,
>> unsigned long message, kernel_siginfo_t *info)
>> __releases(¤t->sighand->siglock)
>> __acquires(¤t->sighand->siglock)
>> {
>> bool gstop_done = false;
>> + bool read_code = true;
>>
>> if (arch_ptrace_stop_needed()) {
>> /*
>> @@ -2305,8 +2307,9 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
>>
>> /* tasklist protects us from ptrace_freeze_traced() */
>> __set_current_state(TASK_RUNNING);
>> + read_code = false;
>> if (clear_code)
>> - current->exit_code = 0;
>> + exit_code = 0;
>> read_unlock(&tasklist_lock);
>> }
>>
>> @@ -2316,8 +2319,10 @@ static void ptrace_stop(int exit_code, int why, int clear_code,
>> * any signal-sending on another CPU that wants to examine it.
>> */
>> spin_lock_irq(¤t->sighand->siglock);
>> + if (read_code) exit_code = current->exit_code;
>
> style ;)
>
>> current->last_siginfo = NULL;
>> current->ptrace_message = 0;
>> + current->exit_code = 0;
>
> OK, I like it... but can't we remove the ugly "int clear_code" arg?
The flag clear_code controls what happens if a ptrace_stop does not
stop. In particular clear_code means do not continue with
a signal if we can not stop.
For do_jobctl_trap calling ptrace_stop it clearly does not matter.
For ptrace_signal it would be a change in behavior, that would
cause the signal not to be delivered.
For ptrace_do_notify we don't have a signal to deliver unless userspace
gives us one so clear_code makes sense at that point.
Which is a long way of saying that no we can not remove clear_stop
because the behavior it is used to implement makes sense and userspace
can reasonably depend upon it.
Eric
On Tue, Mar 15, 2022 at 06:22:26PM -0500, Eric W. Biederman wrote:
>
> The signal a task should continue with after a ptrace stop is
> inconsistently read, cleared, and sent. Solve this by reading and
> clearing the signal to be sent in ptrace_stop.
>
> In an ideal world everything except ptrace_signal would share a common
> implementation of continuing with the signal, so ptracers could count
> on the signal they ask to continue with actually being delivered. For
> now retain bug compatibility and just return with the signal number
> the ptracer requested the code continue with.
>
> Signed-off-by: "Eric W. Biederman" <[email protected]>
> ---
> include/linux/ptrace.h | 12 ++++++------
> kernel/signal.c | 31 ++++++++++++++++++-------------
> 2 files changed, 24 insertions(+), 19 deletions(-)
>
> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> index 3e6b46e2b7be..15b3d176b6b4 100644
> --- a/include/linux/ptrace.h
> +++ b/include/linux/ptrace.h
> @@ -60,7 +60,7 @@ extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned
> extern void ptrace_disable(struct task_struct *);
> extern int ptrace_request(struct task_struct *child, long request,
> unsigned long addr, unsigned long data);
> -extern void ptrace_notify(int exit_code, unsigned long message);
> +extern int ptrace_notify(int exit_code, unsigned long message);
> [...]
> -static void ptrace_stop(int exit_code, int why, int clear_code,
> +static int ptrace_stop(int exit_code, int why, int clear_code,
> unsigned long message, kernel_siginfo_t *info)
> [...]
> -static void ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
> +static int ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
> [...]
> -void ptrace_notify(int exit_code, unsigned long message)
> +int ptrace_notify(int exit_code, unsigned long message)
Just for robustness, how about marking the functions that have switched
from void to int return as __must_check (or at least just ptrace_notify)?
With that and the style nit Oleg already mentioned, yeah, this looks
good too.
Reviewed-by: Kees Cook <[email protected]>
--
Kees Cook
Kees Cook <[email protected]> writes:
> On Tue, Mar 15, 2022 at 06:22:26PM -0500, Eric W. Biederman wrote:
>>
>> The signal a task should continue with after a ptrace stop is
>> inconsistently read, cleared, and sent. Solve this by reading and
>> clearing the signal to be sent in ptrace_stop.
>>
>> In an ideal world everything except ptrace_signal would share a common
>> implementation of continuing with the signal, so ptracers could count
>> on the signal they ask to continue with actually being delivered. For
>> now retain bug compatibility and just return with the signal number
>> the ptracer requested the code continue with.
>>
>> Signed-off-by: "Eric W. Biederman" <[email protected]>
>> ---
>> include/linux/ptrace.h | 12 ++++++------
>> kernel/signal.c | 31 ++++++++++++++++++-------------
>> 2 files changed, 24 insertions(+), 19 deletions(-)
>>
>> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
>> index 3e6b46e2b7be..15b3d176b6b4 100644
>> --- a/include/linux/ptrace.h
>> +++ b/include/linux/ptrace.h
>> @@ -60,7 +60,7 @@ extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned
>> extern void ptrace_disable(struct task_struct *);
>> extern int ptrace_request(struct task_struct *child, long request,
>> unsigned long addr, unsigned long data);
>> -extern void ptrace_notify(int exit_code, unsigned long message);
>> +extern int ptrace_notify(int exit_code, unsigned long message);
>> [...]
>> -static void ptrace_stop(int exit_code, int why, int clear_code,
>> +static int ptrace_stop(int exit_code, int why, int clear_code,
>> unsigned long message, kernel_siginfo_t *info)
>> [...]
>> -static void ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
>> +static int ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
>> [...]
>> -void ptrace_notify(int exit_code, unsigned long message)
>> +int ptrace_notify(int exit_code, unsigned long message)
>
> Just for robustness, how about marking the functions that have switched
> from void to int return as __must_check (or at least just ptrace_notify)?
We can't. There are historical cases that simply don't check if a
signal should be sent after the function, and they exist for every
function that is modified.
If we can modify the code so that everyone is checking the return value
than certainly, but that just doesn't happen to reflect how this
ptrace helper is being used today.
> With that and the style nit Oleg already mentioned, yeah, this looks
> good too.
Alright style nit fixed.
> Reviewed-by: Kees Cook <[email protected]>
Eric
On Fri, Mar 18, 2022 at 09:52:46AM -0500, Eric W. Biederman wrote:
> Kees Cook <[email protected]> writes:
>
> > On Tue, Mar 15, 2022 at 06:22:26PM -0500, Eric W. Biederman wrote:
> >>
> >> The signal a task should continue with after a ptrace stop is
> >> inconsistently read, cleared, and sent. Solve this by reading and
> >> clearing the signal to be sent in ptrace_stop.
> >>
> >> In an ideal world everything except ptrace_signal would share a common
> >> implementation of continuing with the signal, so ptracers could count
> >> on the signal they ask to continue with actually being delivered. For
> >> now retain bug compatibility and just return with the signal number
> >> the ptracer requested the code continue with.
> >>
> >> Signed-off-by: "Eric W. Biederman" <[email protected]>
> >> ---
> >> include/linux/ptrace.h | 12 ++++++------
> >> kernel/signal.c | 31 ++++++++++++++++++-------------
> >> 2 files changed, 24 insertions(+), 19 deletions(-)
> >>
> >> diff --git a/include/linux/ptrace.h b/include/linux/ptrace.h
> >> index 3e6b46e2b7be..15b3d176b6b4 100644
> >> --- a/include/linux/ptrace.h
> >> +++ b/include/linux/ptrace.h
> >> @@ -60,7 +60,7 @@ extern int ptrace_writedata(struct task_struct *tsk, char __user *src, unsigned
> >> extern void ptrace_disable(struct task_struct *);
> >> extern int ptrace_request(struct task_struct *child, long request,
> >> unsigned long addr, unsigned long data);
> >> -extern void ptrace_notify(int exit_code, unsigned long message);
> >> +extern int ptrace_notify(int exit_code, unsigned long message);
> >> [...]
> >> -static void ptrace_stop(int exit_code, int why, int clear_code,
> >> +static int ptrace_stop(int exit_code, int why, int clear_code,
> >> unsigned long message, kernel_siginfo_t *info)
> >> [...]
> >> -static void ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
> >> +static int ptrace_do_notify(int signr, int exit_code, int why, unsigned long message)
> >> [...]
> >> -void ptrace_notify(int exit_code, unsigned long message)
> >> +int ptrace_notify(int exit_code, unsigned long message)
> >
> > Just for robustness, how about marking the functions that have switched
> > from void to int return as __must_check (or at least just ptrace_notify)?
>
> We can't. There are historical cases that simply don't check if a
> signal should be sent after the function, and they exist for every
> function that is modified.
This seems at least worth documenting with a comment, otherwise we're
just trading one kind of "weirdness" (setting/clearing
current->exit_code) with another (ignoring the signal returned by
ptrace_notify()).
I see only two cases that would need comments:
static inline void ptrace_event(int event, unsigned long message)
{
if (unlikely(ptrace_event_enabled(current, event))) {
ptrace_notify((event << 8) | SIGTRAP, message);
} else if (event == PTRACE_EVENT_EXEC) {
/* legacy EXEC report via SIGTRAP */
if ((current->ptrace & (PT_PTRACED|PT_SEIZED)) == PT_PTRACED)
send_sig(SIGTRAP, current, 0);
}
}
static void signal_delivered(struct ksignal *ksig, int stepping)
{
...
if (stepping)
ptrace_notify(SIGTRAP, 0);
}
--
Kees Cook
On 03/17, Eric W. Biederman wrote:
>
> Oleg Nesterov <[email protected]> writes:
>
> > OK, I like it... but can't we remove the ugly "int clear_code" arg?
>
> The flag clear_code controls what happens if a ptrace_stop does not
> stop. In particular clear_code means do not continue with
> a signal if we can not stop.
>
> For do_jobctl_trap calling ptrace_stop it clearly does not matter.
>
> For ptrace_signal it would be a change in behavior, that would
> cause the signal not to be delivered.
Well I meant that "clear_code" should be false, iirc only
ptrace_report_syscall() should be updated to void the spurious
send_sig() if debugger exits... Nevermind, pleasee forget, this is
not as trivial as I thought.
Acked-by: Oleg Nesterov <[email protected]>
Oleg Nesterov <[email protected]> writes:
> Not sure I understand this patch, I can't apply it. I guess I need to
> clone your tree first, will do later.
Yes this series is on top of:
https://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git kill-tracehook-for-v5.18
Nothing particularly interesting happens in my kill tracehook series
but I do get rid of tracehook.h and so everything gets moved and
renamed.
Eric