Page MenuHomeFreeBSD

Clean up locking around do_jail_attach, fixing zombie jails from jail_attach_jd
Needs ReviewPublic

Authored by jamie on Fri, Jun 19, 5:51 PM.
Tags
None
Referenced Files
F160369313: D57674.diff
Tue, Jun 23, 7:17 PM
F160369298: D57674.diff
Tue, Jun 23, 7:17 PM
F160310297: D57674.id180083.diff
Tue, Jun 23, 3:20 AM
F160307590: D57674.id180083.diff
Tue, Jun 23, 2:47 AM
F160305606: D57674.id180198.diff
Tue, Jun 23, 2:34 AM
F160305572: D57674.id180198.diff
Tue, Jun 23, 2:34 AM
Unknown Object (File)
Tue, Jun 23, 12:44 AM
Unknown Object (File)
Tue, Jun 23, 12:17 AM
Subscribers

Details

Reviewers
markj
Summary

jail_attach_jd passed PD_DEREF to do_jail_attach, assuming it would take care of freeing the held prison. This is not true, as do_jail_attach immediately cleared that flag, leaving the jail stock in dying state when it is later removed. This is largely a documentation problem: add a comment block to do_jail_attach that lays out what it expects and what it returns. And now sys_jail_attach_fd does the right thing, calling prison_deref itself to remove its reference.

Also relax do_jail_attach's requirement, as a locked prison isn't necessary unless the lock is the only hold (it will just hold and unlock). This allows kern_jail_set (which holds a reference) to skip the prison_lock_xlock call.

Test Plan

The existing jaildesc tests tripped the bug, ending up with zombie jails (not reported as an error, but visible after the run). Now the jail list (jls -d) is clean.

Diff Detail

Repository
rG FreeBSD src repository
Lint
Lint Skipped
Unit
Tests Skipped

Event Timeline

jamie requested review of this revision.Fri, Jun 19, 5:51 PM
sys/kern/kern_jail.c
3249

Ignore these brackets - they're part of a fix for another jail_attach problem that I've decided not to merge with this one.

Removed an unrelated fix to a different jail_attach problem.

Looks right to me. My comments are tangential to the actual change.

sys/kern/kern_jail.c
3110

If we follow this goto, we're passing an uninitialized pr to prison_deref(). I think that happens to work, since prison_deref(pr, PD_LIST_SLOCKED) will not try to dereference pr, but you might defensively assign pr = NULL.

3130

To my taste, it'd be a bit cleaner to pass &drflags to do_jail_attach(), and have do_jail_attach() set/clear lock flags as appropriate before returning.

3154

Unrelated, but is this comment worth addressing? It's hard for me to convince myself that this couldn't be exploited somehow by an already-jailed process attaching to a pair of child jails.

This revision is now accepted and ready to land.Sat, Jun 20, 3:01 PM
jamie added inline comments.
sys/kern/kern_jail.c
3110

Good point, good safe practice.

3130

I though of this, and after some consideration went with option that was cleaner to code. But you're tilting that decision; even with the new comment block at the top of the function, making drflags continue to define the current state can prevent future errors like this.

3154

Yes, I'm looking into this - I've had this comment staring me in the face for a couple days. thread_single seems like the solution.

sys/kern/kern_jail.c
3154

That sounds reasonable to me. I suspect that it's quite rare for a multi-threaded program to call jail_attach() in the first place...

jamie marked an inline comment as done.

New diff with drflags pointer passed to do_jail_attach. I also removed the requirement that the jail be locked, which hasn't been the case since pr_ref and pr_uref went atomic.

This revision now requires review to proceed.Sat, Jun 20, 7:53 PM
sys/kern/kern_jail.c
3137

Typo, reflected

3163

When I ran the regression test suite with this patch applied, I got a panic:

panic: Cannot add a process to a non-alive prison (jid=31)                     
cpuid = 15                                                                                                                                                     
time = 1782242794                                                          
KDB: stack backtrace:                                                      
db_trace_self_wrapper() at db_trace_self_wrapper+0xa5/frame 0xfffffe00f11ca110                                                                                 
kdb_backtrace() at kdb_backtrace+0xc6/frame 0xfffffe00f11ca270                                                                                                                                                                                                                                                                
vpanic() at vpanic+0x214/frame 0xfffffe00f11ca410                              
panic() at panic+0xb5/frame 0xfffffe00f11ca4d0                                                                                                                 
do_jail_attach() at do_jail_attach+0x736/frame 0xfffffe00f11ca5f0                                                                                              
kern_jail_set() at kern_jail_set+0x5a21/frame 0xfffffe00f11cac70                                                                                               
sys_jail_set() at sys_jail_set+0xc3/frame 0xfffffe00f11cad10                                                                                                   
amd64_syscall() at amd64_syscall+0x3d5/frame 0xfffffe00f11caf30                                                                                                
fast_syscall_common() at fast_syscall_common+0xf8/frame 0xfffffe00f11caf30                                                                                     
--- syscall (507, FreeBSD ELF64, jail_set), rip = 0x181547a2674a, rsp = 0x181544f6fa08, rbp = 0x181544f6fa60 ---

I'm not sure yet if it's reproducible. Previously we didn't assert that uref > 0 before the increment, so I'm not sure if the assertion is too strong or whether there's another bug lurking...

sys/kern/kern_jail.c
3163

I think it's the former: lib/libc/tests/secure/fortify_unistd_test.c's dhost_jail() seems to trigger the crash, by creating and attaching to a jail in the same operation.

jamie added inline comments.
sys/kern/kern_jail.c
3163

Yes, that makes sense. I was hasty in replacing the refcount_acquire with the "more imformative" prison_proc_hold, forgetting the invariant part. Creating a jail with attachment and without persist does indeed count on do_jail_attach for its only user reference.

jamie marked an inline comment as done.

Don't change refcount_acquire to prison_proc_hold - the INVARIANTS test makes then not the same.