| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAHC9VhTipjBx9CF4FdTbc2a8bSqKqWiHfi9Krv=Lu74Yvquyrg@mail.gmail.com>
Date: Mon, 25 Jun 2018 18:35:59 -0400
From: Paul Moore <paul@...l-moore.com>
To: jannh@...gle.com
Cc: Stephen Smalley <sds@...ho.nsa.gov>,
Eric Paris <eparis@...isplace.org>, selinux@...ho.nsa.gov,
security@...nel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <jannh@...gle.com> wrote:
> 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.
Forgive me, I'm thinking about this quickly so I could be very wrong
here, but isn't the mutex needed to prevent problems in multi-threaded
apps hitting the same fd at the same time?
> 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@...r.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn <jannh@...gle.com>
> ---
> security/selinux/selinuxfs.c | 77 ++++++++++++++++--------------------
> 1 file changed, 33 insertions(+), 44 deletions(-)
>
> diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
> index f3d374d2ca04..065f8cea84e3 100644
> --- a/security/selinux/selinuxfs.c
> +++ b/security/selinux/selinuxfs.c
> @@ -445,18 +445,13 @@ static ssize_t sel_read_policy(struct file *filp, char __user *buf,
> 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 +1183,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 +1218,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 +1243,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 +1274,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 +1294,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;
> --
> 2.18.0.rc2.346.g013aa6912e-goog
>
--
paul moore
www.paul-moore.com
Powered by blists - more mailing lists