Re: [PATCH v2] ptrace: disable single step in __ptrace_unlink for protecting init task

From: Oleg Nesterov
Date: Tue Oct 25 2022 - 06:22:34 EST


On 10/25, chen zhang wrote:
>
> Thanks for your reply. I think kernel should not panic when the
> application has a bug, or a fault operation such as ctrl+c,

a) init is special. If it exits, the kernel panics. This is
by design.

b) debugger can always crash the tracee. In particular if it
exits without ptrace(PTRACE_DETACH) which implies
user_disable_single_step().

> This patch can really prevent kernel panic on
> my x86 machine.

Really? You ignored this part of my previous email,

Not to mention I don't understand how your patch can actually help. If nothing
else,

- debugger does ptrace(PTRACE_SINGLESTEP), this wakes the tracee up

- the tracee enters force_sig_info_to_task(SIGTRAP) after single step

- debugger exits, __ptrace_unlink() clears ptrace/TIF_SINGLESTEP

- force_sig_info_to_task() clears SIGNAL_UNKILLABLE, the traced init
will be killed.

Am I wrong?

Finally,

> --- a/kernel/ptrace.c
> +++ b/kernel/ptrace.c
> @@ -130,6 +130,8 @@ void __ptrace_unlink(struct task_struct *child)
> put_cred(old_cred);
>
> spin_lock(&child->sighand->siglock);
> + if (unlikely(child->signal->flags & SIGNAL_UNKILLABLE))
> + user_disable_single_step(child);
> child->ptrace = 0;
> /*

I don't understnd why do you call user_disable_single_step() with ->siglock
held, but this is minor.

user_disable_single_step(child) assumes that child is stopped and frozen,
see ptrace_freeze_traced(). This is not necessarily true if __ptrace_unlink()
is called by the exiting tracer, so the patch is wrong in any case.

Nack, sorry.

Oleg.