Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Fri, Jun 29, 2018 at 2:38 AM Paul Moore wrote:
>
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley 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: sta...@vger.kernel.org
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn
> > >
> > > 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.
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Thu, Jun 28, 2018 at 8:38 PM Paul Moore wrote:
> On Thu, Jun 28, 2018 at 8:23 PM Paul Moore wrote:
> > On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley 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: sta...@vger.kernel.org
> > > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > > Signed-off-by: Jann Horn
> > >
> > > 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
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Thu, Jun 28, 2018 at 8:23 PM Paul Moore wrote:
> On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley 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: sta...@vger.kernel.org
> > > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > > Signed-off-by: Jann Horn
> >
> > 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
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Tue, Jun 26, 2018 at 8:15 AM Stephen Smalley 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: sta...@vger.kernel.org
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn
>
> 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
--
paul moore
www.paul-moore.com
___
Selinux mailing list
Selinux@tycho.nsa.gov
To unsubscribe, send email to selinux-le...@tycho.nsa.gov.
To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Mon, Jun 25, 2018 at 6:40 PM Jann Horn wrote: > > On Tue, Jun 26, 2018 at 12:36 AM Paul Moore wrote: > > > > On Mon, Jun 25, 2018 at 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. > > > > 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 ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On 06/26/2018 08:42 AM, Jann Horn wrote:
> On Tue, Jun 26, 2018 at 2:15 PM Stephen Smalley 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: sta...@vger.kernel.org
>>> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
>>> Signed-off-by: Jann Horn
>>
>> 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
>
> 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(>mutex);
>>> -
>>> ret = avc_has_perm(_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(>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(>mutex);
>>> + ret = simple_read_from_buffer(buf, count, ppos, page, length);
>>> +out_free:
>>> free_page((unsigned long)page);
>>> return ret;
>>> +
>>> +out_unlock:
>>> + mutex_unlock(>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(>mutex);
>>>
>>> length = avc_has_perm(_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 =
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Tue, Jun 26, 2018 at 2:15 PM Stephen Smalley 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: sta...@vger.kernel.org
> > Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> > Signed-off-by: Jann Horn
>
> 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
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(>mutex);
> > -
> > ret = avc_has_perm(_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(>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(>mutex);
> > + ret = simple_read_from_buffer(buf, count, ppos, page, length);
> > +out_free:
> > free_page((unsigned long)page);
> > return ret;
> > +
> > +out_unlock:
> > + mutex_unlock(>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(>mutex);
> >
> > length = avc_has_perm(_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;
> > - }
> > -
> >
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
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: sta...@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn
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
> ---
> 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(>mutex);
> -
> ret = avc_has_perm(_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(>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(>mutex);
> + ret = simple_read_from_buffer(buf, count, ppos, page, length);
> +out_free:
> free_page((unsigned long)page);
> return ret;
> +
> +out_unlock:
> + mutex_unlock(>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(>mutex);
>
> length = avc_has_perm(_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", _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(>mutex);
>
> length = avc_has_perm(_state,
> @@ -1289,22 +1294,6 @@ static ssize_t
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Tue, Jun 26, 2018 at 12:36 AM Paul Moore wrote: > > On Mon, Jun 25, 2018 at 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. > > 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. ___ Selinux mailing list Selinux@tycho.nsa.gov To unsubscribe, send email to selinux-le...@tycho.nsa.gov. To get help, send an email containing "help" to selinux-requ...@tycho.nsa.gov.
Re: [PATCH] selinux: move user accesses in selinuxfs out of locked regions
On Mon, Jun 25, 2018 at 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.
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: sta...@vger.kernel.org
> Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
> Signed-off-by: Jann Horn
> ---
> 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(>mutex);
> -
> ret = avc_has_perm(_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(>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(>mutex);
> + ret = simple_read_from_buffer(buf, count, ppos, page, length);
> +out_free:
> free_page((unsigned long)page);
> return ret;
> +
> +out_unlock:
> + mutex_unlock(>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(>mutex);
>
> length = avc_has_perm(_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", _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(>mutex);
>
> length =
