On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote: > On Wed, Nov 9, 2016 at 11:25 AM, David Graziano > <david.graziano@xxxxxxxxxxxxxxxxxxx> wrote: >> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <paul@xxxxxxxxxxxxxx> wrote: >>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano >>> <david.graziano@xxxxxxxxxxxxxxxxxxx> wrote: >>>> This patch adds support for generic extended attributes within the >>>> POSIX message queues filesystem and setting them by consulting the LSM. >>>> This is needed so that the security.selinux extended attribute can be >>>> set via a SELinux named type transition on file inodes created within >>>> the filesystem. The implementation and LSM call back function are based >>>> off tmpfs/shmem. >>>> >>>> Signed-off-by: David Graziano <david.graziano@xxxxxxxxxxxxxxxxxxx> >>>> --- >>>> ipc/mqueue.c | 46 ++++++++++++++++++++++++++++++++++++++++++++++ >>>> 1 file changed, 46 insertions(+) >>> >>> Hi David, >>> >>> At first glance this looks reasonable to me, I just have a two >>> questions/comments: >>> >>> * Can you explain your current need for this functionality? For >>> example, what are you trying to do that is made easier by allowing >>> greater message queue labeling flexibility? This helps put things in >>> context and helps people review and comment on your patch. >>> >>> * How have you tested this? While this patch is not SELinux specific, >>> I think adding a test to the selinux-testsuite[1] would be worthwhile. >>> The other LSM maintainers may suggest something similar if they have >>> an established public testsuite. >>> >>> [1] https://github.com/SELinuxProject/selinux-testsuite >> >> Hi Paul, >> >> I needed to write a selinux policy for a set of custom applications that use >> POSIX message queues for their IPC. The queues are created by one >> application and we needed a way for selinux to enforce which of the other >> apps are able to read/write to each individual queue. Uniquely labeling them >> based on the app that created them and the file name seemed to be our best >> solution since it’s an embedded system and we don’t have restorecond to >> handle any relabeling. > > In the future putting things like the above in the patch description > can be helpful. In other words, instead of simply saying this allows > you to better control the labels assigned to message queues, you could > expand upon it by saying that this patch allows you to better control > which applications have access to a given queue. Yes, I realize that > is implied by better control over the labels, but being explicit is > rarely a bad thing when it comes to patch descriptions. > > I've never rejected a patch for a description that was too lengthy, > but I have rejected patches that need better descriptions ;) > >> To test this patch I used both a selinux enabled, buildroot based qemu target >> with a customized selinux policy and test C app to create the mqueues. I also >> tested with our real apps and selinux policy on our target hardware. I can >> certainly look at adding a test to the selinux-testsuite if that would >> be helpful. > > Please do. I've been requiring tests for all new SELinux > functionality lately; this isn't strictly a SELinux patch but I think > it is a good practice regardless. Sorry for the delay. I have created a pull request within the selinux-testsuite github project with a set of mqueue tests. > >>>> diff --git a/ipc/mqueue.c b/ipc/mqueue.c >>>> index 0b13ace..512a546 100644 >>>> --- a/ipc/mqueue.c >>>> +++ b/ipc/mqueue.c >>>> @@ -35,6 +35,7 @@ >>>> #include <linux/ipc_namespace.h> >>>> #include <linux/user_namespace.h> >>>> #include <linux/slab.h> >>>> +#include <linux/xattr.h> >>>> >>>> #include <net/sock.h> >>>> #include "util.h" >>>> @@ -70,6 +71,7 @@ struct mqueue_inode_info { >>>> struct rb_root msg_tree; >>>> struct posix_msg_tree_node *node_cache; >>>> struct mq_attr attr; >>>> + struct simple_xattrs xattrs; /* list of xattrs */ >>>> >>>> struct sigevent notify; >>>> struct pid *notify_owner; >>>> @@ -254,6 +256,7 @@ static struct inode *mqueue_get_inode(struct super_block *sb, >>>> info->attr.mq_maxmsg = attr->mq_maxmsg; >>>> info->attr.mq_msgsize = attr->mq_msgsize; >>>> } >>>> + simple_xattrs_init(&info->xattrs); >>>> /* >>>> * We used to allocate a static array of pointers and account >>>> * the size of that array as well as one msg_msg struct per >>>> @@ -413,6 +416,41 @@ static void mqueue_evict_inode(struct inode *inode) >>>> put_ipc_ns(ipc_ns); >>>> } >>>> >>>> +/* >>>> + * Callback for security_inode_init_security() for acquiring xattrs. >>>> + */ >>>> +static int mqueue_initxattrs(struct inode *inode, >>>> + const struct xattr *xattr_array, >>>> + void *fs_info) >>>> +{ >>>> + struct mqueue_inode_info *info = MQUEUE_I(inode); >>>> + const struct xattr *xattr; >>>> + struct simple_xattr *new_xattr; >>>> + size_t len; >>>> + >>>> + for (xattr = xattr_array; xattr->name != NULL; xattr++) { >>>> + new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len); >>>> + if (!new_xattr) >>>> + return -ENOMEM; >>>> + len = strlen(xattr->name) + 1; >>>> + new_xattr->name = kmalloc(XATTR_SECURITY_PREFIX_LEN + len, >>>> + GFP_KERNEL); >>>> + if (!new_xattr->name) { >>>> + kfree(new_xattr); >>>> + return -ENOMEM; >>>> + } >>>> + >>>> + memcpy(new_xattr->name, XATTR_SECURITY_PREFIX, >>>> + XATTR_SECURITY_PREFIX_LEN); >>>> + memcpy(new_xattr->name + XATTR_SECURITY_PREFIX_LEN, >>>> + xattr->name, len); >>>> + >>>> + simple_xattr_list_add(&info->xattrs, new_xattr); >>>> + } >>>> + >>>> + return 0; >>>> +} >>>> + >>>> static int mqueue_create(struct inode *dir, struct dentry *dentry, >>>> umode_t mode, bool excl) >>>> { >>>> @@ -443,6 +481,14 @@ static int mqueue_create(struct inode *dir, struct dentry *dentry, >>>> ipc_ns->mq_queues_count--; >>>> goto out_unlock; >>>> } >>>> + error = security_inode_init_security(inode, dir, >>>> + &dentry->d_name, >>>> + mqueue_initxattrs, NULL); >>>> + if (error && error != -EOPNOTSUPP) { >>>> + spin_lock(&mq_lock); >>>> + ipc_ns->mq_queues_count--; >>>> + goto out_unlock; >>>> + } >>>> >>>> put_ipc_ns(ipc_ns); >>>> dir->i_size += DIRENT_SIZE; >>>> -- >>>> 1.9.1 >>>> >>>> -- >>>> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in >>>> the body of a message to majordomo@xxxxxxxxxxxxxxx >>>> More majordomo info at http://vger.kernel.org/majordomo-info.html >>> >>> -- >>> paul moore >>> www.paul-moore.com > > > > -- > paul moore > www.paul-moore.com _______________________________________________ Selinux mailing list Selinux@xxxxxxxxxxxxx To unsubscribe, send email to Selinux-leave@xxxxxxxxxxxxx. To get help, send an email containing "help" to Selinux-request@xxxxxxxxxxxxx.
