In D54833#1256867, @olce wrote:

In D54833#1256748, @kevans wrote:

rer: removing the prlabel, @csjp noted (paraphrasing, maybe poorly) that that's a MAC-philosophical thing that policy (writers?) shouldn't have to have 'that' much knowledge of kernel internals. It turns out that they often do anyways, but the historically consistent thing is that object labels get passed along with the labels to avoid the policy reaching into the object when it's something that they'll want to act on.

Mmm... I agree it's historical, and think it's essentially because the first MAC policies revolved around labels. However, "recent" ones have departed from these completely, so not sure if we should continue with the same trend. It's also slightly confusing at first that the same information are made available through different ways, e.g., via prlabel and pr->pr_label, leading to questions about what prlabel is exactly (e.g., is it stored outside the 'struct prison'?). Redundancy always introduce the consistency question, so in general should be avoided (here, it's not too hard to find what prlabel is, but that requires reading internal MAC code, defeating more or less the purpose of this "abstraction"). All this, compared to the only benefit I see which is avoiding code changes if we ever change pr_label to some other name or move it somewhere else, makes this practice really dubious to me. And we can actually have the same advantage differently by just providing a getter and a setter (or some variant).

I won't strongly object for this revision, but I think we should really consider removing passing labels in subsequent commits and replace with some getters/setters (possibly).

In D54833#1256564, @olce wrote:

In D54833#1255914, @kevans wrote:

prison_created is done after we know that the jail's going to stick around because I wanted the prison state to be 'final' (post-OSD) before we do any label propagation stuff... maybe that wasn't quite the right call.

Irrespective of labels, it seems important that we have a hook that is called when the prison is final and will (or is likely to) go live.

It seems to make sense to propagate labels only then, although I'm not sure the actual point of occurrence really matters in the end since I don't see cases where that propagation could affect modules setting data on the jail itself.

Perhaps should we remove the prlabel argument from both hooks (prison_cleanup and prison_created) as it feels really redundant?

For prison_created it makes more sense because that's the specific point where you're ideally propagating the label (to, e.g., the root vnode) now that all of these other things that could have possibly failed haven't done so. I would agree that there's probably not much point in prison_cleanup taking a label, though, since a labelled policy would probably just use destroy_label and an unlabelled policy wouldn't be able to do anything constructive with it.

But if some label is propagated to the root vnode on jail creation, shouldn't it be "unpropagated" on jail shutdown? In other words, I'm not sure I see a case for an asymmetry of hooks. And it seems we could have simple jail policies where there's no label propagation. With my current understanding, I'd still remove prlabel from these hooks (and probably more of them, such as check_get or check_set).

I have some more pseudofs patches in the pipeline still, I'll look at being consistent there as follow-up

In D54833#1255903, @olce wrote:

The /* Symmetry with prison_created */ comment is indeed welcome to clear the slight but apparently necessary confusion coming from the hook prison_cleanup being called in mac_prison_destroy() while prison_created is not called from mac_prison_init(). :-)

Gentle ping

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