On Fri, Aug 21, 2020 at 7:39 PM Stephen Smalley
<stephen.smalley.work@xxxxxxxxx> wrote:
> On Fri, Aug 21, 2020 at 11:47 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.
> >
> > Fix it by doing everything in a single transaction and getting the
> > length directly from policydb instead of calling
> > security_policydb_len().
> >
> > 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 | 19 +++++++++++--------
> > 1 file changed, 11 insertions(+), 8 deletions(-)
> >
> > diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
> > index a48fc1b337ba9..ab4de2a01634a 100644
> > --- a/security/selinux/ss/services.c
> > +++ b/security/selinux/ss/services.c
> > @@ -3842,22 +3842,25 @@ int security_read_policy(struct selinux_state *state,
> > void **data, size_t *len)
> > {
> > int rc;
> > + struct policydb *policydb;
> > struct policy_file fp;
> >
> > if (!selinux_initialized(state))
> > return -EINVAL;
> >
> > - *len = security_policydb_len(state);
> > + read_lock(&state->ss->policy_rwlock);
> > + policydb = &state->ss->policy->policydb;
> >
> > + *len = policydb->len;
> > *data = vmalloc_user(*len);
>
> I don't believe you can hold a read_lock() across a vmalloc. That's
> why this is done the way it is now.
Fair point. Then I guess the only option is to keep retrying the
allocation until the allocated size is >= the size we are about to
write. I'll send a revised patch soon.
>
> > - if (!*data)
> > - return -ENOMEM;
> > -
> > - fp.data = *data;
> > - fp.len = *len;
> > + if (!*data) {
> > + rc = -ENOMEM;
> > + } else {
> > + fp.data = *data;
> > + fp.len = *len;
> >
> > - read_lock(&state->ss->policy_rwlock);
> > - rc = policydb_write(&state->ss->policy->policydb, &fp);
> > + rc = policydb_write(policydb, &fp);
> > + }
> > read_unlock(&state->ss->policy_rwlock);
> >
> > if (rc)
> > --
> > 2.26.2
> >
>
--
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.
