In the interest of not leaving main broken, I'm going to push this since it's functional both with a jail policy and without- noting that I fully expect to perhaps need another round to cleanup some remaining issue(s) pertaining to the expedited timeline.

Extra fixes, also for bd55cbb50c58876

In D54786#1252172, @jlduran wrote:

I think this is not sufficient (just to avoid a missing mac.conf entry):
I will test it thoroughly later, but for now:

# jail -c path=/ name=D54786 persist
# jls -s

(Scales a little better in the sense that one can have an image that can do any number of multiple fs, and they can only disable autoloading of specific rootfs)

My preference would be that we add in a config.isModuleDisabled():

I'm not sure I'm convinced that dir is actually always set, but I haven't spent that much time reading the above logic. An assertion on that here might be good to try and do something useful instead of infinitely looping, but I don't insist

+des for recent stdio work, jhb for having worked on something like this specifically that hadn't landed

Address review comments

In D54168#1244291, @knz_thaumogen.net wrote:

Ahh, sorry, I got confused with the missing context- can you reupload this with either git-arc or -U99999, please?

Done. But IMHO I find this workflow tremendously impractical. With github I can do 1) fine-grained commits, with separate commit per logical change and 2) git push -f to update the PR.

Here all this gets mashed up together and I need to copy-paste the diff manually in a HTML form... 😭

Ahh, sorry, I got confused with the missing context- can you reupload this with either git-arc or -U99999, please? It's immensely useful to be able to scroll back up and confirm where different hunks are applying

I am quite confused about what happened here. Can you explain the connection between my previous note and the changes made?

I don't think this is actually sufficient, but I don't think fixing it will be all that hard. sprkopen currently uses the fact that it's locked by Giant, so you'll probably want one spkr mutex to be taken in spkropen() and spkrclose() to be sure it's only opened by a single thread (and not leaking an allocation if spkr_inbuf gets clobbered`).

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.

Highlights:

• Remove vfs_opterror() for those entry points that take the opts already
• Move one case of mac_prison_check_get back as a special-case to avoid breaking jail enumeration.
• Unbreak the build of this patch: prison_copy_label comes in a later change
• Drop redundant JAIL_ATTACH check

In D53954#1239739, @olce wrote:

In D53954#1239729, @kevans wrote:

I'm not sure I understand this last bit

Oh... It's just a pilot error on my part, sorry!

I'm already passing the struct vfsoptlist all around so that MAC modules can reject jail_[sg]et operations based on the parameters they want to fetch or set.

Don't know how I managed to forget that for a while.

I just wanted to limit propagation of struct vfsoptlist elsewhere, but passing it makes sense for some hooks, so nevermind.

In D53954#1239691, @olce wrote:

In D53954#1239668, @kevans wrote:

I'm not too worried about that, though it does occur to me that it probably makes sense to stop setting an error in the opts for any of those checks. The MAC policy should probably have free reign over both errno and error probably in case it's some kind of custom vendor integration.

I agree, we should probably stop setting the err opt. But that doesn't solve the discoverability of whether MAC (and not, e.g., real nonexistence of a jail) was the reason some access was denied, as in non-MAC we still set "errmsg" in the VFS options.

Giving this control to MAC modules would be nice; that requires obviously changing the interface of the hooks (if you do it, please don't pass struct vfsoptlist all around, but instead some function pointer to a printf-like function, so that we can clean up the use of struct vfsoptlist in jails at some point).

In D53954#1239654, @olce wrote:

Coming back to highlights, there's indeed still the problem that enumeration returns an error if the last jail is denied via mac_prison_check_get(), so doing what you suggest is probably mandatory: Put back the MAC check in the loop, and once a jail is returned, jump to a prison_found_nomac label, even a _nomac_noalive variant as to eliminate also the (harmless) redundant test.

Additionally, as mentioned before, there's that, if mac_prison_check_get() denies access, we return a VFS error saying "access denied by MAC" which would be an info leak for situations where the jail must remain hidden. E.g., we could have mac_prison_check_get() return EPERM for a visible deny and ENOENT for an invisible one, and then treat them differently with respect to vfs_opterror(). But it seems that goes farther than what you're envisioning at this point (I haven't checked further revisions in the series thoroughly yet, to see if they need something like this).

Ping

Ping; any other complaints besides the type nits? I'll fix those to use standard types for the next revision or pre commit.

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()

Highlights:

• Removed prison_check_list, because it's of limited value; this could pose a problem if check_get rejects a prison as lastjid enumeration would halt when MAC denied it, but that probably requires a different solution
• Moved mac_prison_check_get calls to after found_prison, but on the tail of that last note, it might make sense to provide a found_prison_nomac and move one check back into the lastjid loop and just continue if we encounter one that we can't fetch. This would be cleaner than policies having to implement both a check_list and check_get to avoid breaking enumeration
• Moved the attach check to the point that we're about to attach; there are other ways this can fail from external modules anyways (e.g., osd), so trying to keep it all atomic may not be worth as much as being sure that the policy can examine the current properties of the jail. This also ensures that the creation+attach case is covered
• Add missing unlocking

Highlights:

• Remove mac_prison_create_init(): one would probably want to special-case prison0 earlier in mac_prison_init(), and they don't actually need to know anything about prison0 specifically: pr_parent == NULL is a good indicator of the root prison
• Don't leak labels, call mac_prison_destroy()
• Drop a bunch of locking assertions in label operations