On Sun, Jun 12, 2022 at 12:12:33AM -0600, Theo de Raadt wrote: > David Gwynne <[email protected]> wrote: > > > On Wed, May 18, 2022 at 07:42:32PM -0600, Theo de Raadt wrote: > > > Mark Kettenis <[email protected]> wrote: > > > > > > > > Isn't the vm_map_lock enough? > > > > > > > > Could be. The fast path is going to take that lock anyway. This > > > > would require a bit of surgery to uvm_map_extract() to make sure we > > > > don't take the vm_map_lock twice. Worth exploring I'd say. > > > > > > I think the vm_map_lock can be dropped before it reaches that code, > > > because of 3 cases: (1) new kbind lock, (2) a repeated kbind lock and > > > return, or (3) violation and process termination. > > > > > > So before doing the copyin() and updates, simply vm_map_unlock() > > > > > > Will that work and isn't it simpler than David's proposal? > > > > I'm not super familiar with uvm so it's hard for me to say it would or > > wouldn't be simpler, but my initial impressions are that it would be a > > lot more surgery and I wouldn't be confident I did such changes right. > > i don't understand. As a rule, using fewer locks and mutexes is simpler > to understand. > > i do not understand the purpose for your diff, and think you have simply > brushed my questions aside. > > > Releasing a lock and then taking it again quickly can have undesirable > > consequences too. If something else is waiting on the rwlock, > > releasing it will wake them up, but they will probably lose because > > we've already taken it again. > > Then don't release the vm_map_lock and regrab it, but keep it active. > > I think there is a lot of fuss going on here for a system call which, as > far as I can tell, is never called in a threaded program. Static libc or > ld.so always call kbind early on, before threads, precisely ONCE, and any > later call to it kills the program.... but why do we need a mutex for the > simple action of observing that ps_kbind_addr is not 0? > > Am I wrong that kbind is never called twice in the same address space?
Isn't this exactly what happened the last time we tried this? > Can someone describe a synthetic sequence where racing sys_kbind calls > perform the wrong action? This is highly synthetic, but: If Thread1 sets ps_kbind_addr before Thread2 but Thread2 compares ps_kbind_cookie with SCARG(uap, proc_cookie) before Thread1 can change ps_kbind_addr from 0 to its own SCARG(uap, proc_cookie) it is possible that Thread2 will pass the security check and proceed, even though it should have caused a SIGILL. Thread2 knows ps_kbind_cookie is initialy zero, so there is a theoretical race. If you wanted to make this statistically impossible you could initialize ps_kbind_cookie to a random value during fork(2) so that Thread2 has a 1 in 2^64 chance of guessing the the initial ps_kbind_cookie value and bypassing the security check as described above. If you do that, then we can do the security check locklessly with atomic_cas_ulong(9).
