linux-kernel - Re: [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT

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] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Apr 2022 18:58:17 +0200
From:   Peter Zijlstra <peterz@...radead.org>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     rjw@...ysocki.net, mingo@...nel.org, vincent.guittot@...aro.org,
        dietmar.eggemann@....com, rostedt@...dmis.org, mgorman@...e.de,
        ebiederm@...ssion.com, bigeasy@...utronix.de,
        Will Deacon <will@...nel.org>, linux-kernel@...r.kernel.org,
        tj@...nel.org, linux-pm@...r.kernel.org
Subject: Re: [PATCH 2/5] sched,ptrace: Fix ptrace_check_attach() vs PREEMPT_RT

On Wed, Apr 13, 2022 at 03:24:52PM +0200, Oleg Nesterov wrote:
> Hi Peter,
> 
> I like 1-2 but I need to read them (and other patches) again, a
> couple of nits right now.
> 
> On 04/12, Peter Zijlstra wrote:
> >
> > +static int __ptrace_freeze_cond(struct task_struct *p)
> > +{
> > +	if (!task_is_traced(p))
> > +		return -ESRCH;
> 
> 	if (!task_is_traced(p) || p->parent != current)
> 		return -ESRCH;
> 
> we should not spin/sleep if it is traced by another task

Yes, fair enough. And I suppose doing this test without holding siglock
is safe enough.

> > +static int __ptrace_freeze(struct task_struct *p, void *arg)
> > +{
> > +	int ret;
> > +
> > +	ret = __ptrace_freeze_cond(p);
> > +	if (ret)
> > +		return ret;
> > +
> > +	/*
> > +	 * Task scheduled between __ptrace_pre_freeze() and here, not our task
> > +	 * anymore.
> > +	 */
> > +	if (*(unsigned long *)arg != p->nvcsw)
> > +		return -ESRCH;
> > +
> > +	if (looks_like_a_spurious_pid(p))
> > +		return -ESRCH;
> 
> Oh, I do not think __ptrace_freeze() should check for spurious pid...
> looks_like_a_spurious_pid() should be called once in ptrace_check_attach()
> before task_call_func(__ptrace_freeze).

I can certainly do that, but since that needs be done with siglock held,
and the __ptrace_freeze call is a one-time affair, I didn't really see
the point in making the code more complicated.

Something like so then?

--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c
@@ -222,7 +222,7 @@ static void ptrace_unfreeze_traced(struc
  */
 static int __ptrace_freeze_cond(struct task_struct *p)
 {
-	if (!task_is_traced(p))
+	if (!task_is_traced(p) || p->parent != current)
 		return -ESRCH;
 
 	if (task_curr(p))
@@ -283,9 +283,6 @@ static int __ptrace_freeze(struct task_s
 	if (*(unsigned long *)arg != p->nvcsw)
 		return -ESRCH;
 
-	if (looks_like_a_spurious_pid(p))
-		return -ESRCH;
-
 	if (__fatal_signal_pending(p))
 		return -ESRCH;
 
@@ -378,6 +375,9 @@ static int ptrace_check_attach(struct ta
 		 * does ptrace_unlink() before __exit_signal().
 		 */
 		spin_lock_irq(&child->sighand->siglock);
+		if (looks_like_a_spurious_pid(child))
+			goto unlock_sig;
+
 		ret = task_call_func(child, __ptrace_freeze, &nvcsw);
 		if (ret) {
 			/*
@@ -386,6 +386,7 @@ static int ptrace_check_attach(struct ta
 			 */
 			ret = -ESRCH;
 		}
+unlock_sig:
 		spin_unlock_irq(&child->sighand->siglock);
 	}
 unlock: