Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 25 Oct 2022 12:19:00 +0200
From:   Oleg Nesterov <oleg@redhat.com>
To:     chen zhang <chenzhang@kylinos.cn>
Cc:     chenzhang_0901@163.com, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] ptrace: disable single step in __ptrace_unlink for
 protecting init task
Message-ID: <20221025101858.GB17158@redhat.com>
References: <20221025051032.280352-1-chenzhang@kylinos.cn>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20221025051032.280352-1-chenzhang@kylinos.cn>
User-Agent: Mutt/1.5.24 (2015-08-30)
Precedence: bulk

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.