From: Martin Pieuchot <mpi@grenadille.net>
Subject: Re: patch: relax ni_pledge panic
To: Theo de Raadt <deraadt@openbsd.org>
Cc: Mark Kettenis <mark.kettenis@xs4all.nl>, semarie@kapouay.eu.org, tech@openbsd.org
Date: Thu, 6 Feb 2025 18:46:52 +0100

On 06/02/25(Thu) 09:43, Theo de Raadt wrote:
> 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.

I'd rather see a rwlock be used to serialized access to the per-process
data structures.  I don't see any reason to use the single thread API
for this and I'd rather not spread its usage.  It is already a pain to
work with.

> > 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
>