On Tue, Apr 6, 2021 at 9:15 PM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> On Tue, Apr 6, 2021 at 5:02 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
> > On Tue, Apr 6, 2021 at 12:11 AM Paul Moore <paul@xxxxxxxxxxxxxx> wrote:
> > > On Mon, Apr 5, 2021 at 5:11 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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.
Ah, yes, I was blind... somehow I believed it was allocated outside
the retry loop, yet it pretty obviously isn't :) It's a constant-size
allocation though, so it could just be moved outside and even changed
to a GFP_KERNEL allocation. So that'll be an easy fix and even a tiny
improvement.
>
> > (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
Hah, yeah, didn't realize that :)
--
Ondrej Mosnacek
Software Engineer, Linux Security - SELinux kernel
Red Hat, Inc.
