Re: [PATCH 1/2] selinux: fix a race condition in security_read_policy()

[Stephen Smalley <stephen.smalley.work@gmail.com>]

On Mon, Aug 24, 2020 at 8:47 AM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
>
> On Mon, Aug 24, 2020 at 7:30 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> >
> > In security_read_policy(), the policy length is computed using
> > security_policydb_len(), which does a separate transaction, and then
> > another transaction is done to write the policydb into a buffer of this
> > length.
> >
> > The bug is that the policy might be re-loaded in between the two
> > transactions and so the length can be wrong. In case the new length is
> > lower than the old length, the length is corrected at the end of the
> > function. In case the new length is higher than the old one, an error is
> > returned.
> >
> > Since we can't call vmalloc_user() under read_lock(), fix it by checking
> > if the allocated buffer is sufficiently large after doing the allocation
> > and taking the read lock and if not, retry the whole operation with the
> > new size.
> >
> > Fixes: cee74f47a6ba ("SELinux: allow userspace to read policy back out of the kernel")
> > Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
> > ---
> >  security/selinux/ss/services.c | 10 +++++++++-
> >  1 file changed, 9 insertions(+), 1 deletion(-)
> >
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index a48fc1b337ba9..2c9072f095985 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -3849,14 +3849,22 @@ int security_read_policy(struct selinux_state *state,
> >
> >         *len = security_policydb_len(state);
> >
> > +again:
> >         *data = vmalloc_user(*len);
> >         if (!*data)
> >                 return -ENOMEM;
> >
> > +       read_lock(&state->ss->policy_rwlock);
> > +       if (*len < state->ss->policy->policydb.len) {
> > +               *len = state->ss->policy->policydb.len;
> > +               read_unlock(&state->ss->policy_rwlock);
> > +               vfree(*data);
> > +               goto again;
> > +       }
> > +
> >         fp.data = *data;
> >         fp.len = *len;
> >
> > -       read_lock(&state->ss->policy_rwlock);
> >         rc = policydb_write(&state->ss->policy->policydb, &fp);
> >         read_unlock(&state->ss->policy_rwlock);
> >
>
> security_read_policy() is called with fsi->mutex held by selinuxfs, so
> policy reload cannot occur in between the length computation and the
> writing of the policydb.  Right?  It's another case where we could
> pass down the mutex as in my rcu patches for a lockdep assertion.

If my RCU patches are merged, we could modify security_read_policy()
to take the mutex too and use rcu_dereference_protected() there,
likewise getting rid of the separate security_policydb_len().  Or I
could re-spin them to do that if any other changes are needed.
Waiting to see if Paul wants any changes to either of those.