From: Mateusz Guzik Subject: Re: SYS_pinsyscalls question To: Theo de Raadt Cc: Marc Espie , tech@openbsd.org Date: Sun, 23 Feb 2025 18:01:36 +0100 On Sun, Feb 23, 2025 at 5:21 PM Theo de Raadt wrote: > > > 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. > Presumably someone wanting to mess with the kernel would go there. > 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. > Another angle is what happens when one thread is messing about with installing the table, while another is issuing syscalls. Note there is no synchronization between this and pin_check. It may be this does not blow up in practice at the moment, but I don't think depending on it is viable. The safest and easiest way out that I see is to only allow pinsyscalls if there is just one thread, assuming multithreaded usage was not meant to happen to begin with. > 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; -- Mateusz Guzik