Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans

From: Paul Moore <paul@paul-moore.com>
To: Ondrej Mosnacek <omosnace@redhat.com>
Cc: SElinux list <selinux@vger.kernel.org>,
	Stephen Smalley <stephen.smalley.work@gmail.com>
Subject: Re: [PATCH 1/3] selinux: fix cond_list corruption when changing booleans
Date: Thu, 1 Apr 2021 11:36:23 -0400	[thread overview]
Message-ID: <CAHC9VhQUo0cp_7Zt=ogsj0RknZ-YK7pv40rUevZztF-PJ+99ig@mail.gmail.com> (raw)
In-Reply-To: <CAFqZXNuq2aBs6mW6jZ+bt_SY2xrE4LYL3knEjxD26bOw8J3Eqg@mail.gmail.com>

On Thu, Apr 1, 2021 at 3:59 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Thu, Apr 1, 2021 at 1:20 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Wed, Mar 31, 2021 at 5:12 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> > > On Wed, Mar 31, 2021 at 4:04 AM Paul Moore <paul@paul-moore.com> wrote:
> > > > On Tue, Mar 30, 2021 at 9:16 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > ...
> >
> > > > > -int avtab_duplicate(struct avtab *new, struct avtab *orig)
> > > > > +int avtab_alloc(struct avtab *h, u32 nrules)
> > > > >  {
> > > > > -       int i;
> > > > > -       struct avtab_node *node, *tmp, *tail;
> > > > > -
> > > > > -       memset(new, 0, sizeof(*new));
> > > > > +       int rc;
> > > > > +       u32 shift = 0;
> > > > > +       u32 work = nrules;
> > > > > +       u32 nslot = 0;
> > > > >
> > > > > -       new->htable = kvcalloc(orig->nslot, sizeof(void *), GFP_KERNEL);
> > > > > -       if (!new->htable)
> > > > > -               return -ENOMEM;
> > > > > -       new->nslot = orig->nslot;
> > > > > -       new->mask = orig->mask;
> > > > > -
> > > > > -       for (i = 0; i < orig->nslot; i++) {
> > > > > -               tail = NULL;
> > > > > -               for (node = orig->htable[i]; node; node = node->next) {
> > > > > -                       tmp = kmem_cache_zalloc(avtab_node_cachep, GFP_KERNEL);
> > > > > -                       if (!tmp)
> > > > > -                               goto error;
> > > > > -                       tmp->key = node->key;
> > > > > -                       if (tmp->key.specified & AVTAB_XPERMS) {
> > > > > -                               tmp->datum.u.xperms =
> > > > > -                                       kmem_cache_zalloc(avtab_xperms_cachep,
> > > > > -                                                       GFP_KERNEL);
> > > > > -                               if (!tmp->datum.u.xperms) {
> > > > > -                                       kmem_cache_free(avtab_node_cachep, tmp);
> > > > > -                                       goto error;
> > > > > -                               }
> > > > > -                               tmp->datum.u.xperms = node->datum.u.xperms;
> > > > > -                       } else
> > > > > -                               tmp->datum.u.data = node->datum.u.data;
> > > > > -
> > > > > -                       if (tail)
> > > > > -                               tail->next = tmp;
> > > > > -                       else
> > > > > -                               new->htable[i] = tmp;
> > > > > -
> > > > > -                       tail = tmp;
> > > > > -                       new->nel++;
> > > > > +       if (nrules != 0) {
> > > > > +               while (work) {
> > > > > +                       work  = work >> 1;
> > > >
> > > > Extra  horizontal  spaces  are  awkward  and  bad.
> > > >
> > > > > +                       shift++;
> > > > >                 }
> > > > > +               if (shift > 2)
> > > > > +                       shift = shift - 2;
> > > >
> > > > Since we are getting nit-picky with this code, why not make the
> > > > loop/if a bit more elegant?  How about something like below?
> > > >
> > > >   u32 shift = 2;
> > > >   u32 work = nrules >> 4;
> > > >   while (work) {
> > > >     work >>= 1;
> > > >     shift++;
> > > >   }
> > >
> > > I think you meant:
> > > u32 shift = 0;
> > > u32 work = nrules >> 2;
> > > while (work) {
> > >     work >>= 1;
> > >     shift++;
> > > }
> > >
> > > ...which is equivalent to the current code and yes, I'll use that :)
> >
> > Well, no, not really, but that's okay as looking at it now we both got
> > it wrong :)
> >
> > Basically I wanted to avoid the odd problem where the current code has
> > a dip in the number of slots/buckets when nrules is equal to 4, 5, 6,
> > or 7.  While the code I proposed yesterday did that, it inflated the
> > number of buckets beyond the current code; your suggestion had
> > problems with low numbers resulting in zero buckets.
>
> Aah, I wrongly parsed the "if (shift > 2) shift -= 2;" statement...
> Yeah, I guess even the original intent was different than what the
> code does.

I think we all mis-interpreted that bit of code at some point.  I
seriously doubt that dip was what the original author intended :)

> Anyway, my code doesn't result in zero buckets, at worst in 1 bucket
> (nslot = 1 << shift) ...

Sorry, yes, I mis-spoke (mis-typed?); I was talking about the
shift/exponent value.

> So I'll argue that my proposed solution is actually slightly better
> (and avoids adding a new magic number).

The magic number argument isn't really valid as both approaches use
them to some degree.  Creating a #define constant is overkill here,
but I guess a short comment wouldn't be a bad idea if you wanted to
add one; I'm not going to require it in this case.

Since we are at -rc5 I really want to wrap this up soon so I'm going
to make one final suggestion:

  shift = 1;
  work = nrules >> 3;
  while (work) {
    work >>= 1;
    shift++;
  }

... this preserves the original nslot count, minus the bump/dip seen
with low rule numbers.  The shift value starts at 1, increases to 2
when the rules reach 8, increases to 3 when the rules reach 16, and so
on.  Of the approaches we've discussed, I believe it is the most
faithful to original intent.

-- 
paul moore
www.paul-moore.com