On Mon, Aug 24, 2020 at 4:33 AM Ondrej Mosnacek <omosnace@xxxxxxxxxx> wrote:
>
> 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.
Wondering if this is worthwhile/necessary versus just having userspace
retry if needed. Reading /sys/fs/selinux/policy is not a common or
frequent operation. By the way, if you have CONFIG_DEBUG_ATOMIC_SLEEP
enabled, it should catch things like this for you. I have
CONFIG_LOCK_DEBUGGING_SUPPORT=y
CONFIG_DEBUG_RT_MUTEXES=y
CONFIG_DEBUG_SPINLOCK=y
CONFIG_DEBUG_MUTEXES=y
CONFIG_DEBUG_WW_MUTEX_SLOWPATH=y
CONFIG_DEBUG_RWSEMS=y
CONFIG_DEBUG_LOCK_ALLOC=y
# CONFIG_DEBUG_LOCKDEP is not set
CONFIG_DEBUG_ATOMIC_SLEEP=y
