On Mon, Feb 24, 2020 at 8:27 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> On 2/22/20 4:33 PM, Paul Moore wrote:
> > On Fri, Feb 14, 2020 at 8:22 AM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> >> On 2/14/20 7:46 AM, Ondrej Mosnacek wrote:
> >>> On Wed, Jan 29, 2020 at 5:42 PM Stephen Smalley <sds@xxxxxxxxxxxxx> wrote:
> >>>> Fully decoupling the policy and kernel initial SID values will
> >>>> require introducing a mapping between them and dyhamically
> >>>
> >>> Nit: s/dyhamically/dynamically/
> >>
> >> Ah, thanks; will fix if I need to re-spin.
> >
> > Normally this would fall under the category of something I could
> > fix-up during a merge, but I think there are a few changes, mostly
> > documentation, that we should add to this patch.
> >
> > First off, I know MLS is the policy everyone wants to forget, but it
> > *is* used so let's not cause them any more pain then they are already
> > feeling. That should add a few initial SIDs back into the list, but I
> > think it still frees up plenty.
>
> Not sure exactly what you mean here. The patch should still remove all
> the unused initial SIDs (i.e. all initial SIDs for which there are no
> hardcoded references to SECINITSID_name). The MLS discussion (which was
> only in the GH issue, not in the patch description) is about which
> initial SIDs can we start reusing in the near term without any
> compatibility implications. So at most this affects what we say in the
> revised patch description when we pull in the GH issue information, not
> the patch itself.
Yes, I was thinking of isid reuse, but I wasn't very clear.
> I'd also question whether any MLS users would ever try to use a new
> kernel without also having a correspondingly updated policy; MLS users
> seems unlikely to run the latest kernels on existing distros.
That's a good point, but I see no reason why we should not preserve
that ability. Especially since I don't see us adding a lot of new
initial SIDs in the near future.
> > Second, when we load the initial SIDs, in policydb_load_isids(), you
> > show an error if one of these isid's is assigned a context:
> >
> > + if (sid == SECSID_NULL) {
> > + pr_err("SELinux: SID null was assigned a context.\n");
> >
> > ... I would suggest that we also display the SID number like below so
> > that policy devs have a better idea of which isid is causing the
> > problem:
> >
> > + if (sid == SECSID_NULL) {
> > + pr_err("SELinux: SID null(%u) was assigned a context.\n", sid);
>
> This isn't an error check for unused initial SIDs; it is retaining the
> existing test for the NULL (0) SID, which isn't supposed to ever be
> assigned a context, while dropping the restriction on adding initial
> SIDs > SECINITSID_NUM. sid can only be 0 here, and the message already
> says "null". If you'd prefer it say "SID 0 was assigned a context" I
> can do that if/when I can ever post to vger again.
Sorry, I misread the patch; it's fine the way it is.
I've kept your reply intact (no trimming), with the exception of my
own inline comments, since your mail didn't hit the list. Until you
find a way to workaround your mailing list problems Stephen I would
encourage others to do the same.
> > Lastly, and most importantly, there is a lot of good discussion,
> > including a "roadmap" in the GH issue, let's try to capture that in
> > this patch description (maybe minus your retirement plans Stephen
> > <g>). I have no idea where GH may be in a few years, but the git log
> > is FOREVER ;)
--
paul moore
www.paul-moore.com
