[PATCH] selinux: move user accesses in selinuxfs out of locked regions

[PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Jann Horn]

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>
---
 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

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Paul Moore]

On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <jannh@google.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@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn <jannh@google.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

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Jann Horn]

On Tue, Jun 26, 2018 at 12:36 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <jannh@google.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?

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.

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Paul Moore]

On Mon, Jun 25, 2018 at 6:40 PM Jann Horn <jannh@google.com> wrote:
>
> On Tue, Jun 26, 2018 at 12:36 AM Paul Moore <paul@paul-moore.com> wrote:
> >
> > On Mon, Jun 25, 2018 at 12:34 PM Jann Horn <jannh@google.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?
>
> 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
www.paul-moore.com

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Stephen Smalley]

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: stable@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn <jannh@google.com>

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 <sds@tycho.nsa.gov>

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

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Jann Horn]

On Tue, Jun 26, 2018 at 2:15 PM Stephen Smalley <sds@tycho.nsa.gov> 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: stable@vger.kernel.org
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> 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 <sds@tycho.nsa.gov>

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

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Stephen Smalley]

On 06/26/2018 08:42 AM, Jann Horn wrote:
> On Tue, Jun 26, 2018 at 2:15 PM Stephen Smalley <sds@tycho.nsa.gov> 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: stable@vger.kernel.org
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Signed-off-by: Jann Horn <jannh@google.com>
>>
>> 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 <sds@tycho.nsa.gov>
> 
> 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;
>>>
>>

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Paul Moore]

On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <sds@tycho.nsa.gov> 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: stable@vger.kernel.org
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn <jannh@google.com>
>
> 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 <sds@tycho.nsa.gov>

-- 
paul moore
www.paul-moore.com

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Paul Moore]

On Thu, Jun 28, 2018 at 8:23 PM Paul Moore <paul@paul-moore.com> wrote:
> On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <sds@tycho.nsa.gov> 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: stable@vger.kernel.org
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Jann Horn <jannh@google.com>
> >
> > 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
www.paul-moore.com

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Paul Moore]

On Thu, Jun 28, 2018 at 8:38 PM Paul Moore <paul@paul-moore.com> wrote:
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <sds@tycho.nsa.gov> 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: stable@vger.kernel.org
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > >
> > > 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
www.paul-moore.com

Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions

[Jann Horn]

On Fri, Jun 29, 2018 at 2:38 AM Paul Moore <paul@paul-moore.com> wrote:
>
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore <paul@paul-moore.com> wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley <sds@tycho.nsa.gov> 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: stable@vger.kernel.org
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn <jannh@google.com>
> > >
> > > 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.