On Mon, Aug 24, 2020 at 8:47 AM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
>
> On Mon, Aug 24, 2020 at 7:30 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> > 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.
