On Tue, 2010-07-27 at 15:28 -0400, Stephen Smalley wrote:
> On Tue, 2010-07-27 at 15:24 -0400, Eric Paris wrote:
> > On Tue, 2010-07-27 at 15:20 -0400, Stephen Smalley wrote:
> > > On Tue, 2010-07-27 at 15:16 -0400, Eric Paris wrote:
> > > > On Tue, 2010-07-27 at 15:09 -0400, Stephen Smalley wrote:
> > > > > On Tue, 2010-07-27 at 14:51 -0400, Eric Paris wrote:
> > > >
> > > > > > My fear was:
> > > > > >
> > > > > > Process A Process B
> > > > > > ------------------------ -------------------------
> > > > > > open("/selinux/policy");
> > > > > > plm->data = p1
> > > > > > plm->len = l1
> > > > > > i_size = l1
> > > > > > load_policy
> > > > > > open("/selinux/policy");
> > > > > > plm->data = p2;
> > > > > > plm->len = l2;
> > > > > > i_size = l2;
> > > > > > stat("/selinux/policy");
> > > > > > we get i_size=l2
> > > > > > stat("/selinux/policy");
> > > > > > we get i_size=l2 WHOOPS.
> > > > >
> > > > > How is that different from corresponding situation for a regular file?
> > > > > Doesn't seem like the sort of thing the kernel should worry about.
> > > > > I thought the check was to prevent arbitrary allocation of kernel memory
> > > > > by spinning in a loop opening /selinux/policy forever. But that doesn't
> > > > > require worrying about the inode size changing.
> > > >
> > > > This is actually more the problem with files in /proc. The difference
> > > > is that the plm->data and plm->len are still p1 and l1. It's just
> > > > stat() that is going to lie to you. So you will either mmap to much or
> > > > too little space and you have no idea how long the data you are going to
> > > > be reading is...
> > > >
> > > > read() works ok, because it is going to go till the EOF, but you have no
> > > > idea where that is with mmap.....
> > >
> > > I see. It appears that setools and coreutils use read() rather than
> > > mmap() since I could run seinfo, sediff, and cp on /selinux/policy, so
> > > maybe nothing other than checkpolicy -b would even use the mmap support?
> > > And that can be worked around by first copying to a regular file.
> > > checkpolicy -b isn't exactly a typical user command; most people don't
> > > even know it exists.
> >
> > Another horrific option would be to create a new inode for each open()
> > so your fd would always have your i_size info....
>
> That would indeed be horrific ;)
>
> > I'm going to finish my second mmap implementation attempt just to learn,
> > but maybe we want to just throw it all away? If so, thoughts on
> > addressing the memory usage issue?
>
> I don't care about userspace getting confused by an interleaving of
> policy load and policy read - that is a userspace problem IMO. So I
> wouldn't be opposed to having the mmap handler upstreamed, although I
> don't feel qualified to review the implementation - might want some
> -fsdevel or -kernel eyes on it.
>
> Limiting /selinux/policy to one user at a time might be reasonable as a
> way of limiting memory consumption. But you don't need the i_size hacks
> as part of that.
My new mmap implementation (a whole lot easier to review even by an
idiot like me) gives:
# checkpolicy -M -b /selinux/policy
checkpolicy: loading policy configuration from /selinux/policy
libsepol.policydb_index_others: security: 9 users, 13 roles, 3341 types, 170 bools
libsepol.policydb_index_others: security: 1 sens, 1024 cats
libsepol.policydb_index_others: security: 77 classes, 195841 rules, 236706 cond rules
checkpolicy: policy configuration loaded
-----
commit 7e9368a5d130e6caa9b914cc60da7bc3791e04a5
Author: Eric Paris <eparis@xxxxxxxxxx>
Date: Tue Jul 27 16:22:33 2010 -0400
selinux: implement mmap on /selinux/policy
/selinux/policy allows a user to copy the policy back out of the kernel.
This patch allows userspace to actually mmap that file and use it directly.
Signed-off-by: Eric Paris <eparis@xxxxxxxxxx>
diff --git a/security/selinux/selinuxfs.c b/security/selinux/selinuxfs.c
index 8f64eec..6562ecd 100644
--- a/security/selinux/selinuxfs.c
+++ b/security/selinux/selinuxfs.c
@@ -68,6 +68,8 @@ static int *bool_pending_values;
static struct dentry *class_dir;
static unsigned long last_class_ino;
+static char policy_opened;
+
/* global data for policy capabilities */
static struct dentry *policycap_dir;
@@ -315,11 +317,23 @@ static int sel_open_policy(struct inode *inode, struct file *filp)
if (rc)
goto err;
+ if (policy_opened) {
+ mutex_unlock(&sel_mutex);
+ return -EBUSY;
+ }
+ policy_opened = 1;
+
rc = -ENOMEM;
plm = kzalloc(sizeof(*plm), GFP_KERNEL);
if (!plm)
goto err;
+ if (i_size_read(inode) != security_policydb_len()) {
+ mutex_lock(&inode->i_mutex);
+ i_size_write(inode, security_policydb_len());
+ mutex_unlock(&inode->i_mutex);
+ }
+
rc = security_read_policy(&plm->data, &plm->len);
if (rc)
goto err;
@@ -344,6 +358,8 @@ static int sel_release_policy(struct inode *inode, struct file *filp)
BUG_ON(!plm);
+ policy_opened = 0;
+
vfree(plm->data);
kfree(plm);
@@ -380,9 +396,44 @@ out:
return ret;
}
+static int sel_mmap_policy_fault(struct vm_area_struct *vma,
+ struct vm_fault *vmf)
+{
+ struct policy_load_memory *plm = vma->vm_file->private_data;
+ unsigned long offset;
+ struct page *page;
+
+ if (vmf->flags & (FAULT_FLAG_MKWRITE | FAULT_FLAG_WRITE))
+ return VM_FAULT_SIGBUS;
+
+ offset = vmf->pgoff << PAGE_SHIFT;
+ if (offset >= roundup(plm->len, PAGE_SIZE))
+ return VM_FAULT_SIGBUS;
+
+ page = vmalloc_to_page(plm->data + offset);
+ get_page(page);
+
+ vmf->page = page;
+
+ return 0;
+}
+
+static struct vm_operations_struct sel_mmap_policy_ops = {
+ .fault = sel_mmap_policy_fault,
+};
+
+int sel_mmap_policy(struct file *filp, struct vm_area_struct *vma)
+{
+ vma->vm_flags |= VM_RESERVED;
+ vma->vm_ops = &sel_mmap_policy_ops;
+
+ return 0;
+}
+
static const struct file_operations sel_policy_ops = {
.open = sel_open_policy,
.read = sel_read_policy,
+ .mmap = sel_mmap_policy,
.release = sel_release_policy,
};
--
This message was distributed to subscribers of the selinux mailing list.
If you no longer wish to subscribe, send mail to majordomo@xxxxxxxxxxxxx with
the words "unsubscribe selinux" without quotes as the message.
