Index | Thread | Search

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

Download raw body.

Thread
  • Theo de Raadt:

    SYS_pinsyscalls question

  • Mark Kettenis:

    SYS_pinsyscalls question

  • > 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;
    > 
    > 
    
    
    
  • Theo de Raadt:

    SYS_pinsyscalls question

  • Mark Kettenis:

    SYS_pinsyscalls question