From: "Theo de Raadt" <deraadt@openbsd.org>
Subject: Re: patch: relax ni_pledge panic
To: Mark Kettenis <mark.kettenis@xs4all.nl>
Cc: semarie@kapouay.eu.org, tech@openbsd.org
Date: Thu, 06 Feb 2025 09:43:06 -0700

Mark Kettenis <mark.kettenis@xs4all.nl> wrote:

> > From: "Theo de Raadt" <deraadt@openbsd.org>
> > Date: Thu, 06 Feb 2025 09:17:52 -0700
> > 
> > > [2] in another thread, pledge("stdio rpath wpath"), and returns.
> > >    the process is now pledged.
> > 
> > How can this be allowed?
> > 
> > I am pretty sure sys_pledge should single-thread the process, which
> > means it will wait until other threads complete their in-kernel sleeps.
> 
> I'm not sure clauio@ will agree with you ;)

He just agreed with me privately.

> One possible stance would be to disallow pledge(2) after __tfork(2)
> has been called (and make it kill the process).  But I suspect there
> is code out there that does already do this... (cough, chromium).

I don't think that's the way forward.  Many other process-wide things
like process groups, gids, can be adjusted and affect threads.  They
work today.  Pledge and unveil should also work post-thread.

The problem is a majority of pledge checks happen before kernel sleep,
but a few happen after.

> > Obviously not all pledge-variable checks occur before the first
> > in-kernel sleep of other system calls.
> 
> And of course syzkaller is doing completely nonsensical things.  So a
> pledge failure that kills the process may be totally acceptable here.
> But it shouldn't panic the kernel.

I disagree.  It should panic the kernel.  An invarient has flipped.
This is quite a bad bug.

Maybe somethin glike this will ensure that the various pledge and
unveil variables on a process remain consistant through the operation
of all system calls.

Index: kern_pledge.c
===================================================================
RCS file: /cvs/src/sys/kern/kern_pledge.c,v
diff -u -p -u -r1.321 kern_pledge.c
--- kern_pledge.c	6 Oct 2024 23:39:24 -0000	1.321
+++ kern_pledge.c	6 Feb 2025 16:37:31 -0000
@@ -451,7 +451,7 @@ sys_pledge(struct proc *p, void *v, regi
 	} */	*uap = v;
 	struct process *pr = p->p_p;
 	uint64_t promises, execpromises;
-	int error;
+	int error = 0;
 	int unveil_cleanup = 0;
 
 	/* Check for any error in user input */
@@ -469,6 +469,7 @@ sys_pledge(struct proc *p, void *v, regi
 	}
 
 	mtx_enter(&pr->ps_mtx);
+	single_thread_set(p, SINGLE_UNWIND);
 
 	/* Check for any error wrt current promises */
 	if (SCARG(uap, promises)) {
@@ -481,16 +482,16 @@ sys_pledge(struct proc *p, void *v, regi
 		/* Only permit reductions */
 		if (ISSET(pr->ps_flags, PS_PLEDGE) &&
 		    (((promises | pr->ps_pledge) != pr->ps_pledge))) {
-			mtx_leave(&pr->ps_mtx);
-			return (EPERM);
+			error = EPERM;
+			goto fail;
 		}
 	}
 	if (SCARG(uap, execpromises)) {
 		/* Only permit reductions */
 		if (ISSET(pr->ps_flags, PS_EXECPLEDGE) &&
 		    (((execpromises | pr->ps_execpledge) != pr->ps_execpledge))) {
-			mtx_leave(&pr->ps_mtx);
-			return (EPERM);
+			error = EPERM;
+			goto fail;
 		}
 	}
 
@@ -509,6 +510,8 @@ sys_pledge(struct proc *p, void *v, regi
 		atomic_setbits_int(&pr->ps_flags, PS_EXECPLEDGE);
 	}
 
+fail:
+	single_thread_clear(p);
 	mtx_leave(&pr->ps_mtx);
 
 	if (unveil_cleanup) {
@@ -521,7 +524,7 @@ sys_pledge(struct proc *p, void *v, regi
 		KERNEL_UNLOCK();
 	}
 
-	return (0);
+	return (error);
 }
 
 int