| Message ID | 20200821154704.1214330-1-omosnace@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | selinux: fix a race condition in security_read_policy() | expand |
On Fri, Aug 21, 2020 at 11:47 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. > > 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@redhat.com> > --- > 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. > - 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 >
On Fri, Aug 21, 2020 at 7:39 PM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > On Fri, Aug 21, 2020 at 11:47 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. > > > > 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@redhat.com> > > --- > > 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.
On Mon, Aug 24, 2020 at 4:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > On Fri, Aug 21, 2020 at 7:39 PM Stephen Smalley > <stephen.smalley.work@gmail.com> wrote: > > On Fri, Aug 21, 2020 at 11:47 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. > > > > > > 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@redhat.com> > > > --- > > > 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
On Mon, Aug 24, 2020 at 8:40 AM Stephen Smalley <stephen.smalley.work@gmail.com> wrote: > > On Mon, Aug 24, 2020 at 4:33 AM Ondrej Mosnacek <omosnace@redhat.com> wrote: > > > > On Fri, Aug 21, 2020 at 7:39 PM Stephen Smalley > > <stephen.smalley.work@gmail.com> wrote: > > > On Fri, Aug 21, 2020 at 11:47 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. > > > > > > > > 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@redhat.com> > > > > --- > > > > 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. I tend to agree. If the kernel is under memory pressure, continually hammering it with requests seems like the last thing we want to do; not to mention that we would need to bound the requests at some point so we don't potentially hang userspace (or worse). Failing and letting userspace rety seems like the best option to me.
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); - 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)
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@redhat.com> --- security/selinux/ss/services.c | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-)