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: Mon, 03 Mar 2025 22:19:13 +0100

> From: "Theo de Raadt" <deraadt@openbsd.org>
> Date: Mon, 03 Mar 2025 10:04:54 -0700
> 
> Mark Kettenis <mark.kettenis@xs4all.nl> wrote:
> 
> > here.  Although that can only happen if you write your own version of
> > ld.so that creates threads before pinning down the syscalls using
> > pinsyscalls(2).
> > 
> > The pinsyscals(2) code does
> > 
> >         pr->ps_libcpin.pn_start = base;
> >         pr->ps_libcpin.pn_end = base + len;
> >         pr->ps_libcpin.pn_pins = pins;
> >         pr->ps_libcpin.pn_npins = npins;
> > 
> > but another thread may observe those assignments in a different order
> > because there are no (memory) barriers.
> 
> I still don't see it.  The testing condition in syscall_mi.h is this:
> 
>         if (plibcpin->pn_pins &&
>             addr >= plibcpin->pn_start && addr < plibcpin->pn_end)
>                 pin = plibcpin;
> 
> At process start, the 3 libcpin variables are set to 0.
> 
> This test is checking if all 3 have become non-0.

No it isn't.

> If any are still 0, the libc pin is not chosen (goes into "pin"
> variable).

The

            addr >= plibcpin->pn_start

bit is true if pn_start is zero.

> 
> Please don't be confused by these "else" statements in the block:
> 
>         if (plibcpin->pn_pins &&
>             addr >= plibcpin->pn_start && addr < plibcpin->pn_end)
>                 pin = plibcpin;
>         else if (ppin->pn_pins &&
>             addr >= ppin->pn_start && addr < ppin->pn_end)
>                 pin = ppin;
>         else if (PROC_PC(p) == pr->ps_sigcoderet) {
> 
> This could be rewritten as
> 
>         if (plibcpin->pn_pins &&
>             addr >= plibcpin->pn_start && addr < plibcpin->pn_end)
>                 pin = plibcpin;
>         if (ppin->pn_pins &&
>             addr >= ppin->pn_start && addr < ppin->pn_end)
>                 pin = ppin;
>         if (PROC_PC(p) == pr->ps_sigcoderet) {
>                 if (code == SYS_sigreturn)
>                         return (0);
>                 error = EPERM;
>                 goto die;
>         }

That isn't the same.  In the second bit of code a match for ppin would
override a match on plibcpin.

> The "addr" or "PROC_PC(p)" cannot be inside all 3 of these ranges
> because these ranges cannot overlap.  So there is no priority scheme
> occuring here, the else's are improving the logic by avoiding pointless
> tests.

The ranges can appear to overlap in another thread that executes in
parallel with another thread that executes pinsyscalls(2).  Consider
the case where pinsyscalls(2) is setting the following values:

   ppin->pn_start = 0x1000
   ppin->pn_end   = 0x4000
   plibcpin->pn_start = 0x9000
   plibcpin->pn_end   = 0xc000

Without the (memory) barriers in my diff, the other thread might
observe this as:

   ppin->pn_start = 0x1000
   ppin->pn_end   = 0x4000
   plibcpin->pn_start = 0
   plibcpin->pn_end   = 0xc000

So if addr is (for example) 0x2000, this will match both the ppin and
ppinlibc areas, and since ppinlibc is checked it will incorrectly use
plicpin->pn_pins to check if the syscall is allowed.

> > Fortunately the code that
> > checks the libc.so syscall range explicitly checks pn_pins,
> 
> This won't happen, unless the address of those other threads is inside
> ld.so.  If the other thread is also doing a syscall from inside ld.so,
> then it is not doing it within the libc range, so it should be permitted
> based upon the ld.so pin table, which is ppin, not libcpin, this is case
> 
>          * 2b) dynamic binary: syscalls in ld.so (in the ps_pin region)
> 
> And ps_pin is not changed by pinsyscalls(), it is established at execve()
> time.
> 
> > I think the worst case is when the thread sees the correct pn_end,
> > pn_pins and pn_npins, but pn_start is still zero.  That could
> > potentially permit a syscall that shouldn't be allowed.
> 
> If pn_start is zero, then that pintable is not accepted, variable "pin"
> remains NULL, and therefore errno = ENOSYS, and the entire process reaches
> sigabort().
> 
> 
> I don't see the problem at all.
>