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: [email protected]
Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Signed-off-by: Jann Horn <[email protected]>
---
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
On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <[email protected]> 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: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn <[email protected]>
> ---
> 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
http://www.paul-moore.com
On Tue, Jun 26, 2018 at 12:36 AM Paul Moore <[email protected]> wrote:
>
> On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <[email protected]> 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?
sel_read_policy() operates on a read-only copy of the policy, accessed
via ->private_data, allocated using vmalloc in sel_open_policy() via
security_read_policy(). As far as I can tell, nothing can write to
that read-only copy of the policy. None of the handlers in
sel_policy_ops write - they just mmap as readonly (in which case
you're already reading without locks, by the way) or read.
On 06/25/2018 12:34 PM, Jann Horn 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.
>
> 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: [email protected]
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn <[email protected]>
Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
do we need it to be back-ported any further than the commit which introduced it.
Otherwise, you can add my
Acked-by: Stephen Smalley <[email protected]>
> ---
> 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;
>
On Tue, Jun 26, 2018 at 2:15 PM Stephen Smalley <[email protected]> wrote:
>
> On 06/25/2018 12:34 PM, Jann Horn 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.
> >
> > 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: [email protected]
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn <[email protected]>
>
> Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
> do we need it to be back-ported any further than the commit which introduced it.
You can also use FUSE, if the system is configured appropriately:
Mount a FUSE filesystem, mmap() a file from it, then pass a pointer to
the mmapped region to a syscall. AFAICS FUSE was added to the kernel
in commit d8a5ba45457e4a22aa39c939121efd7bb6c76672, first in
v2.6.16.28.
> Otherwise, you can add my
> Acked-by: Stephen Smalley <[email protected]>
This patch should go through Paul Moore's tree, right?
> > ---
> > 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;
> >
>
On 06/26/2018 08:42 AM, Jann Horn wrote:
> On Tue, Jun 26, 2018 at 2:15 PM Stephen Smalley <[email protected]> wrote:
>>
>> On 06/25/2018 12:34 PM, Jann Horn 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.
>>>
>>> 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: [email protected]
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Signed-off-by: Jann Horn <[email protected]>
>>
>> Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
>> do we need it to be back-ported any further than the commit which introduced it.
>
> You can also use FUSE, if the system is configured appropriately:
> Mount a FUSE filesystem, mmap() a file from it, then pass a pointer to
> the mmapped region to a syscall. AFAICS FUSE was added to the kernel
> in commit d8a5ba45457e4a22aa39c939121efd7bb6c76672, first in
> v2.6.16.28.
Ok, then I guess it would be splitting hairs to not just take it all the way back.
>
>> Otherwise, you can add my
>> Acked-by: Stephen Smalley <[email protected]>
>
> This patch should go through Paul Moore's tree, right?
Yes, thanks.
>
>>> ---
>>> 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;
>>>
>>
On Mon, Jun 25, 2018 at 6:40 PM Jann Horn <[email protected]> wrote:
>
> On Tue, Jun 26, 2018 at 12:36 AM Paul Moore <[email protected]> wrote:
> >
> > On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <[email protected]> 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?
>
> sel_read_policy() operates on a read-only copy of the policy, accessed
> via ->private_data, allocated using vmalloc in sel_open_policy() via
> security_read_policy(). As far as I can tell, nothing can write to
> that read-only copy of the policy. None of the handlers in
> sel_policy_ops write - they just mmap as readonly (in which case
> you're already reading without locks, by the way) or read.
Great, thanks.
--
paul moore
http://www.paul-moore.com
On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <[email protected]> wrote:
> On 06/25/2018 12:34 PM, Jann Horn 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.
> >
> > 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: [email protected]
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn <[email protected]>
>
> Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
> do we need it to be back-ported any further than the commit which introduced it.
Considering we are talking about v2.6.12 I have to wonder if anyone is
bothering with backports for kernels that old. Even the RHEL-5.x
based systems are at least on v2.6.18.
Regardless, I think this is fine to merge as-is; thanks everyone.
> Otherwise, you can add my
> Acked-by: Stephen Smalley <[email protected]>
--
paul moore
http://www.paul-moore.com
On Thu, Jun 28, 2018 at 8:23 PM Paul Moore <[email protected]> wrote:
> On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <[email protected]> wrote:
> > On 06/25/2018 12:34 PM, Jann Horn 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.
> > >
> > > 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: [email protected]
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Jann Horn <[email protected]>
> >
> > Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
> > do we need it to be back-ported any further than the commit which introduced it.
>
> Considering we are talking about v2.6.12 I have to wonder if anyone is
> bothering with backports for kernels that old. Even the RHEL-5.x
> based systems are at least on v2.6.18.
>
> Regardless, I think this is fine to merge as-is; thanks everyone.
FYI, I did have to remove the "fsi" variable from sel_read_policy() to
keep the compiler happy. Please double check to make sure your code
compiles cleanly in the future.
--
paul moore
http://www.paul-moore.com
On Thu, Jun 28, 2018 at 8:38 PM Paul Moore <[email protected]> wrote:
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore <[email protected]> wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <[email protected]> wrote:
> > > On 06/25/2018 12:34 PM, Jann Horn 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.
> > > >
> > > > 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: [email protected]
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn <[email protected]>
> > >
> > > Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
> > > do we need it to be back-ported any further than the commit which introduced it.
> >
> > Considering we are talking about v2.6.12 I have to wonder if anyone is
> > bothering with backports for kernels that old. Even the RHEL-5.x
> > based systems are at least on v2.6.18.
> >
> > Regardless, I think this is fine to merge as-is; thanks everyone.
>
> FYI, I did have to remove the "fsi" variable from sel_read_policy() to
> keep the compiler happy. Please double check to make sure your code
> compiles cleanly in the future.
I realize I didn't specify this above ... I merged this into
selinux/stable-4.18; I'm building a test kernel now and if everything
looks okay I'll send it to Linus tomorrow.
--
paul moore
http://www.paul-moore.com
On Fri, Jun 29, 2018 at 2:38 AM Paul Moore <[email protected]> wrote:
>
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore <[email protected]> wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <[email protected]> wrote:
> > > On 06/25/2018 12:34 PM, Jann Horn 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.
> > > >
> > > > 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: [email protected]
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn <[email protected]>
> > >
> > > Only question I have is wrt the Fixes line, i.e. was this an issue until userfaultfd was introduced, and if not,
> > > do we need it to be back-ported any further than the commit which introduced it.
> >
> > Considering we are talking about v2.6.12 I have to wonder if anyone is
> > bothering with backports for kernels that old. Even the RHEL-5.x
> > based systems are at least on v2.6.18.
> >
> > Regardless, I think this is fine to merge as-is; thanks everyone.
>
> FYI, I did have to remove the "fsi" variable from sel_read_policy() to
> keep the compiler happy. Please double check to make sure your code
> compiles cleanly in the future.
Oof, don't know how I missed that. Sorry, I'll be more careful.