selinux: move user accesses in selinuxfs out of locked regions

If a user is accessing a file in selinuxfs with a pointer to a userspace
buffer that is backed by e.g. a userfaultfd, the userspace access can
stall indefinitely, which can block fsi->mutex if it is held.

For sel_read_policy(), remove the locking, since this method doesn't seem
to access anything that requires locking.

For sel_read_bool(), move the user access below the locked region.

For sel_write_bool() and sel_commit_bools_write(), move the user access
up above the locked region.

Cc: stable@vger.kernel.org
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn <jannh@google.com>
Acked-by: Stephen Smalley <sds@tycho.nsa.gov>
[PM: removed an unused variable in sel_read_policy()]
Signed-off-by: Paul Moore <paul@paul-moore.com>

diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index c0cadbc5f85cd23fbb37ee20f2d88dba3e97f779..19e35dd695d7a4efda74b270a00ceb1e074ab7cc 100644 (file)
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -441,22 +441,16 @@ static int sel_release_policy(struct inode *inode, struct file *filp)
 static ssize_t sel_read_policy(struct file *filp, char __user *buf,
                               size_t count, loff_t *ppos)
 {
-       struct selinux_fs_info *fsi = file_inode(filp)->i_sb->s_fs_info;
        struct policy_load_memory *plm = filp->private_data;
        int ret;
 
-       mutex_lock(&fsi->mutex);
-
        ret = avc_has_perm(&selinux_state,
                           current_sid(), SECINITSID_SECURITY,
                          SECCLASS_SECURITY, SECURITY__READ_POLICY, NULL);
        if (ret)
-               goto out;
+               return ret;
 
-       ret = simple_read_from_buffer(buf, count, ppos, plm->data, plm->len);
-out:
-       mutex_unlock(&fsi->mutex);
-       return ret;
+       return simple_read_from_buffer(buf, count, ppos, plm->data, plm->len);
 }
 
 static vm_fault_t sel_mmap_policy_fault(struct vm_fault *vmf)
@@ -1188,25 +1182,29 @@ static ssize_t sel_read_bool(struct file *filep, char __user *buf,
        ret = -EINVAL;
        if (index >= fsi->bool_num || strcmp(name,
                                             fsi->bool_pending_names[index]))
-               goto out;
+               goto out_unlock;
 
        ret = -ENOMEM;
        page = (char *)get_zeroed_page(GFP_KERNEL);
        if (!page)
-               goto out;
+               goto out_unlock;
 
        cur_enforcing = security_get_bool_value(fsi->state, index);
        if (cur_enforcing < 0) {
                ret = cur_enforcing;
-               goto out;
+               goto out_unlock;
        }
        length = scnprintf(page, PAGE_SIZE, "%d %d", cur_enforcing,
                          fsi->bool_pending_values[index]);
-       ret = simple_read_from_buffer(buf, count, ppos, page, length);
-out:
        mutex_unlock(&fsi->mutex);
+       ret = simple_read_from_buffer(buf, count, ppos, page, length);
+out_free:
        free_page((unsigned long)page);
        return ret;
+
+out_unlock:
+       mutex_unlock(&fsi->mutex);
+       goto out_free;
 }
 
 static ssize_t sel_write_bool(struct file *filep, const char __user *buf,
@@ -1219,6 +1217,17 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf,
        unsigned index = file_inode(filep)->i_ino & SEL_INO_MASK;
        const char *name = filep->f_path.dentry->d_name.name;
 
+       if (count >= PAGE_SIZE)
+               return -ENOMEM;
+
+       /* No partial writes. */
+       if (*ppos != 0)
+               return -EINVAL;
+
+       page = memdup_user_nul(buf, count);
+       if (IS_ERR(page))
+               return PTR_ERR(page);
+
        mutex_lock(&fsi->mutex);
 
        length = avc_has_perm(&selinux_state,
@@ -1233,22 +1242,6 @@ static ssize_t sel_write_bool(struct file *filep, const char __user *buf,
                                             fsi->bool_pending_names[index]))
                goto out;
 
-       length = -ENOMEM;
-       if (count >= PAGE_SIZE)
-               goto out;
-
-       /* No partial writes. */
-       length = -EINVAL;
-       if (*ppos != 0)
-               goto out;
-
-       page = memdup_user_nul(buf, count);
-       if (IS_ERR(page)) {
-               length = PTR_ERR(page);
-               page = NULL;
-               goto out;
-       }
-
        length = -EINVAL;
        if (sscanf(page, "%d", &new_value) != 1)
                goto out;
@@ -1280,6 +1273,17 @@ static ssize_t sel_commit_bools_write(struct file *filep,
        ssize_t length;
        int new_value;
 
+       if (count >= PAGE_SIZE)
+               return -ENOMEM;
+
+       /* No partial writes. */
+       if (*ppos != 0)
+               return -EINVAL;
+
+       page = memdup_user_nul(buf, count);
+       if (IS_ERR(page))
+               return PTR_ERR(page);
+
        mutex_lock(&fsi->mutex);
 
        length = avc_has_perm(&selinux_state,
@@ -1289,22 +1293,6 @@ static ssize_t sel_commit_bools_write(struct file *filep,
        if (length)
                goto out;
 
-       length = -ENOMEM;
-       if (count >= PAGE_SIZE)
-               goto out;
-
-       /* No partial writes. */
-       length = -EINVAL;
-       if (*ppos != 0)
-               goto out;
-
-       page = memdup_user_nul(buf, count);
-       if (IS_ERR(page)) {
-               length = PTR_ERR(page);
-               page = NULL;
-               goto out;
-       }
-
        length = -EINVAL;
        if (sscanf(page, "%d", &new_value) != 1)
                goto out;