On Sat, Jun 17, 2023 at 5:30 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> On Fri, Jun 16, 2023 at 10:43 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > On Mon, Jun 12, 2023 at 5:01 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> ...
>
> > > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> > > index 99ded60a6b911..dd410ceb178cb 100644
> > > --- a/security/selinux/hooks.c
> > > +++ b/security/selinux/hooks.c
> > > @@ -2264,6 +2264,18 @@ static int selinux_bprm_creds_for_exec(struct linux_binprm *bprm)
> > > new_tsec->keycreate_sid = 0;
> > > new_tsec->sockcreate_sid = 0;
> > >
> > > + /*
> > > + * Before policy is loaded, label any task outside kernel space
> > > + * as SECINITSID_INIT, so that any userspace tasks surviving from
> > > + * early boot end up with a label different from SECINITSID_KERNEL
> > > + * (if the policy chooses to set SECINITSID_INIT != SECINITSID_KERNEL).
> > > + */
> > > + if (!selinux_initialized()) {
> > > + new_tsec->sid = SECINITSID_INIT;
> > > + new_tsec->exec_sid = 0; /* just in case */
> >
> > Style nit, I don't like placing trailing comments on the same line as
> > code. Don't respin this patch just for this, but remember this for
> > future submissions.
>
> Ack.
>
> > > + return 0;
> > > + }
> > > +
> > > if (old_tsec->exec_sid) {
> > > new_tsec->sid = old_tsec->exec_sid;
> > > /* Reset exec SID on execve. */
> >
> > ...
> >
> > > diff --git a/security/selinux/ss/policydb.c b/security/selinux/ss/policydb.c
> > > index 97c0074f9312a..240e0fb1d57f9 100644
> > > --- a/security/selinux/ss/policydb.c
> > > +++ b/security/selinux/ss/policydb.c
> > > @@ -863,6 +863,8 @@ void policydb_destroy(struct policydb *p)
> > > int policydb_load_isids(struct policydb *p, struct sidtab *s)
> > > {
> > > struct ocontext *head, *c;
> > > + bool secsid_init_supported = ebitmap_get_bit(&p->policycaps,
> > > + POLICYDB_CAP_USERSPACE_INITIAL_CONTEXT);
> >
> > This is another "please don't respin for this", but if you have to
> > respin for any reason can you change the variable name to
> > "isid_init_supported" or something similar? The "secsid" portion of
> > the name looks wrong to me.
>
> It was supposed to be "secinitsid_init_supported" but I botched it :)
> Though that name is very long, so if I were to change it, I would go
> with your suggestion.
:)
Since we've got a couple of weeks (-rc7 + merge window), why not go
ahead and do the respin to fix up those small things and simplify the
policycap accessor (see the other patch I posted) - does that sound
reasonable?
--
paul-moore.com
