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.
> (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
