From: Mark Kettenis <mark.kettenis@xs4all.nl>
Subject: Re: SYS_pinsyscalls question
To: "Theo de Raadt" <deraadt@openbsd.org>
Cc: mjguzik@gmail.com, marc.espie.openbsd@gmail.com, tech@openbsd.org
Date: Sun, 23 Feb 2025 18:29:02 +0100

> From: "Theo de Raadt" <deraadt@openbsd.org>
> Date: Sun, 23 Feb 2025 09:21:26 -0700
> 
> > Then this copyin:
> >           error = copyin(SCARG(uap, pins), pins, npins * sizeof(u_int));
> >           if (error)
> >                   goto err;
> > 
> > will go off cpu and drop the kernel lock, allowing other threads to
> > reach the same point, all still observing pr->ps_libcpin.pn_start == 0.
> > 
> > Then several threads will reach this assignment:
> >           pr->ps_libcpin.pn_start = base;
> > 
> > is going to override all previously set bufs, leaking them.
> 
> Yes, would need very custom crt0 in those static binaries.

Static binaries can't get there because they will have
VM_MAP_PINSYSCALL_ONCE set.  Do you mean a specially crafted ld.so?

> This diff should allow the first thread to do pinsyscalls to win, and
> re-check the condition post-sleep so that others lose and free their
> pintable.  EPERM matches the existing spec in the man page.

Is there any reason why we can't set VM_MAP_PINSYSCALL_ONCE early in
the syscall?  Then we can also simplify the EPERM condition.  Something like:


        if (pr->ps_vmspace->vm_map.flags & VM_MAP_PINSYSCALL_ONCE)
	        return (EPERM);
	mtx_enter(&pr->ps_vmspace->vm_map.flags_lock);
	pr->ps_vmspace->vm_map.flags |= VM_MAP_PINSYSCALL_ONCE;
	mtx_leave(&pr->ps_vmspace->vm_map.flags_lock);

	...

This doesn mean that if someone invokes pinsyscalls(2) with bogus
parameters they will have blown their one chance at it.  But I'd argue
that's a good thing ;).

> Index: uvm_mmap.c
> ===================================================================
> RCS file: /cvs/src/sys/uvm/uvm_mmap.c,v
> diff -u -p -u -r1.193 uvm_mmap.c
> --- uvm_mmap.c	14 Dec 2024 12:07:38 -0000	1.193
> +++ uvm_mmap.c	23 Feb 2025 16:20:01 -0000
> @@ -648,6 +648,10 @@ err:
>  		free(pins, M_PINSYSCALL, npins * sizeof(u_int));
>  		return (error);
>  	}
> +	if (pr->ps_libcpin.pn_pins) {	/* raced by another static thread */
> +		error = EPERM;
> +		goto err;
> +	}
>  	pr->ps_libcpin.pn_start = base;
>  	pr->ps_libcpin.pn_end = base + len;
>  	pr->ps_libcpin.pn_pins = pins;
> 
>