Security/sysfs: Enable security xattrs to be set on sysfs files, directories, and symlinks.

This patch adds a setxattr handler to the file, directory, and symlink inode_operations structures for sysfs. This handler uses two new LSM hooks. The first hook takes the xattr name and value and turns the context into a secid. The second hook allows for the secid to be taken from the sysfs_dirent and be pushed into the inode structure as the actual secid for the inode. As was suggested by Eric Biederman the struct iattr in the sysfs_dirent structure has been replaced by a structure which contains the iattr and secid to allow the changes to persist in the event that the inode representing the sysfs_dirent is evicted. Because sysfs only stores this information when a change is made all the optional data is moved into one dynamically allocated field.

This patch addresses an issue where SELinux was denying virtd access to the PCI configuration entries in sysfs. The lack of setxattr handlers for sysfs required that a single label be assigned to all entries in sysfs. Granting virtd access to every entry in sysfs is not an acceptable solution so fine grained labeling of sysfs is required such that individual entries can be labeled appropriately.

Signed-off-by David P. Quigley <[email protected]>

---
 fs/sysfs/dir.c             |    1 +
 fs/sysfs/inode.c           |  112 ++++++++++++++++++++++++++++++--------------
 fs/sysfs/symlink.c         |    2 +
 fs/sysfs/sysfs.h           |   11 ++++-
 include/linux/security.h   |   16 ++++++
 security/capability.c      |   12 +++++
 security/security.c        |   11 ++++
 security/selinux/hooks.c   |   24 +++++++++
 security/smack/smack_lsm.c |   42 ++++++++++++++++-
 9 files changed, 193 insertions(+), 38 deletions(-)

fs/sysfs/dir.c +1 -0

index d88d0fa..fc21682 100644
@@ -760,6 +760,7 @@ static struct dentry * sysfs_lookup(struct inode *dir, struct dentry *dentry,
 const struct inode_operations sysfs_dir_inode_operations = {
 	.lookup		= sysfs_lookup,
 	.setattr	= sysfs_setattr,
+	.setxattr	= sysfs_setxattr,
 };
 
 static void remove_dir(struct sysfs_dirent *sd)

fs/sysfs/inode.c +76 -36

index 555f0ff..9bb1ce7 100644
@@ -18,6 +18,7 @@
 #include <linux/capability.h>
 #include <linux/errno.h>
 #include <linux/sched.h>
+#include <linux/security.h>
 #include "sysfs.h"
 
 extern struct super_block * sysfs_sb;

@@ -35,6 +36,7 @@ static struct backing_dev_info sysfs_backing_dev_info = {
 
 static const struct inode_operations sysfs_inode_operations ={
 	.setattr	= sysfs_setattr,
+	.setxattr	= sysfs_setxattr,
 };
 
 int __init sysfs_inode_init(void)

@@ -42,18 +44,37 @@ int __init sysfs_inode_init(void)
 	return bdi_init(&sysfs_backing_dev_info);
 }
 
+struct sysfs_inode_attrs * sysfs_init_inode_attrs(struct sysfs_dirent * sd)
+{
+	struct sysfs_inode_attrs * attrs;
+	struct iattr * iattrs;
+
+	attrs = kzalloc(sizeof(struct sysfs_inode_attrs), GFP_KERNEL);
+	if(!attrs)
+		return NULL;
+	iattrs = &attrs->ia_iattr;
+	
+	/* assign default attributes */
+	iattrs->ia_mode = sd->s_mode;
+	iattrs->ia_uid = 0;
+	iattrs->ia_gid = 0;
+	iattrs->ia_atime = iattrs->ia_mtime = iattrs->ia_ctime = CURRENT_TIME;
+	
+	return attrs;	
+}
 int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
 {
 	struct inode * inode = dentry->d_inode;
 	struct sysfs_dirent * sd = dentry->d_fsdata;
-	struct iattr * sd_iattr;
+	struct sysfs_inode_attrs * sd_attrs;
+	struct iattr * iattrs;
 	unsigned int ia_valid = iattr->ia_valid;
 	int error;
 
 	if (!sd)
 		return -EINVAL;
 
-	sd_iattr = sd->s_iattr;
+	sd_attrs = sd->s_iattr;
 
 	error = inode_change_ok(inode, iattr);
 	if (error)

@@ -65,41 +86,57 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
 	if (error)
 		return error;
 
-	if (!sd_iattr) {
+	if (!sd_attrs) {
 		/* setting attributes for the first time, allocate now */
-		sd_iattr = kzalloc(sizeof(struct iattr), GFP_KERNEL);
-		if (!sd_iattr)
+		sd_attrs = sysfs_init_inode_attrs(sd);
+		if (!sd_attrs)
 			return -ENOMEM;
-		/* assign default attributes */
-		sd_iattr->ia_mode = sd->s_mode;
-		sd_iattr->ia_uid = 0;
-		sd_iattr->ia_gid = 0;
-		sd_iattr->ia_atime = sd_iattr->ia_mtime = sd_iattr->ia_ctime = CURRENT_TIME;
-		sd->s_iattr = sd_iattr;
+		sd->s_iattr = sd_attrs;
+	} else {
+		/* attributes were changed atleast once in past */
+		iattrs = &sd_attrs->ia_iattr;
+
+		if (ia_valid & ATTR_UID)
+			iattrs->ia_uid = iattr->ia_uid;
+		if (ia_valid & ATTR_GID)
+			iattrs->ia_gid = iattr->ia_gid;
+		if (ia_valid & ATTR_ATIME)
+			iattrs->ia_atime = timespec_trunc(iattr->ia_atime,
+					inode->i_sb->s_time_gran);
+		if (ia_valid & ATTR_MTIME)
+			iattrs->ia_mtime = timespec_trunc(iattr->ia_mtime,
+					inode->i_sb->s_time_gran);
+		if (ia_valid & ATTR_CTIME)
+			iattrs->ia_ctime = timespec_trunc(iattr->ia_ctime,
+					inode->i_sb->s_time_gran);
+		if (ia_valid & ATTR_MODE) {
+			umode_t mode = iattr->ia_mode;
+
+			if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
+				mode &= ~S_ISGID;
+			iattrs->ia_mode = sd->s_mode = mode;
+		}
 	}
+	return error;
+}
 
-	/* attributes were changed atleast once in past */
-
-	if (ia_valid & ATTR_UID)
-		sd_iattr->ia_uid = iattr->ia_uid;
-	if (ia_valid & ATTR_GID)
-		sd_iattr->ia_gid = iattr->ia_gid;
-	if (ia_valid & ATTR_ATIME)
-		sd_iattr->ia_atime = timespec_trunc(iattr->ia_atime,
-						inode->i_sb->s_time_gran);
-	if (ia_valid & ATTR_MTIME)
-		sd_iattr->ia_mtime = timespec_trunc(iattr->ia_mtime,
-						inode->i_sb->s_time_gran);
-	if (ia_valid & ATTR_CTIME)
-		sd_iattr->ia_ctime = timespec_trunc(iattr->ia_ctime,
-						inode->i_sb->s_time_gran);
-	if (ia_valid & ATTR_MODE) {
-		umode_t mode = iattr->ia_mode;
-
-		if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
-			mode &= ~S_ISGID;
-		sd_iattr->ia_mode = sd->s_mode = mode;
-	}
+int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
+		size_t size, int flags)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	
+	int error;
+	u32 secid;
+
+	if (!sd)
+		return -EINVAL;
+	if (!sd->s_iattr)
+		sd->s_iattr = sysfs_init_inode_attrs(sd);
+	if (!sd->s_iattr)
+		return -ENOMEM;
+	error = security_xattr_to_secid(name, value, size, &secid);
+	if (!error)
+		sd->s_iattr->ia_secid = secid;
 
 	return error;
 }

