[PATCH 0/2] Fix race conditions when reading out policy data

[PATCH 0/2] Fix race conditions when reading out policy data

[Ondrej Mosnacek]

v2:
- avoid calling vmalloc_user() under read lock
- add one more patch that fixes another related race condition

v1: https://lore.kernel.org/selinux/CAFqZXNvdtpxveqesYMz3ZxoWGd_vi5euqy6c9gzhmdkKgbU-Fg@mail.gmail.com/T/

Ondrej Mosnacek (2):
  selinux: fix a race condition in security_read_policy()
  selinux: fix a race condition in sel_open_policy()

 security/selinux/include/security.h |  1 -
 security/selinux/selinuxfs.c        | 12 ++++++------
 security/selinux/ss/services.c      | 28 ++++++++++++----------------
 3 files changed, 18 insertions(+), 23 deletions(-)

-- 
2.26.2

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

[Ondrej Mosnacek]

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);
 
-- 
2.26.2

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

[Stephen Smalley]

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.

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

[Stephen Smalley]

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.

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

[Ondrej Mosnacek]

On Mon, Aug 24, 2020 at 2:53 PM Stephen Smalley
<stephen.smalley.work@gmail.com> wrote:
> 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.

Oh, you're right, there is really no race condition thanks to the
mutex... So this patch isn't necessary then, but the second one should
still count as a simplification (with an updated commit message). I'm
fine with merging it into the RCU patch(es) if you want to do it. Or
I'll just send a rebased version if they get merged in the current
form.

-- 
Ondrej Mosnacek
Software Engineer, Platform Security - SELinux kernel
Red Hat, Inc.

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

[Paul Moore]

On Mon, Aug 24, 2020 at 9:04 AM Ondrej Mosnacek <omosnace@redhat.com> wrote:
> Oh, you're right, there is really no race condition thanks to the
> mutex... So this patch isn't necessary then, but the second one should
> still count as a simplification (with an updated commit message). I'm
> fine with merging it into the RCU patch(es) if you want to do it. Or
> I'll just send a rebased version if they get merged in the current
> form.

Yes, I think we can drop this patch, but as you point out, I think
patch 2/2 would still be nice to have even if it isn't strictly
necessary.  If you rebase the patch on top of selinux/next and update
the description I'll happily merge it.

-- 
paul moore
www.paul-moore.com

[PATCH 2/2] selinux: fix a race condition in sel_open_policy()

[Ondrej Mosnacek]

The code to update the policy inode size is racy and inefficient. Move
it below the security_read_policy() call where we already know the
length of the policy we are returning.

Since after this, security_policydb_len() is only called from
security_load_policy(), remove it and just open-code it there.

Fixes: cee74f47a6ba ("SELinux: allow userspace to read policy back out of the kernel")
Signed-off-by: Ondrej Mosnacek <omosnace@redhat.com>
---
 security/selinux/include/security.h |  1 -
 security/selinux/selinuxfs.c        | 12 ++++++------
 security/selinux/ss/services.c      | 18 +++---------------
 3 files changed, 9 insertions(+), 22 deletions(-)

diff --git a/security/selinux/include/security.h b/security/selinux/include/security.h
index c68ed2beadff4..2c14d4165d688 100644
--- a/security/selinux/include/security.h
+++ b/security/selinux/include/security.h
@@ -219,7 +219,6 @@ void selinux_policy_cancel(struct selinux_state *state,
 			struct selinux_policy *policy);
 int security_read_policy(struct selinux_state *state,
 			 void **data, size_t *len);
-size_t security_policydb_len(struct selinux_state *state);
 
 int security_policycap_supported(struct selinux_state *state,
 				 unsigned int req_cap);
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 131816878e503..098d012cf40d8 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -403,16 +403,16 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
 	if (!plm)
 		goto err;
 
-	if (i_size_read(inode) != security_policydb_len(state)) {
-		inode_lock(inode);
-		i_size_write(inode, security_policydb_len(state));
-		inode_unlock(inode);
-	}
-
 	rc = security_read_policy(state, &plm->data, &plm->len);
 	if (rc)
 		goto err;
 
+	if ((size_t)i_size_read(inode) != plm->len) {
+		inode_lock(inode);
+		i_size_write(inode, plm->len);
+		inode_unlock(inode);
+	}
+
 	fsi->policy_opened = 1;
 
 	filp->private_data = plm;
diff --git a/security/selinux/ss/services.c b/security/selinux/ss/services.c
index 2c9072f095985..0745d4f3a5765 100644
--- a/security/selinux/ss/services.c
+++ b/security/selinux/ss/services.c
@@ -2289,20 +2289,6 @@ err:
 	return rc;
 }
 
-size_t security_policydb_len(struct selinux_state *state)
-{
-	size_t len;
-
-	if (!selinux_initialized(state))
-		return 0;
-
-	read_lock(&state->ss->policy_rwlock);
-	len = state->ss->policy->policydb.len;
-	read_unlock(&state->ss->policy_rwlock);
-
-	return len;
-}
-
 /**
  * security_port_sid - Obtain the SID for a port.
  * @protocol: protocol number
@@ -3847,7 +3833,9 @@ int security_read_policy(struct selinux_state *state,
 	if (!selinux_initialized(state))
 		return -EINVAL;
 
-	*len = security_policydb_len(state);
+	read_lock(&state->ss->policy_rwlock);
+	*len = state->ss->policy->policydb.len;
+	read_unlock(&state->ss->policy_rwlock);
 
 again:
 	*data = vmalloc_user(*len);
-- 
2.26.2