Re: [PATCH] selinux: fix race between old and new sidtab

[Paul Moore <paul@paul-moore.com>]

On Tue, Apr 6, 2021 at 5:02 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> On Tue, Apr 6, 2021 at 12:11 AM Paul Moore <paul@paul-moore.com> wrote:
> > On Mon, Apr 5, 2021 at 5:11 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:

...

> > > @@ -2534,10 +2577,18 @@ int security_netif_sid(struct selinux_state *state,
> > >                 if (!c->sid[0] || !c->sid[1]) {
> > >                         rc = sidtab_context_to_sid(sidtab, &c->context[0],
> > >                                                    &c->sid[0]);
> > > +                       if (rc == -ESTALE) {
> > > +                               rcu_read_unlock();
> > > +                               goto retry;
> > > +                       }
> > >                         if (rc)
> > >                                 goto out;
> > >                         rc = sidtab_context_to_sid(sidtab, &c->context[1],
> > >                                                    &c->sid[1]);
> > > +                       if (rc == -ESTALE) {
> > > +                               rcu_read_unlock();
> > > +                               goto retry;
> > > +                       }
> >
> > If the first sidtab_context_to_sid() ran successfully we are not going
> > to see -ESTALE here, correct?  Assuming yes, perhaps we can drop this
> > -ESTALE check with a comment about how a frozen sidtab will be caught
> > by the call above.
>
> No, nothing really prevents the sidtab from becoming frozen between
> the two calls.

Yes, my mistake, I was focused on the RCU policy copies and not
looking at the spinlock for the freeze state.

> > > @@ -2676,18 +2732,20 @@ int security_get_user_sids(struct selinux_state *state,
> > >         struct sidtab *sidtab;
> > >         struct context *fromcon, usercon;
> > >         u32 *mysids = NULL, *mysids2, sid;
> > > -       u32 mynel = 0, maxnel = SIDS_NEL;
> > > +       u32 i, j, mynel, maxnel = SIDS_NEL;
> > >         struct user_datum *user;
> > >         struct role_datum *role;
> > >         struct ebitmap_node *rnode, *tnode;
> > > -       int rc = 0, i, j;
> > > +       int rc;
> > >
> > >         *sids = NULL;
> > >         *nel = 0;
> > >
> > >         if (!selinux_initialized(state))
> > > -               goto out;
> > > +               return 0;
> > >
> > > +retry:
> > > +       mynel = 0;
> > >         rcu_read_lock();
> > >         policy = rcu_dereference(state->policy);
> > >         policydb = &policy->policydb;
> > > @@ -2723,6 +2781,10 @@ int security_get_user_sids(struct selinux_state *state,
> > >                                 continue;
> > >
> > >                         rc = sidtab_context_to_sid(sidtab, &usercon, &sid);
> > > +                       if (rc == -ESTALE) {
> > > +                               rcu_read_unlock();
> >
> > Don't we need to free and reset 'mysids' here?  'mynel' and 'maxnel'
> > are probably less critical since the -ESTALE condition should happen
> > before they are ever modified, but a constant assignment is pretty
> > cheap so it may make sense to reset those as well.
>
> No, we can keep "mysids" and "maxnel", since they represent an
> automatically growing vector, which we can keep and reuse in the retry
> path rather than starting from scratch. It is more efficient, since
> the new policy will likely have the same number of SIDs in the result.
> If it has more, we just grow the vector further as needed; if it has
> less, we just don't use the full capacity and the array will be freed
> after a while anyway (see "out_unlock"), so the extra memory shouldn't
> be held for too long. Not to mention that this is a deprecated
> interface that will hopefully be removed one day :)

I believe you are ignoring the case where mysids is non-NULL when an
-ESTALE occurs and the code jumps to 'retry' and that ends up calling
'mysids = kcalloc(...)' before the ebitmap loop.  Perhaps I'm
mistaken, but this looks like a memory leak to me.

> (And you're wrong that mynel/maxnel can't be modified - notice that
> the sidtab_context_to_sid() call is inside a loop ;)

My comments were correct if you work under the (faulty) assumption
that the sidtab isn't being frozen underneath you :-P

-- 
paul moore
www.paul-moore.com