linux-kernel - Re: [PATCH v2] ptrace: disable single step in __ptrace

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Tue, 25 Oct 2022 12:19:00 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     chen zhang <chenzhang@...inos.cn>
Cc:     chenzhang_0901@....com, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] ptrace: disable single step in __ptrace_unlink for
 protecting init task

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.