Re: [PATCH v4 1/3] xattr: add simple initxattrs function

[David Graziano]

On Sun, Jan 8, 2017 at 3:55 AM, Christoph Hellwig  wrote:
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +int simple_xattr_initxattrs(struct inode *inode,
>> + const struct xattr *xattr_array,
>> + void *fs_info)
>> +{
>> + struct simple_xattrs *xattrs;
>> + const struct xattr *xattr;
>> + struct simple_xattr *new_xattr;
>> + size_t len;
>> +
>> + if (!fs_info)
>> + return -ENOMEM;
>
> This probablt should be an EINVAL, and also a WARN_ON_ONCE.

I will change the return value to -EINVAL and add the WARN_ON_ONCE.
In the next version of the patchset.

>
>> + xattrs = (struct simple_xattrs *) fs_info;
>
> No need for the cast.  In fact we should probably just declarate it
> as struct simple_xattrs *xattrs in the protoype and thus be type safe.

I don't think the prototype can be changed to "struct simple_xattrs" as the
security_inode_init_security() function in security/security.c which calls
this is asumming an initxattrs function with following prototype
int (*initxattrs)  (struct inode *inode, const struct xattr
*xattr_array, void *fs_data)

>
>> +
>> + for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> + new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> + if (!new_xattr)
>> + return -ENOMEM;
>
> We'll need to unwind the previous allocations here.

This patchset essentially relocates the shmem_initxattrs() function from
mm/shmem.c and uses the relocated function for both tmpfs and mqueuefs.
That inital function didn't attempt to unwind the previous allocations. If the
consensus is to unwind any allocations made by this function I can look
at adding it.

Re: [PATCH v4 1/3] xattr: add simple initxattrs function

[David Graziano]

On Sun, Jan 8, 2017 at 3:55 AM, Christoph Hellwig  wrote:
>> +/*
>> + * Callback for security_inode_init_security() for acquiring xattrs.
>> + */
>> +int simple_xattr_initxattrs(struct inode *inode,
>> + const struct xattr *xattr_array,
>> + void *fs_info)
>> +{
>> + struct simple_xattrs *xattrs;
>> + const struct xattr *xattr;
>> + struct simple_xattr *new_xattr;
>> + size_t len;
>> +
>> + if (!fs_info)
>> + return -ENOMEM;
>
> This probablt should be an EINVAL, and also a WARN_ON_ONCE.

I will change the return value to -EINVAL and add the WARN_ON_ONCE.
In the next version of the patchset.

>
>> + xattrs = (struct simple_xattrs *) fs_info;
>
> No need for the cast.  In fact we should probably just declarate it
> as struct simple_xattrs *xattrs in the protoype and thus be type safe.

I don't think the prototype can be changed to "struct simple_xattrs" as the
security_inode_init_security() function in security/security.c which calls
this is asumming an initxattrs function with following prototype
int (*initxattrs)  (struct inode *inode, const struct xattr
*xattr_array, void *fs_data)

>
>> +
>> + for (xattr = xattr_array; xattr->name != NULL; xattr++) {
>> + new_xattr = simple_xattr_alloc(xattr->value, xattr->value_len);
>> + if (!new_xattr)
>> + return -ENOMEM;
>
> We'll need to unwind the previous allocations here.

This patchset essentially relocates the shmem_initxattrs() function from
mm/shmem.c and uses the relocated function for both tmpfs and mqueuefs.
That inital function didn't attempt to unwind the previous allocations. If the
consensus is to unwind any allocations made by this function I can look
at adding it.

[PATCH v4 2/3] shmem: use simple initxattrs callback

[David Graziano]

Updates shmem to use the newly created simple_xattr_initxattrs()
function to minimize code duplication with other LSM callback
functions.

Signed-off-by: David Graziano <david.grazi...@rockwellcollins.com>
---
 mm/shmem.c | 53 -
 1 file changed, 12 insertions(+), 41 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 971fc83..ef4bd52 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static struct vfsmount *shm_mnt;
 
@@ -2140,7 +2141,7 @@ static const struct inode_operations 
shmem_symlink_inode_operations;
 static const struct inode_operations shmem_short_symlink_operations;
 
 #ifdef CONFIG_TMPFS_XATTR
-static int shmem_initxattrs(struct inode *, const struct xattr *, void *);
+#define shmem_initxattrs simple_xattr_initxattrs
 #else
 #define shmem_initxattrs NULL
 #endif
@@ -2892,6 +2893,7 @@ static int
 shmem_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
 {
struct inode *inode;
+   struct shmem_inode_info *info;
int error = -ENOSPC;
 
inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE);
@@ -2899,9 +2901,11 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, 
umode_t mode, dev_t dev)
error = simple_acl_create(dir, inode);
if (error)
goto out_iput;
+   info = SHMEM_I(inode);
error = security_inode_init_security(inode, dir,
 >d_name,
-shmem_initxattrs, NULL);
+shmem_initxattrs,
+>xattrs);
if (error && error != -EOPNOTSUPP)
goto out_iput;
 
@@ -2921,13 +2925,16 @@ static int
 shmem_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
struct inode *inode;
+   struct shmem_inode_info *info;
int error = -ENOSPC;
 
inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE);
if (inode) {
+   info = SHMEM_I(inode);
error = security_inode_init_security(inode, dir,
 NULL,
-shmem_initxattrs, NULL);
+shmem_initxattrs,
+>xattrs);
if (error && error != -EOPNOTSUPP)
goto out_iput;
error = simple_acl_create(dir, inode);
@@ -3119,8 +3126,9 @@ static int shmem_symlink(struct inode *dir, struct dentry 
*dentry, const char *s
if (!inode)
return -ENOSPC;
 
+   info = SHMEM_I(inode);
error = security_inode_init_security(inode, dir, >d_name,
-shmem_initxattrs, NULL);
+shmem_initxattrs, >xattrs);
if (error) {
if (error != -EOPNOTSUPP) {
iput(inode);
@@ -3129,7 +3137,6 @@ static int shmem_symlink(struct inode *dir, struct dentry 
*dentry, const char *s
error = 0;
}
 
-   info = SHMEM_I(inode);
inode->i_size = len-1;
if (len <= SHORT_SYMLINK_LEN) {
inode->i_link = kmemdup(symname, len, GFP_KERNEL);
@@ -3198,42 +3205,6 @@ static const char *shmem_get_link(struct dentry *dentry,
  * filesystem level, though.
  */
 
-/*
- * Callback for security_inode_init_security() for acquiring xattrs.
- */
-static int shmem_initxattrs(struct inode *inode,
-   const struct xattr *xattr_array,
-   void *fs_info)
-{
-   struct shmem_inode_info *info = SHMEM_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(>xattrs, new_xattr);
-   }
-
-   return 0;
-}
-
 static int shmem_xattr_handler_

[PATCH v4 2/3] shmem: use simple initxattrs callback

[David Graziano]

Updates shmem to use the newly created simple_xattr_initxattrs()
function to minimize code duplication with other LSM callback
functions.

Signed-off-by: David Graziano 
---
 mm/shmem.c | 53 -
 1 file changed, 12 insertions(+), 41 deletions(-)

diff --git a/mm/shmem.c b/mm/shmem.c
index 971fc83..ef4bd52 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -33,6 +33,7 @@
 #include 
 #include 
 #include 
+#include 
 
 static struct vfsmount *shm_mnt;
 
@@ -2140,7 +2141,7 @@ static const struct inode_operations 
shmem_symlink_inode_operations;
 static const struct inode_operations shmem_short_symlink_operations;
 
 #ifdef CONFIG_TMPFS_XATTR
-static int shmem_initxattrs(struct inode *, const struct xattr *, void *);
+#define shmem_initxattrs simple_xattr_initxattrs
 #else
 #define shmem_initxattrs NULL
 #endif
@@ -2892,6 +2893,7 @@ static int
 shmem_mknod(struct inode *dir, struct dentry *dentry, umode_t mode, dev_t dev)
 {
struct inode *inode;
+   struct shmem_inode_info *info;
int error = -ENOSPC;
 
inode = shmem_get_inode(dir->i_sb, dir, mode, dev, VM_NORESERVE);
@@ -2899,9 +2901,11 @@ shmem_mknod(struct inode *dir, struct dentry *dentry, 
umode_t mode, dev_t dev)
error = simple_acl_create(dir, inode);
if (error)
goto out_iput;
+   info = SHMEM_I(inode);
error = security_inode_init_security(inode, dir,
 >d_name,
-shmem_initxattrs, NULL);
+shmem_initxattrs,
+>xattrs);
if (error && error != -EOPNOTSUPP)
goto out_iput;
 
@@ -2921,13 +2925,16 @@ static int
 shmem_tmpfile(struct inode *dir, struct dentry *dentry, umode_t mode)
 {
struct inode *inode;
+   struct shmem_inode_info *info;
int error = -ENOSPC;
 
inode = shmem_get_inode(dir->i_sb, dir, mode, 0, VM_NORESERVE);
if (inode) {
+   info = SHMEM_I(inode);
error = security_inode_init_security(inode, dir,
 NULL,
-shmem_initxattrs, NULL);
+shmem_initxattrs,
+>xattrs);
if (error && error != -EOPNOTSUPP)
goto out_iput;
error = simple_acl_create(dir, inode);
@@ -3119,8 +3126,9 @@ static int shmem_symlink(struct inode *dir, struct dentry 
*dentry, const char *s
if (!inode)
return -ENOSPC;
 
+   info = SHMEM_I(inode);
error = security_inode_init_security(inode, dir, >d_name,
-shmem_initxattrs, NULL);
+shmem_initxattrs, >xattrs);
if (error) {
if (error != -EOPNOTSUPP) {
iput(inode);
@@ -3129,7 +3137,6 @@ static int shmem_symlink(struct inode *dir, struct dentry 
*dentry, const char *s
error = 0;
}
 
-   info = SHMEM_I(inode);
inode->i_size = len-1;
if (len <= SHORT_SYMLINK_LEN) {
inode->i_link = kmemdup(symname, len, GFP_KERNEL);
@@ -3198,42 +3205,6 @@ static const char *shmem_get_link(struct dentry *dentry,
  * filesystem level, though.
  */
 
-/*
- * Callback for security_inode_init_security() for acquiring xattrs.
- */
-static int shmem_initxattrs(struct inode *inode,
-   const struct xattr *xattr_array,
-   void *fs_info)
-{
-   struct shmem_inode_info *info = SHMEM_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(>xattrs, new_xattr);
-   }
-
-   return 0;
-}
-
 static int shmem_xattr_handler_get(const struct xattr_handler *handler,
 

[PATCH v4 0/3] initxattr callback update for mqueue xattr support

[David Graziano]

This patchset is for implementing extended attribute support within the 
POSIX message queue (mqueue) file system. 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. I needed to 
write a selinux policy for a set of custom applications that use mqueues 
for their IPC. The mqueues are created by one application and we needed 
a way for selinux to enforce which of the other application are able to 
read/write to each individual queue. Uniquely labelling them based on the 
application that created them and the filename seemed to be our best 
solution as it’s an embedded system and we don’t have restorecond to 
handle any relabeling.

This series is a result of feedback from the v2 mqueue patch 
( http://marc.info/?l=linux-kernel=147855351826081=2 ) which 
duplicated the shmem_initxattrs() function for the mqueue file system. 
This patcheset creates a common simple_xattr_initxattrs() function that 
can be used by multiple virtual file systems to handle extended attribute 
initialization via LSM callback. simple_xattr_initxattrs() is an updated 
version of shmem_initxattrs(). As part of the this series both shmem and 
mqueue are updated to use the new common initxattrs function. 

Changes v3 -> v4:
 - fix uninitialized variable in mqueue patch (3/3)

Changes v2 -> v3:
 - creates new simple_xattr_initxattrs() function
 - updates shmem to use new callback function
 - updates mqueue to use new callback function

Changes v1 -> v2:
 - formatting/commit message


David Graziano (3):
  xattr: add simple initxattrs function
  shmem: use simple initxattrs callback
  mqueue: Implement generic xattr support

 fs/xattr.c| 39 +
 include/linux/xattr.h |  3 +++
 ipc/mqueue.c  | 16 
 mm/shmem.c| 53 ---
 4 files changed, 70 insertions(+), 41 deletions(-)

-- 
1.9.1

[PATCH v4 0/3] initxattr callback update for mqueue xattr support

[David Graziano]

This patchset is for implementing extended attribute support within the 
POSIX message queue (mqueue) file system. 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. I needed to 
write a selinux policy for a set of custom applications that use mqueues 
for their IPC. The mqueues are created by one application and we needed 
a way for selinux to enforce which of the other application are able to 
read/write to each individual queue. Uniquely labelling them based on the 
application that created them and the filename seemed to be our best 
solution as it’s an embedded system and we don’t have restorecond to 
handle any relabeling.

This series is a result of feedback from the v2 mqueue patch 
( http://marc.info/?l=linux-kernel=147855351826081=2 ) which 
duplicated the shmem_initxattrs() function for the mqueue file system. 
This patcheset creates a common simple_xattr_initxattrs() function that 
can be used by multiple virtual file systems to handle extended attribute 
initialization via LSM callback. simple_xattr_initxattrs() is an updated 
version of shmem_initxattrs(). As part of the this series both shmem and 
mqueue are updated to use the new common initxattrs function. 

Changes v3 -> v4:
 - fix uninitialized variable in mqueue patch (3/3)

Changes v2 -> v3:
 - creates new simple_xattr_initxattrs() function
 - updates shmem to use new callback function
 - updates mqueue to use new callback function

Changes v1 -> v2:
 - formatting/commit message


David Graziano (3):
  xattr: add simple initxattrs function
  shmem: use simple initxattrs callback
  mqueue: Implement generic xattr support

 fs/xattr.c| 39 +
 include/linux/xattr.h |  3 +++
 ipc/mqueue.c  | 16 
 mm/shmem.c| 53 ---
 4 files changed, 70 insertions(+), 41 deletions(-)

-- 
1.9.1

[PATCH v4 1/3] xattr: add simple initxattrs function

[David Graziano]

Adds new simple_xattr_initxattrs() initialization function for
initializing the extended attributes via LSM callback. Based
on callback function used by tmpfs/shmem. This is allows for
consolidation and avoiding code duplication when other filesystem
need to implement a simple initxattrs LSM callback function.

Signed-off-by: David Graziano <david.grazi...@rockwellcollins.com>
---
 fs/xattr.c| 39 +++
 include/linux/xattr.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/fs/xattr.c b/fs/xattr.c
index c243905..69dd142 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -994,3 +994,42 @@ void simple_xattr_list_add(struct simple_xattrs *xattrs,
list_add(_xattr->list, >head);
spin_unlock(>lock);
 }
+
+/*
+ * Callback for security_inode_init_security() for acquiring xattrs.
+ */
+int simple_xattr_initxattrs(struct inode *inode,
+   const struct xattr *xattr_array,
+   void *fs_info)
+{
+   struct simple_xattrs *xattrs;
+   const struct xattr *xattr;
+   struct simple_xattr *new_xattr;
+   size_t len;
+
+   if (!fs_info)
+   return -ENOMEM;
+   xattrs = (struct simple_xattrs *) fs_info;
+
+   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(xattrs, new_xattr);
+   }
+
+   return 0;
+}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 94079ba..a787d1a 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -108,5 +108,8 @@ ssize_t simple_xattr_list(struct inode *inode, struct 
simple_xattrs *xattrs, cha
  size_t size);
 void simple_xattr_list_add(struct simple_xattrs *xattrs,
   struct simple_xattr *new_xattr);
+int simple_xattr_initxattrs(struct inode *inode,
+   const struct xattr *xattr_array,
+   void *fs_info);
 
 #endif /* _LINUX_XATTR_H */
-- 
1.9.1

[PATCH v4 1/3] xattr: add simple initxattrs function

[David Graziano]

Adds new simple_xattr_initxattrs() initialization function for
initializing the extended attributes via LSM callback. Based
on callback function used by tmpfs/shmem. This is allows for
consolidation and avoiding code duplication when other filesystem
need to implement a simple initxattrs LSM callback function.

Signed-off-by: David Graziano 
---
 fs/xattr.c| 39 +++
 include/linux/xattr.h |  3 +++
 2 files changed, 42 insertions(+)

diff --git a/fs/xattr.c b/fs/xattr.c
index c243905..69dd142 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -994,3 +994,42 @@ void simple_xattr_list_add(struct simple_xattrs *xattrs,
list_add(_xattr->list, >head);
spin_unlock(>lock);
 }
+
+/*
+ * Callback for security_inode_init_security() for acquiring xattrs.
+ */
+int simple_xattr_initxattrs(struct inode *inode,
+   const struct xattr *xattr_array,
+   void *fs_info)
+{
+   struct simple_xattrs *xattrs;
+   const struct xattr *xattr;
+   struct simple_xattr *new_xattr;
+   size_t len;
+
+   if (!fs_info)
+   return -ENOMEM;
+   xattrs = (struct simple_xattrs *) fs_info;
+
+   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(xattrs, new_xattr);
+   }
+
+   return 0;
+}
diff --git a/include/linux/xattr.h b/include/linux/xattr.h
index 94079ba..a787d1a 100644
--- a/include/linux/xattr.h
+++ b/include/linux/xattr.h
@@ -108,5 +108,8 @@ ssize_t simple_xattr_list(struct inode *inode, struct 
simple_xattrs *xattrs, cha
  size_t size);
 void simple_xattr_list_add(struct simple_xattrs *xattrs,
   struct simple_xattr *new_xattr);
+int simple_xattr_initxattrs(struct inode *inode,
+   const struct xattr *xattr_array,
+   void *fs_info);
 
 #endif /* _LINUX_XATTR_H */
-- 
1.9.1

[PATCH v4 3/3] mqueue: Implement generic xattr support

[David Graziano]

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. It allows a selinux policy to be created  for a set of
custom applications that use POSIX message queues for their IPC and
uniquely labeling them based on the application that creates the mqueue
eliminating the need for relabeling after the mqueue file is created.
The implementation is based on tmpfs/shmem and uses the newly created
simple_xattr_initxattrs() LSM callback function.

Signed-off-by: David Graziano <david.grazi...@rockwellcollins.com>
---
 ipc/mqueue.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 0b13ace..32271a0 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #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(>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
@@ -418,7 +421,8 @@ static int mqueue_create(struct inode *dir, struct dentry 
*dentry,
 {
struct inode *inode;
struct mq_attr *attr = dentry->d_fsdata;
-   int error;
+   struct mqueue_inode_info *info;
+   int error = 0;
struct ipc_namespace *ipc_ns;
 
spin_lock(_lock);
@@ -443,6 +447,18 @@ static int mqueue_create(struct inode *dir, struct dentry 
*dentry,
ipc_ns->mq_queues_count--;
goto out_unlock;
}
+   info = MQUEUE_I(inode);
+   if (info){
+   error = security_inode_init_security(inode, dir,
+>d_name,
+simple_xattr_initxattrs,
+>xattrs);
+   }
+   if (error && error != -EOPNOTSUPP) {
+   spin_lock(_lock);
+   ipc_ns->mq_queues_count--;
+   goto out_unlock;
+   }
 
put_ipc_ns(ipc_ns);
dir->i_size += DIRENT_SIZE;
-- 
1.9.1

[PATCH v4 3/3] mqueue: Implement generic xattr support

[David Graziano]

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. It allows a selinux policy to be created  for a set of
custom applications that use POSIX message queues for their IPC and
uniquely labeling them based on the application that creates the mqueue
eliminating the need for relabeling after the mqueue file is created.
The implementation is based on tmpfs/shmem and uses the newly created
simple_xattr_initxattrs() LSM callback function.

Signed-off-by: David Graziano 
---
 ipc/mqueue.c | 18 +-
 1 file changed, 17 insertions(+), 1 deletion(-)

diff --git a/ipc/mqueue.c b/ipc/mqueue.c
index 0b13ace..32271a0 100644
--- a/ipc/mqueue.c
+++ b/ipc/mqueue.c
@@ -35,6 +35,7 @@
 #include 
 #include 
 #include 
+#include 
 
 #include 
 #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(>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
@@ -418,7 +421,8 @@ static int mqueue_create(struct inode *dir, struct dentry 
*dentry,
 {
struct inode *inode;
struct mq_attr *attr = dentry->d_fsdata;
-   int error;
+   struct mqueue_inode_info *info;
+   int error = 0;
struct ipc_namespace *ipc_ns;
 
spin_lock(_lock);
@@ -443,6 +447,18 @@ static int mqueue_create(struct inode *dir, struct dentry 
*dentry,
ipc_ns->mq_queues_count--;
goto out_unlock;
}
+   info = MQUEUE_I(inode);
+   if (info){
+   error = security_inode_init_security(inode, dir,
+>d_name,
+simple_xattr_initxattrs,
+>xattrs);
+   }
+   if (error && error != -EOPNOTSUPP) {
+   spin_lock(_lock);
+   ipc_ns->mq_queues_count--;
+   goto out_unlock;
+   }
 
put_ipc_ns(ipc_ns);
dir->i_size += DIRENT_SIZE;
-- 
1.9.1

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Dec 5, 2016 at 5:12 PM, Paul Moore <p...@paul-moore.com> wrote:
> On Mon, Dec 5, 2016 at 11:15 AM, David Graziano
> <david.grazi...@rockwellcollins.com> wrote:
>> On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore <p...@paul-moore.com> wrote:
>>> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
>>> <david.grazi...@rockwellcollins.com> wrote:
>>>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <p...@paul-moore.com> wrote:
>>>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>>>> <david.grazi...@rockwellcollins.com> wrote:
>>>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <p...@paul-moore.com> wrote:
>>>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>>>> <david.grazi...@rockwellcollins.com> 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.grazi...@rockwellcollins.com>
>>>>>>>> ---
>>>>>>>>  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
>>&g

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Dec 5, 2016 at 5:12 PM, Paul Moore  wrote:
> On Mon, Dec 5, 2016 at 11:15 AM, David Graziano
>  wrote:
>> On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore  wrote:
>>> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
>>>  wrote:
>>>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore  wrote:
>>>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>>>>  wrote:
>>>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore  wrote:
>>>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>>>>  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 
>>>>>>>> ---
>>>>>>>>  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 thi

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore <p...@paul-moore.com> wrote:
> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
> <david.grazi...@rockwellcollins.com> wrote:
>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <p...@paul-moore.com> wrote:
>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>> <david.grazi...@rockwellcollins.com> wrote:
>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <p...@paul-moore.com> wrote:
>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>> <david.grazi...@rockwellcollins.com> 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.grazi...@rockwellcollins.com>
>>>>>> ---
>>>>>>  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.
>
> For anyone who is curious:
>
> * https://github.com/SELinuxProject/selinux-testsuite/pull/10
>
> Aside from a naming nit, the tests look good to me and I have no
> problem with the kernel patch; it doesn't appear any of the other LSM
> maintainers do either.  I'm happy to pull this into the SELinux tree
> (for v4.11, it's a little late for v4.10 I think), but I think
> Christoph made a good point about consolidation, have you had a chance
> to look at that?
>

I've made the update for the naming nit in the pull request.
I agree with Christoph's point but doing so is a bit outside my expertise at
this point. I would be open to suggestions as to where the function should be
consolidated and work on a second patchset with the update. Maybe in
fs/xattr.c as a simple_xattr_initxattrs function?
-David


> --
> paul moore
> www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Dec 5, 2016 at 9:37 AM, Paul Moore  wrote:
> On Mon, Nov 28, 2016 at 3:04 PM, David Graziano
>  wrote:
>> On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore  wrote:
>>> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>>>  wrote:
>>>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore  wrote:
>>>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>>>  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 
>>>>>> ---
>>>>>>  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.
>
> For anyone who is curious:
>
> * https://github.com/SELinuxProject/selinux-testsuite/pull/10
>
> Aside from a naming nit, the tests look good to me and I have no
> problem with the kernel patch; it doesn't appear any of the other LSM
> maintainers do either.  I'm happy to pull this into the SELinux tree
> (for v4.11, it's a little late for v4.10 I think), but I think
> Christoph made a good point about consolidation, have you had a chance
> to look at that?
>

I've made the update for the naming nit in the pull request.
I agree with Christoph's point but doing so is a bit outside my expertise at
this point. I would be open to suggestions as to where the function should be
consolidated and work on a second patchset with the update. Maybe in
fs/xattr.c as a simple_xattr_initxattrs function?
-David


> --
> paul moore
> www.paul-moore.com

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore <p...@paul-moore.com> wrote:
> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
> <david.grazi...@rockwellcollins.com> wrote:
>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <p...@paul-moore.com> wrote:
>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>> <david.grazi...@rockwellcollins.com> 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.grazi...@rockwellcollins.com>
>>>> ---
>>>>  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 
>>>>  #include 
>>>>  #include 
>>>> +#include 
>>>>
>>>>  #include 
>>>>  #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_

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Wed, Nov 9, 2016 at 4:25 PM, Paul Moore  wrote:
> On Wed, Nov 9, 2016 at 11:25 AM, David Graziano
>  wrote:
>> On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore  wrote:
>>> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>>>  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 
>>>> ---
>>>>  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 
>>>>  #include 
>>>>  #include 
>>>> +#include 
>>>>
>>>>  #include 
>>>>  #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(>xattrs);
>>>> /*
>>>&g

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore <p...@paul-moore.com> wrote:
> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
> <david.grazi...@rockwellcollins.com> 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.grazi...@rockwellcollins.com>
>> ---
>>  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.


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.

Thanks,
David

>
>> 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 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #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(>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,

Re: [PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

On Mon, Nov 7, 2016 at 4:23 PM, Paul Moore  wrote:
> On Mon, Nov 7, 2016 at 3:46 PM, David Graziano
>  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 
>> ---
>>  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.


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.

Thanks,
David

>
>> 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 
>>  #include 
>>  #include 
>> +#include 
>>
>>  #include 
>>  #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(>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-&g

[PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

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.grazi...@rockwellcollins.com>
---
 ipc/mqueue.c | 46 ++
 1 file changed, 46 insertions(+)

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 
 #include 
 #include 
+#include 
 
 #include 
 #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(>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(>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,
+>d_name,
+mqueue_initxattrs, NULL);
+   if (error && error != -EOPNOTSUPP) {
+   spin_lock(_lock);
+   ipc_ns->mq_queues_count--;
+   goto out_unlock;
+   }
 
put_ipc_ns(ipc_ns);
dir->i_size += DIRENT_SIZE;
-- 
1.9.1

[PATCH 1/1 V2] mqueue: Implment generic xattr support

[David Graziano]

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 
---
 ipc/mqueue.c | 46 ++
 1 file changed, 46 insertions(+)

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 
 #include 
 #include 
+#include 
 
 #include 
 #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(>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(>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,
+>d_name,
+mqueue_initxattrs, NULL);
+   if (error && error != -EOPNOTSUPP) {
+   spin_lock(_lock);
+   ipc_ns->mq_queues_count--;
+   goto out_unlock;
+   }
 
put_ipc_ns(ipc_ns);
dir->i_size += DIRENT_SIZE;
-- 
1.9.1