kern: add a mac.label jail parameter

[kevans]

Summary

Have it take a struct mac and we'll paper over the difference for
jail(8)/jls(8) in libjail(3). The mac_syscalls.h model is taken from
mac_set_proc_*() that were previously done.

[jamie]

sys/kern/kern_jail.c

2856

Would it make sense to combine this with the mtx_unlock that immediately follows? On the plus side, you remove a lock with immediate unlock, but on the minus side it complicates the code, especially with MAC being inside an ifdef.

sys/security/mac/mac_syscalls.c

333

Since you're unlocking later anyway, it might be worthwhile to have a second try where you unlock and M_WAITOK, and then re-lock for mac_prison_copy_label.

[kevans]

sys/security/mac/mac_syscalls.c

333

I thought about it a bit, and I think I'd rather just reorder things slightly to allocate intlabel and do the copy early, then drop the lock and copyin the label. with M_WAITOK. We seem to have a little bit of a mixed bag about whether we sanity check the label rergardless of whether the objec type is actually labelled or not (see, e.g., kern___mac_get_path vs. sys___mac_get_fd), so the following ordering seems reasonable:

1.) vfs_copyopt so that we can bail out if a mac.label parameter wasn't even supplied
2.) Check mac_labeled so that we can return EINVAL early
3.) Allocate intlabel and mac_prison_copy_label so that we still have a snapshot of the label consistent with the other jail parameters
4.) Drop the lock, mac_label_copyin_string and malloc buffer both M_WAITOK, externalize and finish

I'd really prefer we keep the label copy under the lock if we can, but typical struct label allocation (+ most policies don't really allocate often in init_label) is small enough that I think failing to allocate intlabel is probably enough of an indicator that the other allocations would be a miserable wait. Does that seem like a reasonable compromise?

[jamie]

sys/security/mac/mac_syscalls.c

333

Yeah, that works. That's a good point on the allocation second tries in general: once we've gotten there, it's a very stressed system.

[kevans]

Highlights:

• Reorganize mac_get_prison() to reduce failure modes and drop the prison lock on the way out
• Assert that the lock is in the correct state afterwards in kern_jail_get()

[olce]

I'd put all new functions of sys/security/mac/mac_syscalls.c into sys/security/mac/mac_prison.c instead, as these are not really system calls, and export mac_label_copyin_string() from the former.

There's one suspect copy in that can probably be removed (see inline comment).

Seems fine otherwise.

sys/kern/kern_jail.c

2856

@jamie We are getting (and in kern_jail_set(), setting) module parameters without the prison lock, which poses atomicity problems (even in the simple case of concurrent retrieval and modification of jail parameters). This needs a re-design one day (tm).

sys/security/mac/mac_syscalls.c

356

Can't we just elide this copy in, as it seems we don't care about the content of the input buffer at all?

396–412

Maybe this code that does vfs_copyopt() on struct mac, also handling the 32-bit case, should be factored out, as it is the same as that of mac_get_prison().

[kevans]

sys/security/mac/mac_syscalls.c

356

My recollection is that externalization does actually use the input buffer to understand which labels it needs to try and capture, since it may not be able to capture all of them. This is where mac.conf(5) comes into play -- userland would populate the buffer based on labels there, and that's a crucial step to getting anything out of the MAC framework here.

[jamie]

sys/kern/kern_jail.c

2856

It's a limitation of the OSD system, which has an sxlock in osd_call. I didn't want to reinvent too many wheels (also the reason for vfsopt), so I went with the restriction. Even without the sxlock, I would expect locking problems, since modules will be wanting to use their own locking, or other things like process locks that aren't lower than a prison lock.

But yes, an atomic jail set would be valuable. OSD isn't the only place where that breaks - the retry of ipaddr allocation comes to mind.

[olce]

You're right about reading the struct mac to know which values to return (see inline comments).

Other comments still apply, but do not block this.

sys/security/mac/mac_syscalls.c

356

I've checked again and you're right. This is kind of awkward, but it's understandable why this was done, and it would be strange not to do it also for jail's labels whereas we already do so for process credentials, vnodes, etc.

[kevans]

In D53958#1241512, @olce wrote:

I'd put all new functions of sys/security/mac/mac_syscalls.c into sys/security/mac/mac_prison.c instead, as these are not really system calls, and export mac_label_copyin_string() from the former.

My working argument here was that these aren't syscalls, but they only exist to implement syscalls -- we're otherwise spreading out a bit as struct mac32 moves to sys/mac.h as the best candidate, and these declarations still go into mac_syscalls.h because that makes more sense than a one-off header (and matches the precedent that you set for the header at creation time).