@@ -146,6 +183,7 @@ static int sysfs_count_nlink(struct sysfs_dirent *sd)
 static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
 {
 	struct bin_attribute *bin_attr;
+	struct sysfs_inode_attrs * iattrs;
 
 	inode->i_private = sysfs_get(sd);
 	inode->i_mapping->a_ops = &sysfs_aops;

@@ -154,16 +192,18 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
 	inode->i_ino = sd->s_ino;
 	lockdep_set_class(&inode->i_mutex, &sysfs_inode_imutex_key);
 
-	if (sd->s_iattr) {
+	iattrs = sd->s_iattr;
+	if (iattrs) {
 		/* sysfs_dirent has non-default attributes
 		 * get them for the new inode from persistent copy
 		 * in sysfs_dirent
 		 */
-		set_inode_attr(inode, sd->s_iattr);
+		set_inode_attr(inode, &iattrs->ia_iattr);
+		if (iattrs->ia_secid)
+			security_inode_setsecid(inode, iattrs->ia_secid);
 	} else
 		set_default_inode_attr(inode, sd->s_mode);
 
-
 	/* initialize inode according to type */
 	switch (sysfs_type(sd)) {
 	case SYSFS_DIR:

fs/sysfs/symlink.c +2 -0

index 1d897ad..c5081ad 100644
@@ -16,6 +16,7 @@
 #include <linux/kobject.h>
 #include <linux/namei.h>
 #include <linux/mutex.h>
+#include <linux/security.h>
 
 #include "sysfs.h"
 

@@ -209,6 +210,7 @@ static void sysfs_put_link(struct dentry *dentry, struct nameidata *nd, void *co
 }
 
 const struct inode_operations sysfs_symlink_inode_operations = {
+	.setxattr = sysfs_setxattr,
 	.readlink = generic_readlink,
 	.follow_link = sysfs_follow_link,
 	.put_link = sysfs_put_link,

fs/sysfs/sysfs.h +10 -1

index 3fa0d98..f59a211 100644
@@ -8,6 +8,8 @@
  * This file is released under the GPLv2.
  */
 
+#include <linux/fs.h>
+
 struct sysfs_open_dirent;
 
 /* type-specific structures for sysfs_dirent->s_* union members */

@@ -31,6 +33,11 @@ struct sysfs_elem_bin_attr {
 	struct hlist_head	buffers;
 };
 
+struct sysfs_inode_attrs {
+	struct iattr	ia_iattr;
+	u32		ia_secid;
+};
+
 /*
  * sysfs_dirent - the building block of sysfs hierarchy.  Each and
  * every sysfs node is represented by single sysfs_dirent.

@@ -56,7 +63,7 @@ struct sysfs_dirent {
 	unsigned int		s_flags;
 	ino_t			s_ino;
 	umode_t			s_mode;
-	struct iattr		*s_iattr;
+	struct sysfs_inode_attrs *s_iattr;
 };
 
 #define SD_DEACTIVATED_BIAS		INT_MIN

@@ -148,6 +155,8 @@ static inline void __sysfs_put(struct sysfs_dirent *sd)
 struct inode *sysfs_get_inode(struct sysfs_dirent *sd);
 void sysfs_delete_inode(struct inode *inode);
 int sysfs_setattr(struct dentry *dentry, struct iattr *iattr);
+int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
+		size_t size, int flags);
 int sysfs_hash_and_remove(struct sysfs_dirent *dir_sd, const char *name);
 int sysfs_inode_init(void);
 

include/linux/security.h +16 -0

index 1459091..35ecc8d 100644
@@ -1440,6 +1440,9 @@ struct security_operations {
 	int (*inode_setsecurity) (struct inode *inode, const char *name, const void *value, size_t size, int flags);
 	int (*inode_listsecurity) (struct inode *inode, char *buffer, size_t buffer_size);
 	void (*inode_getsecid) (const struct inode *inode, u32 *secid);
+	void (*inode_setsecid)(struct inode *inode, u32 secid);
+	int (*xattr_to_secid) (const char *name, const void *value,
+			size_t size, u32 *secid);
 
 	int (*file_permission) (struct file *file, int mask);
 	int (*file_alloc_security) (struct file *file);

@@ -1699,6 +1702,9 @@ int security_inode_getsecurity(const struct inode *inode, const char *name, void
 int security_inode_setsecurity(struct inode *inode, const char *name, const void *value, size_t size, int flags);
 int security_inode_listsecurity(struct inode *inode, char *buffer, size_t buffer_size);
 void security_inode_getsecid(const struct inode *inode, u32 *secid);
+void security_inode_setsecid(struct inode *inode, u32 secid);
+int security_xattr_to_secid(const char *name, const void *value,
+		size_t size, u32 *secid);
 int security_file_permission(struct file *file, int mask);
 int security_file_alloc(struct file *file);
 void security_file_free(struct file *file);

@@ -2172,6 +2178,16 @@ static inline void security_inode_getsecid(const struct inode *inode, u32 *secid
 	*secid = 0;
 }
 
+static inline void security_inode_setsecid(struct inode *inode, u32 secid)
+{
+}
+
+static inline int security_xattr_to_secid(const char *name, const void *value,
+		size_t size, u32 *secid)
+{
+	return -EOPNOTSUPP;
+}
+
 static inline int security_file_permission(struct file *file, int mask)
 {
 	return 0;

security/capability.c +12 -0

index f218dd3..a3f3d5b 100644
@@ -263,6 +263,16 @@ static void cap_inode_getsecid(const struct inode *inode, u32 *secid)
 	*secid = 0;
 }
 
+static void cap_inode_setsecid(struct inode *inode, u32 secid)
+{
+}
+
+int cap_xattr_to_secid(const char *name, const void *value,
+		size_t size, u32 *secid)
+{
+     return -EOPNOTSUPP;
+}
+
 #ifdef CONFIG_SECURITY_PATH
 static int cap_path_mknod(struct path *dir, struct dentry *dentry, int mode,
 			  unsigned int dev)

@@ -926,6 +936,8 @@ void security_fixup_ops(struct security_operations *ops)
 	set_to_cap_if_null(ops, inode_setsecurity);
 	set_to_cap_if_null(ops, inode_listsecurity);
 	set_to_cap_if_null(ops, inode_getsecid);
+	set_to_cap_if_null(ops, inode_setsecid);
+	set_to_cap_if_null(ops, xattr_to_secid);
 #ifdef CONFIG_SECURITY_PATH
 	set_to_cap_if_null(ops, path_mknod);
 	set_to_cap_if_null(ops, path_mkdir);

security/security.c +11 -0

index 4501c5e..8313e15 100644
@@ -615,6 +615,17 @@ void security_inode_getsecid(const struct inode *inode, u32 *secid)
 	security_ops->inode_getsecid(inode, secid);
 }
 
+void security_inode_setsecid(struct inode *inode, u32 secid)
+{
+	security_ops->inode_setsecid(inode, secid);
+}
+
+int security_xattr_to_secid(const char *name, const void *value,
+		size_t size, u32 *secid)
+{
+	return security_ops->xattr_to_secid(name, value, size, secid);
+}
+
 int security_file_permission(struct file *file, int mask)
 {
 	return security_ops->file_permission(file, mask);

security/selinux/hooks.c +24 -0

index 2081055..395c36d 100644
@@ -448,6 +448,10 @@ static int sb_finish_set_opts(struct super_block *sb)
 	    sbsec->behavior > ARRAY_SIZE(labeling_behaviors))
 		sbsec->flags &= ~SE_SBLABELSUPP;
 
+	/* Special handling for sysfs. Is genfs but also has setxattr handler*/
+	if (strncmp(sb->s_type->name, "sysfs", sizeof("sysfs")) == 0)
+		sbsec->flags |= SE_SBLABELSUPP;
+
 	/* Initialize the root inode. */
 	rc = inode_doinit_with_dentry(root_inode, root);
 

@@ -2931,6 +2935,24 @@ static void selinux_inode_getsecid(const struct inode *inode, u32 *secid)
 	*secid = isec->sid;
 }
 
+static void selinux_inode_setsecid(struct inode *inode, u32 secid)
+{
+	struct inode_security_struct *isec = inode->i_security;
+	isec->sid = secid;
+}
+
+static int selinux_xattr_to_secid(const char *name, const void *value,
+		size_t size, u32 *secid)
+{
+	if (strcmp(name, XATTR_NAME_SELINUX))
+		return -EOPNOTSUPP;
+
+	if (!value || !size)
+		return -EINVAL;
+
+	return security_context_to_sid((void *)value, size, secid);
+}
+
 /* file security operations */
 
 static int selinux_revalidate_file_permission(struct file *file, int mask)

@@ -5372,6 +5394,8 @@ static struct security_operations selinux_ops = {
 	.inode_setsecurity =		selinux_inode_setsecurity,
 	.inode_listsecurity =		selinux_inode_listsecurity,
 	.inode_getsecid =		selinux_inode_getsecid,
+	.inode_setsecid =               selinux_inode_setsecid,
+	.xattr_to_secid =               selinux_xattr_to_secid,
 
 	.file_permission =		selinux_file_permission,
 	.file_alloc_security =		selinux_file_alloc_security,

security/smack/smack_lsm.c +38 -0

index 1c9bdbc..be66c8e 100644
@@ -869,6 +869,44 @@ static void smack_inode_getsecid(const struct inode *inode, u32 *secid)
 	*secid = smack_to_secid(isp->smk_inode);
 }
 
+/**
+ * smack_inode_setsecid - Set inode's security id
+ * @inode: inode to set the info in
+ * @secid: secid to set the inode to
+ */
+static void smack_inode_setsecid(struct inode *inode, u32 secid)
+{
+	struct inode_smack *isp = inode->i_security;
+	
+	isp->smk_inode = smack_from_secid(secid);	
+}
+
+/**
+ * smack_xattr_to_secid - convert a valid xattr into a secid
+ * @name: name of the xattr attempting to be converted
+ * @value: value associated with the xattr
+ * @size: size of value
+ * @secid: location to place resuting secid
+ */
+static int smack_xattr_to_secid(const char *name, const void* value,
+			size_t size, u32 *secid)
+{
+	char *sp;	
+
+	if (strcmp(name, XATTR_NAME_SMACK))
+		return -EOPNOTSUPP;
+
+	if (!value || !size)
+		return -EINVAL;
+
+	sp = smk_import(value, size);
+	if (sp == NULL)
+		return -EINVAL;
+
+	*secid = smack_to_secid(sp);
+}
+
+
 /*
  * File Hooks
  */

Its been almost a week and I haven't heard any additional feedback on this patch. Are there any other complaints or should Greg take the patch?

I've found a problem with the patch that I want to fix. So I will be posting another version of it soon.

Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

Looking more closely at the problem I was mistaken. I was concerned with the dereferenceing of the dentry in the first line of the getxattr function but it seems that a precondition for us getting that far is that we will always have a valid dentry at that point. I'd appreciate it if the people whose code the patch touches can review the patch again and either NAK with more comments or sign off on it.

Quoted 6 lines · David P. Quigley

Looking more closely at the problem I was mistaken. I was concerned with the dereferenceing of the dentry in the first line of the getxattr function but it seems that a precondition for us getting that far is that we will always have a valid dentry at that point. I'd appreciate it if the people whose code the patch touches can review the patch again and either NAK with more comments or sign off on it.

Please repost them with a version number so we know exactly which patches we're looking at.

Quoted 10 lines · James Morris

Quoted 7 lines · David P. Quigley

Looking more closely at the problem I was mistaken. I was concerned with the dereferenceing of the dentry in the first line of the getxattr function but it seems that a precondition for us getting that far is that we will always have a valid dentry at that point. I'd appreciate it if the people whose code the patch touches can review the patch again and either NAK with more comments or sign off on it.

Quoted 5 lines

Please repost them with a version number so we know exactly which patches we're looking at.

Casey is taking a shot at implementing generic xattr support for in memory file systems.I told him I'd wait to see the initial cut of his patch before going further with this.

Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

From: Casey Schaufler <[email protected]>

This patch is in response to David P. Quigley's proposal from July of this year. That patch provided special case handling of LSM xattrs in the security name space.

This patch provides an in memory representation of general xattrs. It currently only allows xattrs in the security namespace, but that is only because the support of ACLs is beyond the day's needs. The list of xattrs for a given file is created on demand and a system that does not use xattrs should be pretty well oblivious to the changes. On the down side, this requires an unpleasant locking scheme. Improvements would of course be welcome.

This scheme should generalize to any memory based file system, although I have not attempted to create a generic implementation here.

Signed-off-by Casey Schaufler <[email protected]>

---

 fs/sysfs/dir.c     |    4 
 fs/sysfs/inode.c   |  210 +++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c |   10 +-
 fs/sysfs/sysfs.h   |   16 +++
 4 files changed, 237 insertions(+), 3 deletions(-)

diff -uprN -X linux-2.6/Documentation/dontdiff linux-2.6/fs/sysfs/inode.c linux-0812/fs/sysfs/inode.c

fs/sysfs/inode.c +0 -0

@@ -104,6 +110,210 @@ int sysfs_setattr(struct dentry * dentry
 	return error;
 }

fs/sysfs/inode.c +19 -0

+ */
+static DEFINE_MUTEX(sysfs_xattr_lock);
+
+static struct sysfs_xattr *new_xattr(const char *name, const void *value,
+					size_t size)
+{
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	char *nname;
+
+	nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
+	if (!nxattr)
+		return NULL;
+	nvalue = kzalloc(size, GFP_KERNEL);
+	if (!nvalue) {
+		kfree(nxattr);
+		return NULL;
+	}
+	nname = kzalloc(strlen(name) + 1, GFP_KERNEL);

fs/sysfs/inode.c +14 -0

+
+	return nxattr;
+}
+
+int sysfs_setxattr(struct dentry *dentry, const char *name,
+			const void *value, size_t size, int flags)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist;
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	int rc = 0;
+
+	/*

fs/sysfs/inode.c +98 -0

+
+	mutex_lock(&sysfs_xattr_lock);
+
+	if (!sd->s_xattr) {
+		sd->s_xattr = kzalloc(sizeof(*xlist), GFP_KERNEL);
+		if (!sd->s_xattr) {
+			rc = -ENOMEM;
+			goto unlock_out;
+		}
+		INIT_LIST_HEAD(sd->s_xattr);
+	}
+	xlist = sd->s_xattr;
+
+	list_for_each_entry(nxattr, xlist, list) {
+		if (!strcmp(nxattr->sx_name, name)) {
+			if (flags & XATTR_CREATE) {
+				rc = -EEXIST;
+				goto unlock_out;
+			}
+			nvalue = kzalloc(size, GFP_KERNEL);
+			if (!nvalue) {
+				rc = -ENOMEM;
+				goto unlock_out;
+			}
+			memcpy(nvalue, value, size);
+			kfree(nxattr->sx_value);
+			nxattr->sx_value = nvalue;
+			nxattr->sx_size = size;
+			rc = 0;
+			goto unlock_out;
+		}
+	}
+	if (flags & XATTR_REPLACE) {
+		rc = -ENOENT;
+		goto unlock_out;
+	}
+	nxattr = new_xattr(name, value, size);
+	list_add_tail(&nxattr->list, xlist);
+
+unlock_out:
+	mutex_unlock(&sysfs_xattr_lock);
+	return rc;
+}
+
+ssize_t sysfs_getxattr(struct dentry *dentry, const char *name,
+			void *value, size_t size)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist = sd->s_xattr;
+	struct sysfs_xattr *nxattr;
+	int rc = -ENODATA;
+
+	if (!xlist)
+		return -ENODATA;
+
+	mutex_lock(&sysfs_xattr_lock);
+
+	list_for_each_entry(nxattr, xlist, list) {
+		if (!strcmp(nxattr->sx_name, name)) {
+			if (size <= 0) {
+				rc = nxattr->sx_size;
+				goto unlock_out;
+			}
+			if (nxattr->sx_size > size) {
+				rc = -ERANGE;
+				goto unlock_out;
+			}
+			memcpy(value, nxattr->sx_value, nxattr->sx_size);
+			rc = nxattr->sx_size;
+			goto unlock_out;
+		}
+	}
+
+unlock_out:
+	mutex_unlock(&sysfs_xattr_lock);
+	return rc;
+}
+
+ssize_t sysfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist = sd->s_xattr;
+	struct sysfs_xattr *nxattr;
+	ssize_t total = 0;
+	char *cp = buffer;
+
+	if (!xlist)
+		return 0;
+
+	mutex_lock(&sysfs_xattr_lock);
+
+	list_for_each_entry(nxattr, xlist, list)
+		total += strlen(nxattr->sx_name) + 1;
+
+	if (total > size) {
+		total = -ERANGE;
+		goto unlock_out;
+	}

fs/sysfs/inode.c +33 -0

+int sysfs_removexattr(struct dentry *dentry, const char *name)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist = sd->s_xattr;
+	struct sysfs_xattr *nxattr;
+	int rc = -ENODATA;
+
+	if (!xlist)
+		return -ENODATA;
+
+	mutex_lock(&sysfs_xattr_lock);
+
+	list_for_each_entry(nxattr, xlist, list) {
+		if (!strcmp(nxattr->sx_name, name)) {
+			list_del(&nxattr->list);
+			if (list_empty(xlist)) {
+				kfree(xlist);
+				sd->s_xattr = NULL;
+			}
+			kfree(nxattr->sx_name);
+			kfree(nxattr->sx_value);
+			kfree(nxattr);
+			rc = 0;
+			goto unlock_out;
+		}
+	}
+
+unlock_out:
+	mutex_unlock(&sysfs_xattr_lock);
+	return rc;
+}
+
+
 static inline void set_default_inode_attr(struct inode * inode, mode_t mode)
 {
 	inode->i_mode = mode;

Quoted 15 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

This patch is in response to David P. Quigley's proposal from July of this year. That patch provided special case handling of LSM xattrs in the security name space.

This patch provides an in memory representation of general xattrs. It currently only allows xattrs in the security namespace, but that is only because the support of ACLs is beyond the day's needs. The list of xattrs for a given file is created on demand and a system that does not use xattrs should be pretty well oblivious to the changes. On the down side, this requires an unpleasant locking scheme. Improvements would of course be welcome.

Quoted 3 lines

This scheme should generalize to any memory based file system, although I have not attempted to create a generic implementation here.

I don't understand the benefits of this scheme compared to David's patch
- can you explain? It seems worse in terms of locking, memory use, and is no more general than David's patch. More specific comments below.

Quoted 16 lines

Signed-off-by Casey Schaufler <[email protected]>

---

 fs/sysfs/dir.c     |    4 
 fs/sysfs/inode.c   |  210 +++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c |   10 +-
 fs/sysfs/sysfs.h   |   16 +++
 4 files changed, 237 insertions(+), 3 deletions(-)

diff -uprN -X linux-2.6/Documentation/dontdiff linux-2.6/fs/sysfs/inode.c linux-0812/fs/sysfs/inode.c

fs/sysfs/inode.c +0 -0

@@ -104,6 +110,210 @@ int sysfs_setattr(struct dentry * dentry
 	return error;
 }

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 121

Quoted 10 lines

fs/sysfs/inode.c +9 -0

 
+/*
+ * Extended attributes are stored on a list off of the dirent.
+ * The list head itself is allocated when needed so that a file
+ * with no xattrs does not have the overhead of a list head.
+ * Unfortunately, to lock the xattr list for each dentry would
+ * require a lock in each dentry, which would defeat the purpose
+ * of allocating the list head. So one big sysfs xattr lock.
+ *
+ * A better solution would be welcome.

What was wrong with David's approach of wrapping the iattr with a containing structure that could also include the security attribute?

Quoted 25 lines

fs/sysfs/inode.c +25 -0

+ */
+static DEFINE_MUTEX(sysfs_xattr_lock);
+
+static struct sysfs_xattr *new_xattr(const char *name, const void *value,
+					size_t size)
+{
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	char *nname;
+
+	nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
+	if (!nxattr)
+		return NULL;
+	nvalue = kzalloc(size, GFP_KERNEL);
+	if (!nvalue) {
+		kfree(nxattr);
+		return NULL;
+	}
+	nname = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!nname) {
+		kfree(nxattr);
+		kfree(nvalue);
+		return NULL;
+	}
+	memcpy(nvalue, value, size);

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 150

Quoted 4 lines

fs/sysfs/inode.c +4 -0

+	strcpy(nname, name);
+	nxattr->sx_name = nname;
+	nxattr->sx_value = nvalue;
+	nxattr->sx_size = size;

Storing the name/value pairs here is redundant - the security module already has to store the value in some form (potentially smaller, like a secid + struct in the SELinux case). This wastes memory.

Quoted 20 lines

fs/sysfs/inode.c +19 -0

+
+	return nxattr;
+}
+
+int sysfs_setxattr(struct dentry *dentry, const char *name,
+			const void *value, size_t size, int flags)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist;
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	int rc = 0;
+
+	/*
+	 * Only support the security namespace.
+	 * Only allow privileged processes to set them.
+	 * It has to be OK with the LSM, if any, as well.
+	 */
+	if (strncmp(name, XATTR_SECURITY_PREFIX,

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 174

Quoted 5 lines

fs/sysfs/inode.c +5 -0

+			sizeof XATTR_SECURITY_PREFIX - 1))
+		return -ENOTSUPP;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;

SELinux does not require CAP_SYS_ADMIN to set its attributes, so this breaks its security model.

Quoted 26 lines · Stephen Smalley

...

Quoted 25 lines

+ */

fs/sysfs/inode.c +24 -0

+static DEFINE_MUTEX(sysfs_xattr_lock);
+
+static struct sysfs_xattr *new_xattr(const char *name, const void *value,
+					size_t size)
+{
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	char *nname;
+
+	nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
+	if (!nxattr)
+		return NULL;
+	nvalue = kzalloc(size, GFP_KERNEL);
+	if (!nvalue) {
+		kfree(nxattr);
+		return NULL;
+	}
+	nname = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!nname) {
+		kfree(nxattr);
+		kfree(nvalue);
+		return NULL;
+	}
+	memcpy(nvalue, value, size);

Quoted 9 lines

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 150

Quoted 5 lines

fs/sysfs/inode.c +4 -0

+	strcpy(nname, name);
+	nxattr->sx_name = nname;
+	nxattr->sx_value = nvalue;
+	nxattr->sx_size = size;

Storing the name/value pairs here is redundant - the security module already has to store the value in some form (potentially smaller, like a secid + struct in the SELinux case). This wastes memory.

Sorry - to clarify, I understand that we have to store a representation of the security attribute in the backing data structure so that it can be restored later, but that representation should come from the security module rather than being the original (name, value, size) triple. Which is what David's patch does - he obtains a secid from the security module for storage in the wrapped iattr structure.

Quoted 19 lines

Quoted 19 lines

fs/sysfs/inode.c +19 -0

+
+	return nxattr;
+}
+
+int sysfs_setxattr(struct dentry *dentry, const char *name,
+			const void *value, size_t size, int flags)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist;
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	int rc = 0;
+
+	/*
+	 * Only support the security namespace.
+	 * Only allow privileged processes to set them.
+	 * It has to be OK with the LSM, if any, as well.
+	 */
+	if (strncmp(name, XATTR_SECURITY_PREFIX,

Quoted 9 lines

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 174

Quoted 6 lines

fs/sysfs/inode.c +5 -0

+			sizeof XATTR_SECURITY_PREFIX - 1))
+		return -ENOTSUPP;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;

SELinux does not require CAP_SYS_ADMIN to set its attributes, so this breaks its security model.

And you don't need to apply any permission check here, as it gets covered by the security_inode_setxattr() hook in vfs_setxattr() prior to invoking i_op->setxattr.

Quoted 38 lines · Stephen Smalley

Quoted 26 lines · Stephen Smalley

...

Quoted 25 lines

+ */

fs/sysfs/inode.c +24 -0

+static DEFINE_MUTEX(sysfs_xattr_lock);
+
+static struct sysfs_xattr *new_xattr(const char *name, const void *value,
+					size_t size)
+{
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	char *nname;
+
+	nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
+	if (!nxattr)
+		return NULL;
+	nvalue = kzalloc(size, GFP_KERNEL);
+	if (!nvalue) {
+		kfree(nxattr);
+		return NULL;
+	}
+	nname = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!nname) {
+		kfree(nxattr);
+		kfree(nvalue);
+		return NULL;
+	}
+	memcpy(nvalue, value, size);

Quoted 9 lines

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 150

Quoted 5 lines

fs/sysfs/inode.c +4 -0

+	strcpy(nname, name);
+	nxattr->sx_name = nname;
+	nxattr->sx_value = nvalue;
+	nxattr->sx_size = size;

Storing the name/value pairs here is redundant - the security module already has to store the value in some form (potentially smaller, like a secid + struct in the SELinux case). This wastes memory.

Quoted 7 lines

Sorry - to clarify, I understand that we have to store a representation of the security attribute in the backing data structure so that it can be restored later, but that representation should come from the security module rather than being the original (name, value, size) triple. Which is what David's patch does - he obtains a secid from the security module for storage in the wrapped iattr structure.

Sorry, but I disagree with your assertion. An LSM can do what it likes with the xattr, but the value sent from userland is what should be stored.

Quoted 15 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

This patch is in response to David P. Quigley's proposal from July of this year. That patch provided special case handling of LSM xattrs in the security name space.

This patch provides an in memory representation of general xattrs. It currently only allows xattrs in the security namespace, but that is only because the support of ACLs is beyond the day's needs. The list of xattrs for a given file is created on demand and a system that does not use xattrs should be pretty well oblivious to the changes. On the down side, this requires an unpleasant locking scheme. Improvements would of course be welcome.

Quoted 4 lines

This scheme should generalize to any memory based file system, although I have not attempted to create a generic implementation here.

I don't understand the benefits of this scheme compared to David's patch
- can you explain?

It seems worse in terms of locking,

memory use,

and
is no more general than David's patch.

More specific comments below.

Quoted 19 lines

Signed-off-by Casey Schaufler <[email protected]>

---

 fs/sysfs/dir.c     |    4 
 fs/sysfs/inode.c   |  210 +++++++++++++++++++++++++++++++++++++++++++
 fs/sysfs/symlink.c |   10 +-
 fs/sysfs/sysfs.h   |   16 +++
 4 files changed, 237 insertions(+), 3 deletions(-)

diff -uprN -X linux-2.6/Documentation/dontdiff linux-2.6/fs/sysfs/inode.c linux-0812/fs/sysfs/inode.c

fs/sysfs/inode.c +0 -0

@@ -104,6 +110,210 @@ int sysfs_setattr(struct dentry * dentry
 	return error;
 }
 

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry

Quoted 10 lines

fs/sysfs/inode.c +9 -0

+/*
+ * Extended attributes are stored on a list off of the dirent.
+ * The list head itself is allocated when needed so that a file
+ * with no xattrs does not have the overhead of a list head.
+ * Unfortunately, to lock the xattr list for each dentry would
+ * require a lock in each dentry, which would defeat the purpose
+ * of allocating the list head. So one big sysfs xattr lock.
+ *
+ * A better solution would be welcome.
 

What was wrong with David's approach of wrapping the iattr with a containing structure that could also include the security attribute?

Quoted 26 lines

fs/sysfs/inode.c +26 -0

+ */
+static DEFINE_MUTEX(sysfs_xattr_lock);
+
+static struct sysfs_xattr *new_xattr(const char *name, const void *value,
+					size_t size)
+{
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	char *nname;
+
+	nxattr = kzalloc(sizeof(*nxattr), GFP_KERNEL);
+	if (!nxattr)
+		return NULL;
+	nvalue = kzalloc(size, GFP_KERNEL);
+	if (!nvalue) {
+		kfree(nxattr);
+		return NULL;
+	}
+	nname = kzalloc(strlen(name) + 1, GFP_KERNEL);
+	if (!nname) {
+		kfree(nxattr);
+		kfree(nvalue);
+		return NULL;
+	}
+	memcpy(nvalue, value, size);
+	strcpy(nname, name);

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry

Quoted 4 lines

fs/sysfs/inode.c +3 -0

+	nxattr->sx_name = nname;
+	nxattr->sx_value = nvalue;
+	nxattr->sx_size = size;
 

Storing the name/value pairs here is redundant - the security module already has to store the value in some form (potentially smaller, like a secid + struct in the SELinux case). This wastes memory.

Quoted 20 lines

fs/sysfs/inode.c +20 -0

+
+	return nxattr;
+}
+
+int sysfs_setxattr(struct dentry *dentry, const char *name,
+			const void *value, size_t size, int flags)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist;
+	struct sysfs_xattr *nxattr;
+	void *nvalue;
+	int rc = 0;
+
+	/*
+	 * Only support the security namespace.
+	 * Only allow privileged processes to set them.
+	 * It has to be OK with the LSM, if any, as well.
+	 */
+	if (strncmp(name, XATTR_SECURITY_PREFIX,
+			sizeof XATTR_SECURITY_PREFIX - 1))

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry

Quoted 5 lines

fs/sysfs/inode.c +4 -0

+		return -ENOTSUPP;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
 

SELinux does not require CAP_SYS_ADMIN to set its attributes, so this breaks its security model.

Quoted 99 lines

fs/sysfs/inode.c +98 -0

+
+	mutex_lock(&sysfs_xattr_lock);
+
+	if (!sd->s_xattr) {
+		sd->s_xattr = kzalloc(sizeof(*xlist), GFP_KERNEL);
+		if (!sd->s_xattr) {
+			rc = -ENOMEM;
+			goto unlock_out;
+		}
+		INIT_LIST_HEAD(sd->s_xattr);
+	}
+	xlist = sd->s_xattr;
+
+	list_for_each_entry(nxattr, xlist, list) {
+		if (!strcmp(nxattr->sx_name, name)) {
+			if (flags & XATTR_CREATE) {
+				rc = -EEXIST;
+				goto unlock_out;
+			}
+			nvalue = kzalloc(size, GFP_KERNEL);
+			if (!nvalue) {
+				rc = -ENOMEM;
+				goto unlock_out;
+			}
+			memcpy(nvalue, value, size);
+			kfree(nxattr->sx_value);
+			nxattr->sx_value = nvalue;
+			nxattr->sx_size = size;
+			rc = 0;
+			goto unlock_out;
+		}
+	}
+	if (flags & XATTR_REPLACE) {
+		rc = -ENOENT;
+		goto unlock_out;
+	}
+	nxattr = new_xattr(name, value, size);
+	list_add_tail(&nxattr->list, xlist);
+
+unlock_out:
+	mutex_unlock(&sysfs_xattr_lock);
+	return rc;
+}
+
+ssize_t sysfs_getxattr(struct dentry *dentry, const char *name,
+			void *value, size_t size)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist = sd->s_xattr;
+	struct sysfs_xattr *nxattr;
+	int rc = -ENODATA;
+
+	if (!xlist)
+		return -ENODATA;
+
+	mutex_lock(&sysfs_xattr_lock);
+
+	list_for_each_entry(nxattr, xlist, list) {
+		if (!strcmp(nxattr->sx_name, name)) {
+			if (size <= 0) {
+				rc = nxattr->sx_size;
+				goto unlock_out;
+			}
+			if (nxattr->sx_size > size) {
+				rc = -ERANGE;
+				goto unlock_out;
+			}
+			memcpy(value, nxattr->sx_value, nxattr->sx_size);
+			rc = nxattr->sx_size;
+			goto unlock_out;
+		}
+	}
+
+unlock_out:
+	mutex_unlock(&sysfs_xattr_lock);
+	return rc;
+}
+
+ssize_t sysfs_listxattr(struct dentry *dentry, char *buffer, size_t size)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist = sd->s_xattr;
+	struct sysfs_xattr *nxattr;
+	ssize_t total = 0;
+	char *cp = buffer;
+
+	if (!xlist)
+		return 0;
+
+	mutex_lock(&sysfs_xattr_lock);
+
+	list_for_each_entry(nxattr, xlist, list)
+		total += strlen(nxattr->sx_name) + 1;
+
+	if (total > size) {
+		total = -ERANGE;
+		goto unlock_out;
+	}

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry · line 282

