From 08a2cd30bad7720cf6250d149a96f35524cf9e66 Mon Sep 17 00:00:00 2001
From: cinap_lenrek <cinap_lenrek@felloff.net>
Date: Mon, 2 May 2022 19:34:00 +0000
Subject: [PATCH] kernel: fix noteid change race condition from devproc while
 forking (thanks joe7)

devproc allows changing the noteid of another process
which opens a race condition in sysrfork(), when deciding
to inherit the noteid of "up" to the child and calling
pidalloc() later to take the reference, the noteid could
have been changed and the childs noteid could have been
freed already in the process.

this bug can only happen when one writes the /proc/n/noteid
file of a another process than your own that is in the
process of forking.

the noteid changing functionality of devproc seems questinable
and seems to be only used by ape's setpgrid() implementation.
---
 sys/src/9/port/proc.c    | 29 ++++++++++++++++++-----------
 sys/src/9/port/sysproc.c | 12 +++++++++---
 2 files changed, 27 insertions(+), 14 deletions(-)

diff --git a/sys/src/9/port/proc.c b/sys/src/9/port/proc.c
index e1acf340e..c8deaa401 100644
--- a/sys/src/9/port/proc.c
+++ b/sys/src/9/port/proc.c
@@ -1820,6 +1820,19 @@ procindex(ulong pid)
 	return -1;
 }
 
+/*
+ *  for the following functions:
+ *    setnoteid(), pidalloc() and pidfree()
+ *
+ *  They need to be called with p->debug held
+ *  to prevent devproc's changenoteid() from
+ *  changing the noteid under us and make the
+ *  update atomic.
+ */
+
+/*
+ *  change the noteid of a process
+ */
 ulong
 setnoteid(Proc *p, ulong noteid)
 {
@@ -1840,15 +1853,6 @@ setnoteid(Proc *p, ulong noteid)
 	return p->noteid = i->pid;
 }
 
-static ulong
-setparentpid(Proc *p, Proc *pp)
-{
-	Pid *i;
-
-	i = pidadd(pp->pid);
-	return p->parentpid = i->pid;
-}
-
 /*
  *  allocate pid, noteid and parentpid to a process
  */
@@ -1858,8 +1862,11 @@ pidalloc(Proc *p)
 	Pid *i;
 
 	/* skip for the boot process */
-	if(up != nil)
-		setparentpid(p, up);
+	if(up != nil){
+		i = pidadd(up->pid);
+		p->parentpid = i->pid;
+	} else
+		p->parentpid = 0;
 
 	i = pidadd(0);
 	i->procindex = (int)(p - procalloc.arena);
diff --git a/sys/src/9/port/sysproc.c b/sys/src/9/port/sysproc.c
index f451e047d..b3b7a0439 100644
--- a/sys/src/9/port/sysproc.c
+++ b/sys/src/9/port/sysproc.c
@@ -79,14 +79,18 @@ sysrfork(va_list list)
 				envcpy(up->egrp, oeg);
 			closeegrp(oeg);
 		}
-		if(flag & RFNOTEG)
-			setnoteid(up, 0);
+		if(flag & RFNOTEG){
+			qlock(&up->debug);
+			setnoteid(up, 0);	/* can't error() with 0 argument */
+			qunlock(&up->debug);
+		}
 		return 0;
 	}
 
 	if((p = newproc()) == nil)
 		error("no procs");
 
+	qlock(&up->debug);
 	qlock(&p->debug);
 
 	p->scallnr = up->scallnr;
@@ -112,7 +116,8 @@ sysrfork(va_list list)
 		p->procctl = Proc_tracesyscall;
 	p->kp = 0;
 
-	/* Craft a return frame which will cause the child to pop out of
+	/*
+	 * Craft a return frame which will cause the child to pop out of
 	 * the scheduler in user mode with the return register zero
 	 */
 	forkchild(p, up->dbgreg);
@@ -132,6 +137,7 @@ sysrfork(va_list list)
 	pid = pidalloc(p);
 
 	qunlock(&p->debug);
+	qunlock(&up->debug);
 
 	/* Abort the child process on error */
 	if(waserror()){