On Sun, Feb 23, 2025 at 5:21 PM Theo de Raadt <deraadt@openbsd.org> 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 <mjguzik gmail.com>