| Message ID | 20180625163425.216965-1-jannh@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
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 >
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.
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; >
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; > > >
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; >>> >>
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.
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>
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.
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.
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.
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;
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(-)