Quoted 10 lines

fs/sysfs/inode.c +10 -0

+
+	list_for_each_entry(nxattr, xlist, list) {
+		strcpy(cp, nxattr->sx_name);
+		cp += strlen(nxattr->sx_name) + 1;
+	}
+
+unlock_out:
+	mutex_unlock(&sysfs_xattr_lock);
+	return total;
+}

You are duplicating a lot of machinery that is already supported via the vfs fallback behavior for the security namespace. We can already support getxattr, listxattr, setxattr of security atttributes on in memory filesystems via those fallbacks. The only thing we lack is the ability to preserve attributes when an inode is evicted from memory, which is only an issue when the inodes are not pinned (as in the sysfs case).

And there all we need is support for saving the attribute in the backing data structure, which is precisely what David's patch is about.

Quoted 28 lines

fs/sysfs/inode.c +28 -0

+int sysfs_removexattr(struct dentry *dentry, const char *name)
+{
+	struct sysfs_dirent *sd = dentry->d_fsdata;
+	struct list_head *xlist = sd->s_xattr;
+	struct sysfs_xattr *nxattr;
+	int rc = -ENODATA;
+
+	if (!xlist)
+		return -ENODATA;
+
+	mutex_lock(&sysfs_xattr_lock);
+
+	list_for_each_entry(nxattr, xlist, list) {
+		if (!strcmp(nxattr->sx_name, name)) {
+			list_del(&nxattr->list);
+			if (list_empty(xlist)) {
+				kfree(xlist);
+				sd->s_xattr = NULL;
+			}
+			kfree(nxattr->sx_name);
+			kfree(nxattr->sx_value);
+			kfree(nxattr);
+			rc = 0;
+			goto unlock_out;
+		}
+	}
+
+unlock_out:

fs/sysfs/inode.c int sysfs_setattr(struct dentry * dentry

Quoted 8 lines

fs/sysfs/inode.c +5 -0

+	mutex_unlock(&sysfs_xattr_lock);
+	return rc;
+}
+
+
 static inline void set_default_inode_attr(struct inode * inode, mode_t mode)
 {
 	inode->i_mode = mode

Quoted 17 lines · Stephen Smalley

Quoted 15 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

This patch is in response to David P. Quigley's proposal from July of this year. That patch provided special case handling of LSM xattrs in the security name space.

This patch provides an in memory representation of general xattrs. It currently only allows xattrs in the security namespace, but that is only because the support of ACLs is beyond the day's needs. The list of xattrs for a given file is created on demand and a system that does not use xattrs should be pretty well oblivious to the changes. On the down side, this requires an unpleasant locking scheme. Improvements would of course be welcome.

Quoted 7 lines

Quoted 4 lines

This scheme should generalize to any memory based file system, although I have not attempted to create a generic implementation here.

I don't understand the benefits of this scheme compared to David's patch
- can you explain?

Sure, you bet. David's scheme requires as LSM and is only capable of supporting security namespace attributes. It is further not a reasonable model for other memory based filesystems. If all you ever want to support is SELinux, it would be fine, but an LSM that uses multiple xattrs (Smack only uses multiple xattrs on sockets, but it does use them) would be hard pressed to work off of a secid. This is a swag at real xattr support, which is the right thing to do for any filesystem. Special purpose interfaces to solve a single instance of a problem are for squares.

Quoted 1 line

It seems worse in terms of locking,

I'll buy that, and happily incorporate improvements. The crude locking is an artifact of keeping memory usage down.

Quoted 1 line

memory use,

There are less than 12,000 entries in /sys on my machine. Is that really an issue?

Quoted 2 lines

and
is no more general than David's patch.

Except that one could (fix the locking and) port this code to other memory based file systems, such as smackfs or selinuxfs without much bother. You'd have to implement new LSM hooks for each file system you ported the other approach for. It can be used without an LSM, it could support multiple security xattrs and it can support other namespaces.

From a long term maintenance perspective I have to say I like Casey's

From an implementation point of view Stephen's comments seem accurate.

Quoted 1 line

From a long term maintenance perspective I have to say I like Casey's

version that doesn't require any magic security module compression hooks to implement this.

Quoted 1 line

From an implementation point of view Stephen's comments seem accurate.

Is there any debate that Casey's version will get the job done?

If not the only real question is how do we store xattrs efficiently for in memory filesystems.

Let me propose a different xattr compression scheme instead of making the security module responsible for compression.

How about we have:
struct sysfs_xattr *sysfs_store_xattr(const char *name, const void *value, size_t size); void sysfs_release_xattr(struct sysfs_xattr *xattr);

Where sysfs_xattr looks something like:

struct sysfs_xattr {
       struct hlist_node list;
       atomic_t count;
       const char *name;
       const void *value;
       size_t size;
}

And then at the end of of what is today s_iattr we have something like:
struct sysfs_xattr *xattrs[2];

The key point here is that sysfs_store_xattr will look to see in a hash table to see if another inode/dentry has already stored the specified xattr and if so increment the count of sysfs_xattr count and return it.

My design assumption is that the set of xattrs that people want to apply to filesystems is small.

By optimizing for a small number of distinct xattrs we keep the storage size small (even for those filesystems who want to label everything), and we keep the generality by not making storage of xattrs a security module responsibility.

Does that sound like it will work?

Eric

Actually, I see that as a justification for the special purpose scheme rather than a real issue. The real attribute data is going to take up the same amount of space regardless of how it gets managed. And Stephen is correct in thinking that is most cases where there are xattrs there will be only one. I don't see that a mechanism more elaborate than a list is going to gain much in real life. On the other hand, if you wanted to take the ball and run with it, I have a window manager to deal with.

Quoted 9 lines · Casey Schaufler

Actually, I see that as a justification for the special purpose scheme rather than a real issue. The real attribute data is going to take up the same amount of space regardless of how it gets managed. And Stephen is correct in thinking that is most cases where there are xattrs there will be only one. I don't see that a mechanism more elaborate than a list is going to gain much in real life. On the other hand, if you wanted to take the ball and run with it, I have a window manager to deal with.

If you look at things from the point of view of a single inode I would have to agree that the storage costs are roughly the same however they get managed.

My understanding is that in most inodes all get a label from a very small set of possible labels.

If that is true. It makes sense to store the set of used labels separately from the inodes. Then on the inode just store a pointer to the label.

Saying this in lisp parlance we should be able to use atoms instead of strings.

At which point we have (I believe) an implementation that is as practically as efficient as what was originally proposed but as general and as maintainable as your version.

What I don't know is if the set of labels applied to a filesystem is actually small, despite having a large number of labels applied.

Quoted 17 lines · Eric W. Biederman

Quoted 9 lines · Casey Schaufler

Actually, I see that as a justification for the special purpose scheme rather than a real issue. The real attribute data is going to take up the same amount of space regardless of how it gets managed. And Stephen is correct in thinking that is most cases where there are xattrs there will be only one. I don't see that a mechanism more elaborate than a list is going to gain much in real life. On the other hand, if you wanted to take the ball and run with it, I have a window manager to deal with.

If you look at things from the point of view of a single inode I would have to agree that the storage costs are roughly the same however they get managed.

Quoted 3 lines

My understanding is that in most inodes all get a label from a very small set of possible labels.

Actually, my point is that most files that get xattrs get one xattr. The set of labels will be small for a Smack system or a real world MLS system. This is not true for SELinux, where the number of labels in flight at any given time can be .... large.

Quoted 4 lines

If that is true. It makes sense to store the set of used labels separately from the inodes. Then on the inode just store a pointer to the label.

You have been reading the Smack code, haven't you?

Quoted 3 lines

Saying this in lisp parlance we should be able to use atoms instead of strings.

Sorry, but you're not old enough to be slinging lisp at me. (smiley) But yes, that is an apt comparison.

Quoted 4 lines

At which point we have (I believe) an implementation that is as practically as efficient as what was originally proposed but as general and as maintainable as your version.

Quoted 3 lines

What I don't know is if the set of labels applied to a filesystem is actually small, despite having a large number of labels applied.

The question is whether you're more concerned with the xattr behavior or the LSM behavior. This is where the difference shows itself. If you look at the LSM, there is one value. If you look at xattrs, there may be many.

I personally think that the Right Thing(tm) is the xattr view. My experience is the LSM view. I hope that we don't shut off the Right Thing in preference to the issue of the day. I have been on both sides on this one.

If it is a valid optimization to inter atoms as the labels and just hold a couple. I think the implementation is roughly comparable in size/complexity with what you have now, and likely to be a nicer in the common case for the people who want to do play with labels and sysfs.

From: Casey Schaufler <[email protected]>

Another approach to limited xattr support in sysfs.

I tried to listen to the objections to a linked list representation and I think that I understand that there isn't really any interest in supporting xattrs for real, only for those maintained by LSMs. I also looked carefully into the claims that memory usage is critical and that the code I had before was duplicating effort.

This version lets the surrounding code do as much of the work as possible. Unlike the initial proposal for sysfs xattrs, it does not introduce any new LSM hooks, it uses hooks that already exist. It does not support any attributes on its own, it only provides for the attribute advertised by security_inode_listsecurity(). It could easily be used by other filesystems to provide the same LSM xattr support. It could also be extended to do the list based support for arbitrary xattrs without too much effort.

Probably the oddest bit is that the inode_getsecurity hooks need to check to see if they are getting called before the inode is instantiated and return -ENODATA in that event. It would be possible to do a filesystem specific check instead, but this way provides for generally correct behavior at small cost.

This has been tested with Smack, but not SELinux. I think that SELinux will work correctly, but it could be that a labeling behavior that is different than the "usual" instantiation labeling is actually desired. That would be an easy change.

As always, let me know if I missed something obvious or if there's a fatal flaw in the scheme.

Quoted 25 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

Another approach to limited xattr support in sysfs.

I tried to listen to the objections to a linked list representation and I think that I understand that there isn't really any interest in supporting xattrs for real, only for those maintained by LSMs. I also looked carefully into the claims that memory usage is critical and that the code I had before was duplicating effort.

This version lets the surrounding code do as much of the work as possible. Unlike the initial proposal for sysfs xattrs, it does not introduce any new LSM hooks, it uses hooks that already exist. It does not support any attributes on its own, it only provides for the attribute advertised by security_inode_listsecurity(). It could easily be used by other filesystems to provide the same LSM xattr support. It could also be extended to do the list based support for arbitrary xattrs without too much effort.

Probably the oddest bit is that the inode_getsecurity hooks need to check to see if they are getting called before the inode is instantiated and return -ENODATA in that event. It would be possible to do a filesystem specific check instead, but this way provides for generally correct behavior at small cost.

Quoted 8 lines

This has been tested with Smack, but not SELinux. I think that SELinux will work correctly, but it could be that a labeling behavior that is different than the "usual" instantiation labeling is actually desired. That would be an easy change.

As always, let me know if I missed something obvious or if there's a fatal flaw in the scheme.

The point of the David's patch was to provide a way to save the security xattr in the backing data structure for sysfs entries when an attribute value is set from userspace so that the value can be preserved if the inode is evicted from memory and later re-instantiated. AFAICS, your patch completely misses the problem. How about we just go back to David's patch?

Quoted 36 lines · Stephen Smalley

Quoted 25 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

Another approach to limited xattr support in sysfs.

I tried to listen to the objections to a linked list representation and I think that I understand that there isn't really any interest in supporting xattrs for real, only for those maintained by LSMs. I also looked carefully into the claims that memory usage is critical and that the code I had before was duplicating effort.

This version lets the surrounding code do as much of the work as possible. Unlike the initial proposal for sysfs xattrs, it does not introduce any new LSM hooks, it uses hooks that already exist. It does not support any attributes on its own, it only provides for the attribute advertised by security_inode_listsecurity(). It could easily be used by other filesystems to provide the same LSM xattr support. It could also be extended to do the list based support for arbitrary xattrs without too much effort.

Probably the oddest bit is that the inode_getsecurity hooks need to check to see if they are getting called before the inode is instantiated and return -ENODATA in that event. It would be possible to do a filesystem specific check instead, but this way provides for generally correct behavior at small cost.

Quoted 8 lines

This has been tested with Smack, but not SELinux. I think that SELinux will work correctly, but it could be that a labeling behavior that is different than the "usual" instantiation labeling is actually desired. That would be an easy change.

As always, let me know if I missed something obvious or if there's a fatal flaw in the scheme.

Quoted 7 lines

The point of the David's patch was to provide a way to save the security xattr in the backing data structure for sysfs entries when an attribute value is set from userspace so that the value can be preserved if the inode is evicted from memory and later re-instantiated. AFAICS, your patch completely misses the problem. How about we just go back to David's patch?

Oh no, that would use too much memory!

Either you care about the value the user set, in which case you want to save the value the user set, or you don't. If you do, you have to save that value, not an LSM's interpretation of that value. No secids. No new hooks.

I'll have another go. Thank you for the clarification.

Quoted 47 lines · Casey Schaufler

Quoted 37 lines · Stephen Smalley

Quoted 25 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

Another approach to limited xattr support in sysfs.

I tried to listen to the objections to a linked list representation and I think that I understand that there isn't really any interest in supporting xattrs for real, only for those maintained by LSMs. I also looked carefully into the claims that memory usage is critical and that the code I had before was duplicating effort.

This version lets the surrounding code do as much of the work as possible. Unlike the initial proposal for sysfs xattrs, it does not introduce any new LSM hooks, it uses hooks that already exist. It does not support any attributes on its own, it only provides for the attribute advertised by security_inode_listsecurity(). It could easily be used by other filesystems to provide the same LSM xattr support. It could also be extended to do the list based support for arbitrary xattrs without too much effort.

Probably the oddest bit is that the inode_getsecurity hooks need to check to see if they are getting called before the inode is instantiated and return -ENODATA in that event. It would be possible to do a filesystem specific check instead, but this way provides for generally correct behavior at small cost.

Quoted 9 lines

This has been tested with Smack, but not SELinux. I think that SELinux will work correctly, but it could be that a labeling behavior that is different than the "usual" instantiation labeling is actually desired. That would be an easy change.

As always, let me know if I missed something obvious or if there's a fatal flaw in the scheme.

Quoted 8 lines

The point of the David's patch was to provide a way to save the security xattr in the backing data structure for sysfs entries when an attribute value is set from userspace so that the value can be preserved if the inode is evicted from memory and later re-instantiated. AFAICS, your patch completely misses the problem. How about we just go back to David's patch?

Quoted 7 lines

Oh no, that would use too much memory!

Either you care about the value the user set, in which case you want to save the value the user set, or you don't. If you do, you have to save that value, not an LSM's interpretation of that value. No secids. No new hooks.

As the security module is the only component of the kernel that uses/interprets that value, it isn't unreasonable for the security module's interpretation of that value to be considered canonical. In fact, that is already the case - the hooks within vfs_getxattr() enable the security module to override/replace the actual security xattr value returned to userspace.

In the case of Smack, Smack could just provide a pointer to its own internal copy of the string, and that could be stored in the wrapped iattr. In the case of SELinux, we could provide a secid that could be stored in the wrapped iattr. The hook interface could just handle it as a blob if you prefer. But either way we don't need extra storage aside from a pointer-size field in the wrapped iattr.

Quoted 64 lines · Stephen Smalley

Quoted 47 lines · Casey Schaufler

Quoted 37 lines · Stephen Smalley

Quoted 25 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

Another approach to limited xattr support in sysfs.

I tried to listen to the objections to a linked list representation and I think that I understand that there isn't really any interest in supporting xattrs for real, only for those maintained by LSMs. I also looked carefully into the claims that memory usage is critical and that the code I had before was duplicating effort.

This version lets the surrounding code do as much of the work as possible. Unlike the initial proposal for sysfs xattrs, it does not introduce any new LSM hooks, it uses hooks that already exist. It does not support any attributes on its own, it only provides for the attribute advertised by security_inode_listsecurity(). It could easily be used by other filesystems to provide the same LSM xattr support. It could also be extended to do the list based support for arbitrary xattrs without too much effort.

Probably the oddest bit is that the inode_getsecurity hooks need to check to see if they are getting called before the inode is instantiated and return -ENODATA in that event. It would be possible to do a filesystem specific check instead, but this way provides for generally correct behavior at small cost.

Quoted 9 lines

This has been tested with Smack, but not SELinux. I think that SELinux will work correctly, but it could be that a labeling behavior that is different than the "usual" instantiation labeling is actually desired. That would be an easy change.

As always, let me know if I missed something obvious or if there's a fatal flaw in the scheme.

Quoted 8 lines

The point of the David's patch was to provide a way to save the security xattr in the backing data structure for sysfs entries when an attribute value is set from userspace so that the value can be preserved if the inode is evicted from memory and later re-instantiated. AFAICS, your patch completely misses the problem. How about we just go back to David's patch?

Quoted 7 lines

Oh no, that would use too much memory!

Either you care about the value the user set, in which case you want to save the value the user set, or you don't. If you do, you have to save that value, not an LSM's interpretation of that value. No secids. No new hooks.

As the security module is the only component of the kernel that uses/interprets that value, it isn't unreasonable for the security module's interpretation of that value to be considered canonical. In fact, that is already the case - the hooks within vfs_getxattr() enable the security module to override/replace the actual security xattr value returned to userspace.

Quoted 7 lines

In the case of Smack, Smack could just provide a pointer to its own internal copy of the string, and that could be stored in the wrapped iattr. In the case of SELinux, we could provide a secid that could be stored in the wrapped iattr. The hook interface could just handle it as a blob if you prefer. But either way we don't need extra storage aside from a pointer-size field in the wrapped iattr.

So how often is the SELinux label going to get explicitly set in /sys ? I'm grappling with the value of going hog-wild in optimizing this. If it is something that's quite rare I can see the concern with expanding the d_entry. If it is common, the storage associated with storing the xattr could be an issue. If it is uncommon but not rare there's another story again.

I'm looking at addressing the issues. Thank you.

...

Quoted 7 lines

So how often is the SELinux label going to get explicitly set in /sys ? I'm grappling with the value of going hog-wild in optimizing this. If it is something that's quite rare I can see the concern with expanding the d_entry. If it is common, the storage associated with storing the xattr could be an issue. If it is uncommon but not rare there's another story again.

Quoted 2 lines

I'm looking at addressing the issues. Thank you.

I'd expect most sysfs nodes to be left in the default label, although we don't really know as this would be the first time that people have the option of finer-grained control to sysfs.

Quoted 86 lines · Casey Schaufler

Quoted 69 lines · Stephen Smalley

Quoted 51 lines · Casey Schaufler

Quoted 39 lines · Stephen Smalley

Quoted 25 lines · Casey Schaufler

From: Casey Schaufler <[email protected]>

Another approach to limited xattr support in sysfs.

I tried to listen to the objections to a linked list representation and I think that I understand that there isn't really any interest in supporting xattrs for real, only for those maintained by LSMs. I also looked carefully into the claims that memory usage is critical and that the code I had before was duplicating effort.

This version lets the surrounding code do as much of the work as possible. Unlike the initial proposal for sysfs xattrs, it does not introduce any new LSM hooks, it uses hooks that already exist. It does not support any attributes on its own, it only provides for the attribute advertised by security_inode_listsecurity(). It could easily be used by other filesystems to provide the same LSM xattr support. It could also be extended to do the list based support for arbitrary xattrs without too much effort.

Probably the oddest bit is that the inode_getsecurity hooks need to check to see if they are getting called before the inode is instantiated and return -ENODATA in that event. It would be possible to do a filesystem specific check instead, but this way provides for generally correct behavior at small cost.

Quoted 10 lines

This has been tested with Smack, but not SELinux. I think that SELinux will work correctly, but it could be that a labeling behavior that is different than the "usual" instantiation labeling is actually desired. That would be an easy change.

As always, let me know if I missed something obvious or if there's a fatal flaw in the scheme.

Quoted 9 lines

The point of the David's patch was to provide a way to save the security xattr in the backing data structure for sysfs entries when an attribute value is set from userspace so that the value can be preserved if the inode is evicted from memory and later re-instantiated. AFAICS, your patch completely misses the problem. How about we just go back to David's patch?

Quoted 8 lines

Oh no, that would use too much memory!

Either you care about the value the user set, in which case you want to save the value the user set, or you don't. If you do, you have to save that value, not an LSM's interpretation of that value. No secids. No new hooks.

As the security module is the only component of the kernel that uses/interprets that value, it isn't unreasonable for the security module's interpretation of that value to be considered canonical. In fact, that is already the case - the hooks within vfs_getxattr() enable the security module to override/replace the actual security xattr value returned to userspace.

Quoted 8 lines

In the case of Smack, Smack could just provide a pointer to its own internal copy of the string, and that could be stored in the wrapped iattr. In the case of SELinux, we could provide a secid that could be stored in the wrapped iattr. The hook interface could just handle it as a blob if you prefer. But either way we don't need extra storage aside from a pointer-size field in the wrapped iattr.

So how often is the SELinux label going to get explicitly set in /sys ? I'm grappling with the value of going hog-wild in optimizing this. If it is something that's quite rare I can see the concern with expanding the d_entry. If it is common, the storage associated with storing the xattr could be an issue. If it is uncommon but not rare there's another story again.

Quoted 2 lines

I'm looking at addressing the issues. Thank you.

I'd expect most sysfs nodes to be left in the default label, although we don't really know as this would be the first time that people have the option of finer-grained control to sysfs.

So I still don't like the way it exposes LSM internal data to the file system code, but given how long it's taking for me to create a better solution I don't think that I can in all fairness say NAK to David Quigley's sysfs patch any longer. I withdraw my objection, while maintaining my reservations.

Quoted 5 lines · Casey Schaufler

So I still don't like the way it exposes LSM internal data to the file system code, but given how long it's taking for me to create a better solution I don't think that I can in all fairness say NAK to David Quigley's sysfs patch any longer. I withdraw my objection, while maintaining my reservations.

Until I see it wired up against another filesystem I retain my objections. When I asked he pretty much told me that it doesn't generalize to other filesystems well and it is a sysfs special case.

The way sysctl and proc are wired as special cases into the lsm has been a maintenance disaster so far, and I think it a very bad idea to add yet another lsm special case, that supports only one filesystem.

Since Casey has withdrawn his NAK for the patch I guess the only other concern was about the generality of the solution from Eric. Did Steve's response adequately address this or are there any other questions that people need answered before Greg can take the patch.