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.
This is embedded into the sysfs_dirent structure so it remains persistent even
if the inode structures are evicted from the cache. 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.
This patch addresses an issue where SELinux was denying KVM 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 KVM
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 | 21 +++++++++++++++++++++
fs/sysfs/symlink.c | 2 ++
fs/sysfs/sysfs.h | 3 +++
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, 131 insertions(+), 1 deletions(-)
diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
index d88d0fa..fc21682 100644
--- a/fs/sysfs/dir.c
+++ b/fs/sysfs/dir.c
@@ -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)
diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
index 555f0ff..9f8b760 100644
--- a/fs/sysfs/inode.c
+++ b/fs/sysfs/inode.c
@@ -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)
@@ -104,6 +106,23 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
return error;
}
+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;
+
+ error = security_xattr_to_secid(name, value, size, &secid);
+ if (!error)
+ sd->s_secid = secid;
+
+ return error;
+}
+
static inline void set_default_inode_attr(struct inode * inode, mode_t mode)
{
inode->i_mode = mode;
@@ -163,6 +182,8 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
} else
set_default_inode_attr(inode, sd->s_mode);
+ if (sd->s_secid)
+ security_inode_setsecid(inode, sd->s_secid);
/* initialize inode according to type */
switch (sysfs_type(sd)) {
diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
index 1d897ad..c5081ad 100644
--- a/fs/sysfs/symlink.c
+++ b/fs/sysfs/symlink.c
@@ -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,
diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
index 3fa0d98..732d183 100644
--- a/fs/sysfs/sysfs.h
+++ b/fs/sysfs/sysfs.h
@@ -57,6 +57,7 @@ struct sysfs_dirent {
ino_t s_ino;
umode_t s_mode;
struct iattr *s_iattr;
+ u32 s_secid;
};
#define SD_DEACTIVATED_BIAS INT_MIN
@@ -148,6 +149,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);
diff --git a/include/linux/security.h b/include/linux/security.h
index 1459091..35ecc8d 100644
--- a/include/linux/security.h
+++ b/include/linux/security.h
@@ -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;
diff --git a/security/capability.c b/security/capability.c
index f218dd3..a3f3d5b 100644
--- a/security/capability.c
+++ b/security/capability.c
@@ -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);
diff --git a/security/security.c b/security/security.c
index 4501c5e..8313e15 100644
--- a/security/security.c
+++ b/security/security.c
@@ -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);
diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
index 2081055..395c36d 100644
--- a/security/selinux/hooks.c
+++ b/security/selinux/hooks.c
@@ -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,
diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
index 1c9bdbc..be66c8e 100644
--- a/security/smack/smack_lsm.c
+++ b/security/smack/smack_lsm.c
@@ -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
*/
@@ -3062,7 +3100,9 @@ struct security_operations smack_ops = {
.inode_setsecurity = smack_inode_setsecurity,
.inode_listsecurity = smack_inode_listsecurity,
.inode_getsecid = smack_inode_getsecid,
-
+ .inode_setsecid = smack_inode_setsecid,
+ .xattr_to_secid = smack_xattr_to_secid,
+
.file_permission = smack_file_permission,
.file_alloc_security = smack_file_alloc_security,
.file_free_security = smack_file_free_security,
--
1.5.6.6
David P. Quigley wrote:
> 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.
> This is embedded into the sysfs_dirent structure so it remains persistent even
> if the inode structures are evicted from the cache. 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.
>
Nacked-by: Casey Schaufler <[email protected]>
I'm all for sysfs supporting xattrs.
I am completely opposed to secids as file system metadata.
What do you get when you do an ls -Z?
An LSM must not be beholden to exposing transient internal
representations of security data to userspace, which is what
you're doing here. An LSM gets to decide what the security
information it maintains looks like by defining a security blob.
If you want this in, implement xattrs in sysfs for real. Smack
depends on the existing, published, and supported xattr interfaces
for dealing with getting and setting the values. Not secids.
Smack maintains secids because labeled networking and audit require
them, and they got there first.
> This patch addresses an issue where SELinux was denying KVM 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 KVM
> 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 | 21 +++++++++++++++++++++
> fs/sysfs/symlink.c | 2 ++
> fs/sysfs/sysfs.h | 3 +++
> 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, 131 insertions(+), 1 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index d88d0fa..fc21682 100644
> --- a/fs/sysfs/dir.c
> +++ b/fs/sysfs/dir.c
> @@ -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)
> diff --git a/fs/sysfs/inode.c b/fs/sysfs/inode.c
> index 555f0ff..9f8b760 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -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)
> @@ -104,6 +106,23 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> return error;
> }
>
> +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;
> +
> + error = security_xattr_to_secid(name, value, size, &secid);
> + if (!error)
> + sd->s_secid = secid;
> +
> + return error;
> +}
> +
> static inline void set_default_inode_attr(struct inode * inode, mode_t mode)
> {
> inode->i_mode = mode;
> @@ -163,6 +182,8 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
> } else
> set_default_inode_attr(inode, sd->s_mode);
>
> + if (sd->s_secid)
> + security_inode_setsecid(inode, sd->s_secid);
>
> /* initialize inode according to type */
> switch (sysfs_type(sd)) {
> diff --git a/fs/sysfs/symlink.c b/fs/sysfs/symlink.c
> index 1d897ad..c5081ad 100644
> --- a/fs/sysfs/symlink.c
> +++ b/fs/sysfs/symlink.c
> @@ -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,
> diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> index 3fa0d98..732d183 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -57,6 +57,7 @@ struct sysfs_dirent {
> ino_t s_ino;
> umode_t s_mode;
> struct iattr *s_iattr;
> + u32 s_secid;
> };
>
> #define SD_DEACTIVATED_BIAS INT_MIN
> @@ -148,6 +149,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);
>
> diff --git a/include/linux/security.h b/include/linux/security.h
> index 1459091..35ecc8d 100644
> --- a/include/linux/security.h
> +++ b/include/linux/security.h
> @@ -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;
> diff --git a/security/capability.c b/security/capability.c
> index f218dd3..a3f3d5b 100644
> --- a/security/capability.c
> +++ b/security/capability.c
> @@ -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);
> diff --git a/security/security.c b/security/security.c
> index 4501c5e..8313e15 100644
> --- a/security/security.c
> +++ b/security/security.c
> @@ -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);
> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> index 2081055..395c36d 100644
> --- a/security/selinux/hooks.c
> +++ b/security/selinux/hooks.c
> @@ -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,
> diff --git a/security/smack/smack_lsm.c b/security/smack/smack_lsm.c
> index 1c9bdbc..be66c8e 100644
> --- a/security/smack/smack_lsm.c
> +++ b/security/smack/smack_lsm.c
> @@ -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
> */
> @@ -3062,7 +3100,9 @@ struct security_operations smack_ops = {
> .inode_setsecurity = smack_inode_setsecurity,
> .inode_listsecurity = smack_inode_listsecurity,
> .inode_getsecid = smack_inode_getsecid,
> -
> + .inode_setsecid = smack_inode_setsecid,
> + .xattr_to_secid = smack_xattr_to_secid,
> +
> .file_permission = smack_file_permission,
> .file_alloc_security = smack_file_alloc_security,
> .file_free_security = smack_file_free_security,
>
On Wed, 2009-07-08 at 18:44 -0700, Casey Schaufler wrote:
> An LSM must not be beholden to exposing transient internal
> representations of security data to userspace, which is what
> you're doing here. An LSM gets to decide what the security
> information it maintains looks like by defining a security blob.
Something worth saying is that the sysfs_dirent is already a transient
internal represenation.
On Wed, 2009-07-08 at 18:44 -0700, Casey Schaufler wrote:
> David P. Quigley wrote:
> > 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.
> > This is embedded into the sysfs_dirent structure so it remains persistent even
> > if the inode structures are evicted from the cache. 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.
> >
>
> Nacked-by: Casey Schaufler <[email protected]>
>
> I'm all for sysfs supporting xattrs.
>
> I am completely opposed to secids as file system metadata.
>
> What do you get when you do an ls -Z?
>
> An LSM must not be beholden to exposing transient internal
> representations of security data to userspace, which is what
> you're doing here. An LSM gets to decide what the security
> information it maintains looks like by defining a security blob.
>
> If you want this in, implement xattrs in sysfs for real. Smack
> depends on the existing, published, and supported xattr interfaces
> for dealing with getting and setting the values. Not secids.
> Smack maintains secids because labeled networking and audit require
> them, and they got there first.
>
>
So are you proposing that we embed a variable length string in the
sysfs_dirent structure because that sounds completely silly. It seems
completely reasonable here to take the blob coming in and have the LSM
turn it into a handle that is efficiently referenced by the
sysfs_dirent. The problem here is that sysfs entries have no backing
store at all which means everything we do will have to be added to
sysfs_dirent. I'm pretty sure we don't want to be doing lifecycle
management on strings inside this structure considering the only other
string I see is marked const. If you have a better way of doing this I'm
interested in hearing it but it doesn't seem reasonable to be storing
the xattr itself in the sysfs_dirent. I'd like to hear what Greg thinks
about that.
Dave
David P. Quigley wrote:
> On Wed, 2009-07-08 at 18:44 -0700, Casey Schaufler wrote:
>
>> David P. Quigley wrote:
>>
>>> 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.
>>> This is embedded into the sysfs_dirent structure so it remains persistent even
>>> if the inode structures are evicted from the cache. 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.
>>>
>>>
>> Nacked-by: Casey Schaufler <[email protected]>
>>
>> I'm all for sysfs supporting xattrs.
>>
>> I am completely opposed to secids as file system metadata.
>>
>> What do you get when you do an ls -Z?
>>
>> An LSM must not be beholden to exposing transient internal
>> representations of security data to userspace, which is what
>> you're doing here. An LSM gets to decide what the security
>> information it maintains looks like by defining a security blob.
>>
>> If you want this in, implement xattrs in sysfs for real. Smack
>> depends on the existing, published, and supported xattr interfaces
>> for dealing with getting and setting the values. Not secids.
>> Smack maintains secids because labeled networking and audit require
>> them, and they got there first.
>>
>>
>>
>
> So are you proposing that we embed a variable length string in the
> sysfs_dirent structure because that sounds completely silly.
No, I'm not proposing that because it sounds silly, I'm proposing it
because that's the way xattrs work on Linux.
> It seems
> completely reasonable here to take the blob coming in and have the LSM
> turn it into a handle that is efficiently referenced by the
> sysfs_dirent. The problem here is that sysfs entries have no backing
> store at all which means everything we do will have to be added to
> sysfs_dirent. I'm pretty sure we don't want to be doing lifecycle
> management on strings inside this structure considering the only other
> string I see is marked const. If you have a better way of doing this I'm
> interested in hearing it but it doesn't seem reasonable to be storing
> the xattr itself in the sysfs_dirent.
Smack depends on the xattr interfaces to inspect and manipulate labels
on file system objects. Now you have a file system that "supports"
xattrs, but not the xattr interfaces. What if I want to change the
label on a sysfs entry? Or even read it? I can't with your scheme.
You are proposing a one-off hack to solve a particular problem. It
introduces issues of its own. I don't care that it is clever and
compact. It's not right.
> I'd like to hear what Greg thinks
> about that.
>
> Dave
>
>
>
On Thu, 2009-07-09 at 07:49 -0700, Casey Schaufler wrote:
> David P. Quigley wrote:
> > On Wed, 2009-07-08 at 18:44 -0700, Casey Schaufler wrote:
> >
> >> David P. Quigley wrote:
> >>
> >>> 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.
> >>> This is embedded into the sysfs_dirent structure so it remains persistent even
> >>> if the inode structures are evicted from the cache. 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.
> >>>
> >>>
> >> Nacked-by: Casey Schaufler <[email protected]>
> >>
> >> I'm all for sysfs supporting xattrs.
> >>
> >> I am completely opposed to secids as file system metadata.
> >>
> >> What do you get when you do an ls -Z?
> >>
> >> An LSM must not be beholden to exposing transient internal
> >> representations of security data to userspace, which is what
> >> you're doing here. An LSM gets to decide what the security
> >> information it maintains looks like by defining a security blob.
> >>
> >> If you want this in, implement xattrs in sysfs for real. Smack
> >> depends on the existing, published, and supported xattr interfaces
> >> for dealing with getting and setting the values. Not secids.
> >> Smack maintains secids because labeled networking and audit require
> >> them, and they got there first.
> >>
> >>
> >>
> >
> > So are you proposing that we embed a variable length string in the
> > sysfs_dirent structure because that sounds completely silly.
>
> No, I'm not proposing that because it sounds silly, I'm proposing it
> because that's the way xattrs work on Linux.
>
> > It seems
> > completely reasonable here to take the blob coming in and have the LSM
> > turn it into a handle that is efficiently referenced by the
> > sysfs_dirent. The problem here is that sysfs entries have no backing
> > store at all which means everything we do will have to be added to
> > sysfs_dirent. I'm pretty sure we don't want to be doing lifecycle
> > management on strings inside this structure considering the only other
> > string I see is marked const. If you have a better way of doing this I'm
> > interested in hearing it but it doesn't seem reasonable to be storing
> > the xattr itself in the sysfs_dirent.
>
> Smack depends on the xattr interfaces to inspect and manipulate labels
> on file system objects. Now you have a file system that "supports"
> xattrs, but not the xattr interfaces. What if I want to change the
> label on a sysfs entry? Or even read it? I can't with your scheme.
>
> You are proposing a one-off hack to solve a particular problem. It
> introduces issues of its own. I don't care that it is clever and
> compact. It's not right.
>
Why can't you use that? I used the set and get xattr interfaces to test
the code and it worked just fine. I agree with you that generic xattr
support for all pseudo file systems would be great but we don't see a
usecase for xattr support on sysfs other than in the security namespace.
Now we have a file system here that is not persistent across reboots and
we may only have one LSM active at a given time. You can try to have an
xattr table embedded into the sysfs_dirent structure but it is counter
the problem that is trying to be solved. The reason why we can't just
use the inode for the entry to begin with is that under memory pressure
those inodes can and will be evicted. Given that adding a method that
with the potential to add lots of memory pressure to the system seems
counterproductive. Remember those sysfs_dirent entries have to exist for
the lifetime of the sysfs filesystem. This means that you have the
potential for full page size attributes to be in each and every
sysfs_dirent structure.
The solution we have here says you specify and retrieve the security
blob using the normal xattr format but internally for memory reasons the
LSM is expected to map that blob to a unique identifier which is stored
in the sysfs_dirent. In the case of Smack this is practically a null
translation since your secid is the pointer to your actual label. In
SELinux the initial translation (luckily only done once) is a little
heavy weight but refreshing the inode after that is not. This mapping
can be transient because these xattrs are not persistent and neither is
the file system they are on.
Dave
On Thu, 2009-07-09 at 07:49 -0700, Casey Schaufler wrote:
>
> Smack depends on the xattr interfaces to inspect and manipulate labels
> on file system objects. Now you have a file system that "supports"
> xattrs, but not the xattr interfaces. What if I want to change the
> label on a sysfs entry? Or even read it? I can't with your scheme.
>
> You are proposing a one-off hack to solve a particular problem. It
> introduces issues of its own. I don't care that it is clever and
> compact. It's not right.
getfattr -d -m security.* /sys/fs/
getfattr: Removing leading '/' from absolute path names
# file: sys/fs/
security.selinux="system_u:object_r:sysfs_t:s0\000"
# ls -Z
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 block
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 bus
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 class
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 dev
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 devices
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 firmware
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 fs
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 hypervisor
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 kernel
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 module
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 power
# setfattr -n security.selinux -v "system_u:object_r:usr_t:s0
\000" /sys/fs/
# ls -Z
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 block
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 bus
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 class
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 dev
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 devices
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 firmware
drwxr-xr-x root root system_u:object_r:usr_t:s0 fs
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 hypervisor
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 kernel
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 module
drwxr-xr-x root root system_u:object_r:sysfs_t:s0 power
Now you can argue that it doesn't have the user.* name space or other
name spaces but a file system doesn't have to implement every xattr name
space. We are only implementing the security name space here and your
objection of the xattr interface not being maintained doesn't hold. If
someone wants to go through and do generic xattr support for
non-persistent file systems I welcome that but it's unclear to me what
use case supports that kind of memory usage.
On Thu, Jul 09, 2009 at 10:05:06AM -0400, David P. Quigley wrote:
> So are you proposing that we embed a variable length string in the
> sysfs_dirent structure because that sounds completely silly. It seems
> completely reasonable here to take the blob coming in and have the LSM
> turn it into a handle that is efficiently referenced by the
> sysfs_dirent. The problem here is that sysfs entries have no backing
> store at all which means everything we do will have to be added to
> sysfs_dirent. I'm pretty sure we don't want to be doing lifecycle
> management on strings inside this structure considering the only other
> string I see is marked const. If you have a better way of doing this I'm
> interested in hearing it but it doesn't seem reasonable to be storing
> the xattr itself in the sysfs_dirent. I'd like to hear what Greg thinks
> about that.
I think you all better agree on the proposed solution before I will
accept any changes to the sysfs core code :)
good luck,
greg k-h
On Wed, Jul 08, 2009 at 01:28:26PM -0400, David P. Quigley wrote:
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -57,6 +57,7 @@ struct sysfs_dirent {
> ino_t s_ino;
> umode_t s_mode;
> struct iattr *s_iattr;
> + u32 s_secid;
> };
Why not just make this a void * like all other security hooks, and then
you and SMACK can pick and choose what you want to embed here?
thanks,
greg k-h
On Thu, 2009-07-09 at 08:18 -0700, Greg KH wrote:
> On Wed, Jul 08, 2009 at 01:28:26PM -0400, David P. Quigley wrote:
> > --- a/fs/sysfs/sysfs.h
> > +++ b/fs/sysfs/sysfs.h
> > @@ -57,6 +57,7 @@ struct sysfs_dirent {
> > ino_t s_ino;
> > umode_t s_mode;
> > struct iattr *s_iattr;
> > + u32 s_secid;
> > };
>
> Why not just make this a void * like all other security hooks, and then
> you and SMACK can pick and choose what you want to embed here?
>
> thanks,
>
> greg k-h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
The issue is that there really aren't any LSM hooks to accommodate that.
I have a few LSM hooks for the Labeled NFS work which could be used for
this but it still requires us to store the full xattr value somewhere
and referencing it in the sysfs_dirent structure. The issue here is that
there are two ways of presenting security information. The first is
through the xattr interface which represents the security information as
an opaque blob which the LSM turns into an internal representation. The
second which is left over from the early days is the secid which I
equate to a file handle. The problem I see is that the opaque blob (the
xattr) is the interface presented to user space. It isn't really used
internally except to turn it into a data structure or to write it to
disk for persistence.
The situation we have with sysfs is that there is no persistence for
labels and the in-core inode maybe evicted so we need a way of
persisting changes from the default label. What is really need here is
a way of persisting the security structure maintained by the LSM. Since
these structures are contained in the LSM the only reasonable
abstraction for this is for the LSM to provide a handle to refer to the
structure. There are two ways of doing this. One is with a large string
(the xattr) and the other is with a light weight handle (the secid).
Dave
I just read over Casey's comments again and I'm pretty sure we have a
big misunderstanding here. From his initial response it seems that he
thinks that I am exposing the secids to userspace as the way for setting
the labels on files. That isn't true. We are still using the full string
based labels for the userspace interface what the secid is used for is
to allow the kernel to keep track of changes until the sysfs_dirent is
destroyed.
Dave
On Thu, Jul 09, 2009 at 01:26:44PM -0400, David P. Quigley wrote:
> I just read over Casey's comments again and I'm pretty sure we have a
> big misunderstanding here. From his initial response it seems that he
> thinks that I am exposing the secids to userspace as the way for setting
> the labels on files. That isn't true. We are still using the full string
> based labels for the userspace interface what the secid is used for is
> to allow the kernel to keep track of changes until the sysfs_dirent is
> destroyed.
Ok, if Casey and others agree that this is the best solution, I'll take
it.
thanks,
greg k-h
On Thu, Jul 09, 2009 at 01:13:33PM -0400, David P. Quigley wrote:
> The issue is that there really aren't any LSM hooks to accommodate that.
> I have a few LSM hooks for the Labeled NFS work which could be used for
> this but it still requires us to store the full xattr value somewhere
> and referencing it in the sysfs_dirent structure.
A void pointer would handle that properly, right?
> The issue here is that there are two ways of presenting security
> information. The first is through the xattr interface which represents
> the security information as an opaque blob which the LSM turns into an
> internal representation. The second which is left over from the early
> days is the secid which I equate to a file handle. The problem I see
> is that the opaque blob (the xattr) is the interface presented to user
> space. It isn't really used internally except to turn it into a data
> structure or to write it to disk for persistence.
That is the way that selinux does it, do the other lsms also handle it
this way?
> The situation we have with sysfs is that there is no persistence for
> labels and the in-core inode maybe evicted so we need a way of
> persisting changes from the default label.
So you put it in the structure you did, which is correct. You should
also listen to all sysfs netlink messages to be sure to lable things
when they are created, to handle the lack of persistence.
thanks,
greg k-h
On Thu, 2009-07-09 at 10:52 -0700, Greg KH wrote:
> On Thu, Jul 09, 2009 at 01:13:33PM -0400, David P. Quigley wrote:
> > The issue is that there really aren't any LSM hooks to accommodate that.
> > I have a few LSM hooks for the Labeled NFS work which could be used for
> > this but it still requires us to store the full xattr value somewhere
> > and referencing it in the sysfs_dirent structure.
>
> A void pointer would handle that properly, right?
A void pointer would suffice if we wanted to store the opaque blob. My
argument is that storing that blob is too heavy weight memory wise.
>
> > The issue here is that there are two ways of presenting security
> > information. The first is through the xattr interface which represents
> > the security information as an opaque blob which the LSM turns into an
> > internal representation. The second which is left over from the early
> > days is the secid which I equate to a file handle. The problem I see
> > is that the opaque blob (the xattr) is the interface presented to user
> > space. It isn't really used internally except to turn it into a data
> > structure or to write it to disk for persistence.
>
> That is the way that selinux does it, do the other lsms also handle it
> this way?
Casey handles this a different way in Smack but it has more to do with
his model than his design. Since a Smack label is just a simple 23 byte
string he doesn't do any conversion to store it in Smack. SELinux
differs in that the label contains 4 components so these get broken out
into the security structure so they can be handled separately by the
security structure. I can't say for certain but I would probably say
that a label based LSM which attempts to implement several models will
probably have to do what SELinux does. The only thing I'm concerned with
is that Casey did mention when I was creating hooks for the Labeled NFS
work a situation where an LSM may implement multiple security.* xattrs.
We don't currently have any LSMs that work that way so I'm not sure if I
need to handle that now.
>
> > The situation we have with sysfs is that there is no persistence for
> > labels and the in-core inode maybe evicted so we need a way of
> > persisting changes from the default label.
>
> So you put it in the structure you did, which is correct. You should
> also listen to all sysfs netlink messages to be sure to lable things
> when they are created, to handle the lack of persistence.
Thanks for the heads up. I'll make sure I look into this.
>
> thanks,
>
> greg k-h
On Thu, 2009-07-09 at 10:50 -0700, Greg KH wrote:
> On Thu, Jul 09, 2009 at 01:26:44PM -0400, David P. Quigley wrote:
> > I just read over Casey's comments again and I'm pretty sure we have a
> > big misunderstanding here. From his initial response it seems that he
> > thinks that I am exposing the secids to userspace as the way for setting
> > the labels on files. That isn't true. We are still using the full string
> > based labels for the userspace interface what the secid is used for is
> > to allow the kernel to keep track of changes until the sysfs_dirent is
> > destroyed.
>
> Ok, if Casey and others agree that this is the best solution, I'll take
> it.
>
> thanks,
>
> greg k-h
I haven't heard from Casey since his last email so I'd hold off on
taking this until we come to an agreement. It seems though from your
comments in another mail that putting the persistent data into the
sysfs_dirent is the proper approach and we just need to figure out what
to put there.
Dave
On Thu, Jul 09, 2009 at 03:28:58PM -0400, David P. Quigley wrote:
> On Thu, 2009-07-09 at 10:52 -0700, Greg KH wrote:
> > On Thu, Jul 09, 2009 at 01:13:33PM -0400, David P. Quigley wrote:
> > > The issue is that there really aren't any LSM hooks to accommodate that.
> > > I have a few LSM hooks for the Labeled NFS work which could be used for
> > > this but it still requires us to store the full xattr value somewhere
> > > and referencing it in the sysfs_dirent structure.
> >
> > A void pointer would handle that properly, right?
>
> A void pointer would suffice if we wanted to store the opaque blob. My
> argument is that storing that blob is too heavy weight memory wise.
You could use that void pointer to store your id with no memory
difference at all, so why would it be "heavy weight"? It shoud be
identical, right?
thanks,
greg k-h
On Thu, Jul 09, 2009 at 03:32:56PM -0400, David P. Quigley wrote:
> On Thu, 2009-07-09 at 10:50 -0700, Greg KH wrote:
> > On Thu, Jul 09, 2009 at 01:26:44PM -0400, David P. Quigley wrote:
> > > I just read over Casey's comments again and I'm pretty sure we have a
> > > big misunderstanding here. From his initial response it seems that he
> > > thinks that I am exposing the secids to userspace as the way for setting
> > > the labels on files. That isn't true. We are still using the full string
> > > based labels for the userspace interface what the secid is used for is
> > > to allow the kernel to keep track of changes until the sysfs_dirent is
> > > destroyed.
> >
> > Ok, if Casey and others agree that this is the best solution, I'll take
> > it.
> >
> > thanks,
> >
> > greg k-h
>
>
> I haven't heard from Casey since his last email so I'd hold off on
> taking this until we come to an agreement. It seems though from your
> comments in another mail that putting the persistent data into the
> sysfs_dirent is the proper approach and we just need to figure out what
> to put there.
Yes, I think the approach is proper, you all just need to agree what it
is you are putting there.
thanks,
greg k-h
On Thu, 2009-07-09 at 13:12 -0700, Greg KH wrote:
> On Thu, Jul 09, 2009 at 03:28:58PM -0400, David P. Quigley wrote:
> > On Thu, 2009-07-09 at 10:52 -0700, Greg KH wrote:
> > > On Thu, Jul 09, 2009 at 01:13:33PM -0400, David P. Quigley wrote:
> > > > The issue is that there really aren't any LSM hooks to accommodate that.
> > > > I have a few LSM hooks for the Labeled NFS work which could be used for
> > > > this but it still requires us to store the full xattr value somewhere
> > > > and referencing it in the sysfs_dirent structure.
> > >
> > > A void pointer would handle that properly, right?
> >
> > A void pointer would suffice if we wanted to store the opaque blob. My
> > argument is that storing that blob is too heavy weight memory wise.
>
> You could use that void pointer to store your id with no memory
> difference at all, so why would it be "heavy weight"? It shoud be
> identical, right?
>
> thanks,
>
> greg k-h
Not quite. From the memory foot print inside sysfs_dirent it is the same
(baring a pointer being 64-bit on certain platforms) however that
pointer needs to be backed by something. The two current interfaces in
LSM to set an inode security attribute is 1) the xattr, and 2) a secid.
The issue is if we say some people can store the xattr and some can
store a secid I then don't know what hook to call to set the security
information on creation. Which basically boils down to if we have a void
* in sysfs_dirent it has to be the same void * semantics already used
which means it needs to have a string with the label backing it. This
means in the SELinux case I'm now using memory for the string and the
structure in the security server which is a waste. It seems to me though
that It might be possible to make the size of the secid correspond to
the size of a pointer so Casey can return the address of his string as
the secid and we can return the secid. However, it still needs to have
the semantics of the secid and not the opaque void *.
I could be wrong about this but at the moment the mismatch of semantics
between the two seem to be a problem. If I'm missing anything feel free
to point it out.
On Thu, Jul 09, 2009 at 04:19:45PM -0400, David P. Quigley wrote:
> On Thu, 2009-07-09 at 13:12 -0700, Greg KH wrote:
> > On Thu, Jul 09, 2009 at 03:28:58PM -0400, David P. Quigley wrote:
> > > On Thu, 2009-07-09 at 10:52 -0700, Greg KH wrote:
> > > > On Thu, Jul 09, 2009 at 01:13:33PM -0400, David P. Quigley wrote:
> > > > > The issue is that there really aren't any LSM hooks to accommodate that.
> > > > > I have a few LSM hooks for the Labeled NFS work which could be used for
> > > > > this but it still requires us to store the full xattr value somewhere
> > > > > and referencing it in the sysfs_dirent structure.
> > > >
> > > > A void pointer would handle that properly, right?
> > >
> > > A void pointer would suffice if we wanted to store the opaque blob. My
> > > argument is that storing that blob is too heavy weight memory wise.
> >
> > You could use that void pointer to store your id with no memory
> > difference at all, so why would it be "heavy weight"? It shoud be
> > identical, right?
> >
> > thanks,
> >
> > greg k-h
>
>
> Not quite. From the memory foot print inside sysfs_dirent it is the same
> (baring a pointer being 64-bit on certain platforms) however that
> pointer needs to be backed by something. The two current interfaces in
> LSM to set an inode security attribute is 1) the xattr, and 2) a secid.
> The issue is if we say some people can store the xattr and some can
> store a secid I then don't know what hook to call to set the security
> information on creation. Which basically boils down to if we have a void
> * in sysfs_dirent it has to be the same void * semantics already used
> which means it needs to have a string with the label backing it. This
> means in the SELinux case I'm now using memory for the string and the
> structure in the security server which is a waste. It seems to me though
> that It might be possible to make the size of the secid correspond to
> the size of a pointer so Casey can return the address of his string as
> the secid and we can return the secid. However, it still needs to have
> the semantics of the secid and not the opaque void *.
>
> I could be wrong about this but at the moment the mismatch of semantics
> between the two seem to be a problem. If I'm missing anything feel free
> to point it out.
No, that makes sense as well. I'll let you and Casey work it out :)
good luck,
greg k-h
David P. Quigley wrote:
> On Thu, 2009-07-09 at 10:50 -0700, Greg KH wrote:
>
>> On Thu, Jul 09, 2009 at 01:26:44PM -0400, David P. Quigley wrote:
>>
>>> I just read over Casey's comments again and I'm pretty sure we have a
>>> big misunderstanding here. From his initial response it seems that he
>>> thinks that I am exposing the secids to userspace as the way for setting
>>> the labels on files. That isn't true. We are still using the full string
>>> based labels for the userspace interface what the secid is used for is
>>> to allow the kernel to keep track of changes until the sysfs_dirent is
>>> destroyed.
>>>
>> Ok, if Casey and others agree that this is the best solution, I'll take
>> it.
>>
>> thanks,
>>
>> greg k-h
>>
>
>
> I haven't heard from Casey since his last email so I'd hold off on
> taking this until we come to an agreement.
Yeah. Pardon the day job.
> It seems though from your
> comments in another mail that putting the persistent data into the
> sysfs_dirent is the proper approach and we just need to figure out what
> to put there.
>
Now that I've really had a chance to review the patches carefully
my worst fears have been put to rest. I don't doubt that what you've
got will work any longer. I do object to using a secid, but I've had
to give in on that before.
If your secid is valid at any given time you have a context (which
is a text string) available at the same time that you can point to.
If this were not true a call to security_xattr_to_secid() could
not be counted on to succeed. You could define
security_xattr_to_secctx() and have it return the Smack value for
Smack and the context for SELinux instead of security_xattr_to_secid().
Sure, you've got a string to maintain, but it had better not be going
away in SELinux, because if it does the secid is going with it. Unless
I recall incorrectly (always a possibility) it has been some time
since the avc could really be considered a cache. I am willing to
bet beers that you could safely point to a mapping somewhere and
not worry much about it.
If not, you've got other performance issues in SELinux.
On Thu, 2009-07-09 at 20:25 -0700, Casey Schaufler wrote:
> David P. Quigley wrote:
> > On Thu, 2009-07-09 at 10:50 -0700, Greg KH wrote:
> >
> >> On Thu, Jul 09, 2009 at 01:26:44PM -0400, David P. Quigley wrote:
> >>
> >>> I just read over Casey's comments again and I'm pretty sure we have a
> >>> big misunderstanding here. From his initial response it seems that he
> >>> thinks that I am exposing the secids to userspace as the way for setting
> >>> the labels on files. That isn't true. We are still using the full string
> >>> based labels for the userspace interface what the secid is used for is
> >>> to allow the kernel to keep track of changes until the sysfs_dirent is
> >>> destroyed.
> >>>
> >> Ok, if Casey and others agree that this is the best solution, I'll take
> >> it.
> >>
> >> thanks,
> >>
> >> greg k-h
> >>
> >
> >
> > I haven't heard from Casey since his last email so I'd hold off on
> > taking this until we come to an agreement.
>
> Yeah. Pardon the day job.
>
> > It seems though from your
> > comments in another mail that putting the persistent data into the
> > sysfs_dirent is the proper approach and we just need to figure out what
> > to put there.
> >
>
> Now that I've really had a chance to review the patches carefully
> my worst fears have been put to rest. I don't doubt that what you've
> got will work any longer. I do object to using a secid, but I've had
> to give in on that before.
>
> If your secid is valid at any given time you have a context (which
> is a text string) available at the same time that you can point to.
> If this were not true a call to security_xattr_to_secid() could
> not be counted on to succeed. You could define
> security_xattr_to_secctx() and have it return the Smack value for
> Smack and the context for SELinux instead of security_xattr_to_secid().
> Sure, you've got a string to maintain, but it had better not be going
> away in SELinux, because if it does the secid is going with it. Unless
> I recall incorrectly (always a possibility) it has been some time
> since the avc could really be considered a cache. I am willing to
> bet beers that you could safely point to a mapping somewhere and
> not worry much about it.
>
> If not, you've got other performance issues in SELinux.
Sorry for the late reply but I was out of work on Friday. I'm not really
sure that I see the point of the xattr_to_secctx considering in theory
they should be the same exact thing. The patch was originally Steve's so
I don't know the full history of why it was done this way and
unfortunately he isn't around at the moment to comment on it. With your
hook what would you be storing in the sysfs_dirent? Are we back to
storing a string there?
Dave
Taking the conversation back on the list.
"David P. Quigley" <[email protected]> writes:
> On Sun, 2009-07-12 at 07:51 -0700, Eric W. Biederman wrote:
>> "David P. Quigley" <[email protected]> writes:
>>
>> > 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.
>> > This is embedded into the sysfs_dirent structure so it remains persistent even
>> > if the inode structures are evicted from the cache. 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.
>> >
>> > This patch addresses an issue where SELinux was denying KVM 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 KVM
>> > 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.
>>
>> You are talking about write access from KVM?
>>
>> How can direct hardware access to something that can do arbitrary
>> DMAs be secure?
>
> The bug in question is listed below.
>
> https://bugzilla.redhat.com/show_bug.cgi?id=499259
I see a discussion, but no discuss of the security of direct hardware
access of a DMA capable device.
>> > diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
>> > index 3fa0d98..732d183 100644
>> > --- a/fs/sysfs/sysfs.h
>> > +++ b/fs/sysfs/sysfs.h
>> > @@ -57,6 +57,7 @@ struct sysfs_dirent {
>> > ino_t s_ino;
>> > umode_t s_mode;
>> > struct iattr *s_iattr;
>> > + u32 s_secid;
>> > };
>>
>> Could you please expand s_iattr and store the secid there?
>> That is where all of the rest of the security information is
>> stored in sysfs.
>
> I'm sorry but doing that would make the security labels first class
> attributes. It was decided a long time ago that security labels are
> stored in xattrs and as such don't belong in the iattr structure. I
> tried placing the label in the iattr structure for the Labeled NFS code
> and Christoph told me to do it another way since he didn't find that
> approach acceptable. I'm assuming his response will be the same for a
> secid which is supposed to be very sparingly used outside of the
> security module.
What I mean is something like:
struct sysfs_iattr {
struct iattr s_iattr;
u32 s_secid;
};
struct sysfs_dirent {
...
ino_t s_ino;
umode_t s_mode;
struct sysfs_inode_attr *s_iattr;
};
The point is to simply allocate all of this optional stuff together,
and not use two fields in sysfs_dirent.
sysfs by default keeps a very sparse inode because it can assume
default values for all of the fields, and only bothers to keep
the extra fields when someone changes things explicitly. Like your
xattrs, the uid or the gid.
>> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> > index 2081055..395c36d 100644
>> > --- a/security/selinux/hooks.c
>> > +++ b/security/selinux/hooks.c
>> > @@ -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;
>>
>> What is this about? My impression is that if we have generic xattr
>> handling sysfs is not special so why do we have a special case?
>>
>> Is this interface appropriate for dealing with xattrs to all
>> linux virtual filesystmes similar to sysfs that do not currently
>> implement xattrs. aka debugfs, proc, etc?
>
> Even though sysfs has a setxattr handler the labeling behavior with
> respect to SELinux needs special handling. The idea here is that by
> default sysfs will be labeled across the board with the same label. The
> reason we can't use a normal style xattr handler here is because there
> is no original backing store to pull the label from. Only after a label
> has been changed is there a semi-persistent value that can be used for
> reinstantiating the inode in the case that it is pruned from the cache.
> Otherwise it falls back to the base genfs labeling of sysfs entries as
> sysfs_t.
Sounds like we want a mount option or the like here. Something explicit
in sysfs not something explicit in the security module.
I am also a bit dubious about
> Proc also has some special case handling in the SELinux module but I
> haven't had a chance to look at it and try to understand why. I don't
> think that this would be a general purpose solution for all pseudo file
> systems like you mentioned above but it may work for some of them. I'll
> look into them a bit more and then respond about them.
Sounds good. If we are going to expand the LSM it would be good to design
something decent instead of adding a nasty add-hoc case.
And on a silly note. Rumor has it that selinux has provable security.
If so what impact does this change make to the proofs?
Eric
[ Inline Comments...]
On Mon, 2009-07-13 at 09:50 -0700, Eric W. Biederman wrote:
> Taking the conversation back on the list.
>
> "David P. Quigley" <[email protected]> writes:
>
> > On Sun, 2009-07-12 at 07:51 -0700, Eric W. Biederman wrote:
> >> "David P. Quigley" <[email protected]> writes:
> >>
> >> > 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.
> >> > This is embedded into the sysfs_dirent structure so it remains persistent even
> >> > if the inode structures are evicted from the cache. 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.
> >> >
> >> > This patch addresses an issue where SELinux was denying KVM 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 KVM
> >> > 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.
> >>
> >> You are talking about write access from KVM?
> >>
> >> How can direct hardware access to something that can do arbitrary
> >> DMAs be secure?
> >
> > The bug in question is listed below.
> >
> > https://bugzilla.redhat.com/show_bug.cgi?id=499259
>
> I see a discussion, but no discuss of the security of direct hardware
> access of a DMA capable device.
So after reading through this again the problem isn't with KVM its with
libvirtd and other libvirt related programs.
>
> >> > diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
> >> > index 3fa0d98..732d183 100644
> >> > --- a/fs/sysfs/sysfs.h
> >> > +++ b/fs/sysfs/sysfs.h
> >> > @@ -57,6 +57,7 @@ struct sysfs_dirent {
> >> > ino_t s_ino;
> >> > umode_t s_mode;
> >> > struct iattr *s_iattr;
> >> > + u32 s_secid;
> >> > };
> >>
> >> Could you please expand s_iattr and store the secid there?
> >> That is where all of the rest of the security information is
> >> stored in sysfs.
> >
> > I'm sorry but doing that would make the security labels first class
> > attributes. It was decided a long time ago that security labels are
> > stored in xattrs and as such don't belong in the iattr structure. I
> > tried placing the label in the iattr structure for the Labeled NFS code
> > and Christoph told me to do it another way since he didn't find that
> > approach acceptable. I'm assuming his response will be the same for a
> > secid which is supposed to be very sparingly used outside of the
> > security module.
>
> What I mean is something like:
>
> struct sysfs_iattr {
> struct iattr s_iattr;
> u32 s_secid;
> };
>
> struct sysfs_dirent {
> ...
> ino_t s_ino;
> umode_t s_mode;
> struct sysfs_inode_attr *s_iattr;
> };
>
> The point is to simply allocate all of this optional stuff together,
> and not use two fields in sysfs_dirent.
>
> sysfs by default keeps a very sparse inode because it can assume
> default values for all of the fields, and only bothers to keep
> the extra fields when someone changes things explicitly. Like your
> xattrs, the uid or the gid.
Looking at the sysfs code I can see where the inode gets its default
values for everything but uid and gid. Are those set somewhere higher up
in the vfs on the init_inode path? The approach does seem reasonable but
do we want to have to allocate an entire iattr structure inside the
sysfs_inode_attr structure you propose just to store the secid?
>
> >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> > index 2081055..395c36d 100644
> >> > --- a/security/selinux/hooks.c
> >> > +++ b/security/selinux/hooks.c
> >> > @@ -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;
> >>
> >> What is this about? My impression is that if we have generic xattr
> >> handling sysfs is not special so why do we have a special case?
> >>
> >> Is this interface appropriate for dealing with xattrs to all
> >> linux virtual filesystmes similar to sysfs that do not currently
> >> implement xattrs. aka debugfs, proc, etc?
> >
> > Even though sysfs has a setxattr handler the labeling behavior with
> > respect to SELinux needs special handling. The idea here is that by
> > default sysfs will be labeled across the board with the same label. The
> > reason we can't use a normal style xattr handler here is because there
> > is no original backing store to pull the label from. Only after a label
> > has been changed is there a semi-persistent value that can be used for
> > reinstantiating the inode in the case that it is pruned from the cache.
> > Otherwise it falls back to the base genfs labeling of sysfs entries as
> > sysfs_t.
>
> Sounds like we want a mount option or the like here. Something explicit
> in sysfs not something explicit in the security module.
>
> I am also a bit dubious about
I don't think a mount option is the best thing here. Labeling behavior
is something that is LSM dependent and even file system dependent within
certain LSMs. Since I don't speak for Casey that the way SELinux handles
sysfs is the way he want's Smack handling sysfs, and we can't tell what
future label based LSMs will do I think leaving it to the module to
decide is best.
>
>
> > Proc also has some special case handling in the SELinux module but I
> > haven't had a chance to look at it and try to understand why. I don't
> > think that this would be a general purpose solution for all pseudo file
> > systems like you mentioned above but it may work for some of them. I'll
> > look into them a bit more and then respond about them.
>
> Sounds good. If we are going to expand the LSM it would be good to design
> something decent instead of adding a nasty add-hoc case.
A quick look over proc and debugfs leads me to believe that a generic
mechanism for all of them short of adding generic xattr support to all
pseudo file systems would be tricky at best and even more add-hoc than
what we already do. There isn't any uniformity in the data structures
that are used in these file systems so even if we came up with a lazy
update mechanism for these attributes it looks like the implementation
would vary greatly depending on the file system. Even then it doesn't
change the add-hoc nature of the functionality as we are only trying to
handle security attributes.
The real solution which is a lot of work and I don't exactly know how I
would go about putting it together is to try to provide a generic xattr
mechanism for pseudo file systems. However I don't have any use cases
for the majority of the xattr name spaces. The only thing we have at the
moment that needs attention and only on sysfs is the security.* name
space. So trying to implement full-blown xattrs on sysfs seems like a
bunch of effort with no clear user for it.
>
> And on a silly note. Rumor has it that selinux has provable security.
> If so what impact does this change make to the proofs?
>
I'm not a formal methods person so you would have to consult with the
people who did that work to find out.
> Eric
"David P. Quigley" <[email protected]> writes:
> [ Inline Comments...]
>
> On Mon, 2009-07-13 at 09:50 -0700, Eric W. Biederman wrote:
>> Taking the conversation back on the list.
>>
>> "David P. Quigley" <[email protected]> writes:
>>
>> > On Sun, 2009-07-12 at 07:51 -0700, Eric W. Biederman wrote:
>> >> "David P. Quigley" <[email protected]> writes:
>> >>
>> >> > 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.
>> >> > This is embedded into the sysfs_dirent structure so it remains persistent even
>> >> > if the inode structures are evicted from the cache. 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.
>> >> >
>> >> > This patch addresses an issue where SELinux was denying KVM 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 KVM
>> >> > 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.
>> >>
>> >> You are talking about write access from KVM?
>> >>
>> >> How can direct hardware access to something that can do arbitrary
>> >> DMAs be secure?
>> >
>> > The bug in question is listed below.
>> >
>> > https://bugzilla.redhat.com/show_bug.cgi?id=499259
>>
>> I see a discussion, but no discuss of the security of direct hardware
>> access of a DMA capable device.
>
> So after reading through this again the problem isn't with KVM its with
> libvirtd and other libvirt related programs.
>
>>
>> >> > diff --git a/fs/sysfs/sysfs.h b/fs/sysfs/sysfs.h
>> >> > index 3fa0d98..732d183 100644
>> >> > --- a/fs/sysfs/sysfs.h
>> >> > +++ b/fs/sysfs/sysfs.h
>> >> > @@ -57,6 +57,7 @@ struct sysfs_dirent {
>> >> > ino_t s_ino;
>> >> > umode_t s_mode;
>> >> > struct iattr *s_iattr;
>> >> > + u32 s_secid;
>> >> > };
>> >>
>> >> Could you please expand s_iattr and store the secid there?
>> >> That is where all of the rest of the security information is
>> >> stored in sysfs.
>> >
>> > I'm sorry but doing that would make the security labels first class
>> > attributes. It was decided a long time ago that security labels are
>> > stored in xattrs and as such don't belong in the iattr structure. I
>> > tried placing the label in the iattr structure for the Labeled NFS code
>> > and Christoph told me to do it another way since he didn't find that
>> > approach acceptable. I'm assuming his response will be the same for a
>> > secid which is supposed to be very sparingly used outside of the
>> > security module.
>>
>> What I mean is something like:
>>
>> struct sysfs_iattr {
>> struct iattr s_iattr;
>> u32 s_secid;
>> };
>>
>> struct sysfs_dirent {
>> ...
>> ino_t s_ino;
>> umode_t s_mode;
>> struct sysfs_inode_attr *s_iattr;
>> };
>>
>> The point is to simply allocate all of this optional stuff together,
>> and not use two fields in sysfs_dirent.
>>
>> sysfs by default keeps a very sparse inode because it can assume
>> default values for all of the fields, and only bothers to keep
>> the extra fields when someone changes things explicitly. Like your
>> xattrs, the uid or the gid.
>
> Looking at the sysfs code I can see where the inode gets its default
> values for everything but uid and gid. Are those set somewhere higher up
> in the vfs on the init_inode path? The approach does seem reasonable but
> do we want to have to allocate an entire iattr structure inside the
> sysfs_inode_attr structure you propose just to store the secid?
For a non-default secid. I think so. I assume we want to track
when the label was set and that requires timestamps.
If wind up allocating a lot of these things perhaps it will make
sense to do something different. But I expect the common case will be
few nodes in sysfs having non-default attributes.
>> >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>> >> > index 2081055..395c36d 100644
>> >> > --- a/security/selinux/hooks.c
>> >> > +++ b/security/selinux/hooks.c
>> >> > @@ -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;
>> >>
>> >> What is this about? My impression is that if we have generic xattr
>> >> handling sysfs is not special so why do we have a special case?
>> >>
>> >> Is this interface appropriate for dealing with xattrs to all
>> >> linux virtual filesystmes similar to sysfs that do not currently
>> >> implement xattrs. aka debugfs, proc, etc?
>> >
>> > Even though sysfs has a setxattr handler the labeling behavior with
>> > respect to SELinux needs special handling. The idea here is that by
>> > default sysfs will be labeled across the board with the same label. The
>> > reason we can't use a normal style xattr handler here is because there
>> > is no original backing store to pull the label from. Only after a label
>> > has been changed is there a semi-persistent value that can be used for
>> > reinstantiating the inode in the case that it is pruned from the cache.
>> > Otherwise it falls back to the base genfs labeling of sysfs entries as
>> > sysfs_t.
>>
>> Sounds like we want a mount option or the like here. Something explicit
>> in sysfs not something explicit in the security module.
>>
>> I am also a bit dubious about
>
> I don't think a mount option is the best thing here. Labeling behavior
> is something that is LSM dependent and even file system dependent within
> certain LSMs. Since I don't speak for Casey that the way SELinux handles
> sysfs is the way he want's Smack handling sysfs, and we can't tell what
> future label based LSMs will do I think leaving it to the module to
> decide is best.
Sure the source needs to be the lsm and not user space. The concept
however of setting a default at the beginning of time and having no
special cases beyond that seems reasonable.
After that beginning of time default you would not need special cases.
>> > Proc also has some special case handling in the SELinux module but I
>> > haven't had a chance to look at it and try to understand why. I don't
>> > think that this would be a general purpose solution for all pseudo file
>> > systems like you mentioned above but it may work for some of them. I'll
>> > look into them a bit more and then respond about them.
>>
>> Sounds good. If we are going to expand the LSM it would be good to design
>> something decent instead of adding a nasty add-hoc case.
>
> A quick look over proc and debugfs leads me to believe that a generic
> mechanism for all of them short of adding generic xattr support to all
> pseudo file systems would be tricky at best and even more add-hoc than
> what we already do. There isn't any uniformity in the data structures
> that are used in these file systems so even if we came up with a lazy
> update mechanism for these attributes it looks like the implementation
> would vary greatly depending on the file system. Even then it doesn't
> change the add-hoc nature of the functionality as we are only trying to
> handle security attributes.
So why should the lsm hooks care. That is what I am really after. A
common set of lsm hooks, and the lsm hooks shouldn't need to see all of
the data structures for those filesystems.
> The real solution which is a lot of work and I don't exactly know how I
> would go about putting it together is to try to provide a generic xattr
> mechanism for pseudo file systems. However I don't have any use cases
> for the majority of the xattr name spaces. The only thing we have at the
> moment that needs attention and only on sysfs is the security.* name
> space. So trying to implement full-blown xattrs on sysfs seems like a
> bunch of effort with no clear user for it.
Sort of. What we are taking about with this patch is generic xattr
support facing user space and the security modules. With an optimized
storage backend, that will only store well know attributes.
I think it makes sense to talk about a way to keep from growing lsm
special cases for each new virtual filesystem. Which means sysfs
and debugfs at least should share the same lsm hooks. Ideally
sysctl and proc would as well but they have special cases that
likely would break userspace if we changed things at the moment.
Eric
David P. Quigley wrote:
> Looking at the sysfs code I can see where the inode gets its default
> values for everything but uid and gid. Are those set somewhere higher up
> in the vfs on the init_inode path? The approach does seem reasonable but
> do we want to have to allocate an entire iattr structure inside the
> sysfs_inode_attr structure you propose just to store the secid?
>
>
>>>>> diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
>>>>> index 2081055..395c36d 100644
>>>>> --- a/security/selinux/hooks.c
>>>>> +++ b/security/selinux/hooks.c
>>>>> @@ -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;
>>>>>
>>>> What is this about? My impression is that if we have generic xattr
>>>> handling sysfs is not special so why do we have a special case?
>>>>
>>>> Is this interface appropriate for dealing with xattrs to all
>>>> linux virtual filesystmes similar to sysfs that do not currently
>>>> implement xattrs. aka debugfs, proc, etc?
>>>>
>>> Even though sysfs has a setxattr handler the labeling behavior with
>>> respect to SELinux needs special handling. The idea here is that by
>>> default sysfs will be labeled across the board with the same label. The
>>> reason we can't use a normal style xattr handler here is because there
>>> is no original backing store to pull the label from. Only after a label
>>> has been changed is there a semi-persistent value that can be used for
>>> reinstantiating the inode in the case that it is pruned from the cache.
>>> Otherwise it falls back to the base genfs labeling of sysfs entries as
>>> sysfs_t.
>>>
>> Sounds like we want a mount option or the like here. Something explicit
>> in sysfs not something explicit in the security module.
>>
>> I am also a bit dubious about
>>
>
> I don't think a mount option is the best thing here. Labeling behavior
> is something that is LSM dependent and even file system dependent within
> certain LSMs. Since I don't speak for Casey that the way SELinux handles
> sysfs is the way he want's Smack handling sysfs, and we can't tell what
> future label based LSMs will do I think leaving it to the module to
> decide is best.
>
>
My preference would be to stop having to do special case handling for
file systems that don't support xattrs. The obvious way to address the
situation would be generic support for xattrs in memory based file
systems. Yes, there would be bookkeeping to do, and yes, there would
be memory fragmentation issues to consider. But really, all we're
talking about is maintaining a list of name/value pairs on the
file system entry. The machinery for doing the work is all already
there. The LSMs already know how to initialize xattrs that they care
about. If there's a problem it will be with LSMs that count on the
lack of proper labeling when using these file systems.
No new hooks required. Rather than putting in a special case for
a file system that does not support xattrs, how about we solve the
problem and fix the real issue, which is the file systems that don't
support them?
The Smack project is running somewhat lean right now, but I could
shift some resources around (multi-label window managers are a pain
anyhow) if that is what it takes.
>>
>>> Proc also has some special case handling in the SELinux module but I
>>> haven't had a chance to look at it and try to understand why. I don't
>>> think that this would be a general purpose solution for all pseudo file
>>> systems like you mentioned above but it may work for some of them. I'll
>>> look into them a bit more and then respond about them.
>>>
>> Sounds good. If we are going to expand the LSM it would be good to design
>> something decent instead of adding a nasty add-hoc case.
>>
>
> A quick look over proc and debugfs leads me to believe that a generic
> mechanism for all of them short of adding generic xattr support to all
> pseudo file systems would be tricky at best and even more add-hoc than
> what we already do. There isn't any uniformity in the data structures
> that are used in these file systems so even if we came up with a lazy
> update mechanism for these attributes it looks like the implementation
> would vary greatly depending on the file system. Even then it doesn't
> change the add-hoc nature of the functionality as we are only trying to
> handle security attributes.
>
> The real solution which is a lot of work and I don't exactly know how I
> would go about putting it together is to try to provide a generic xattr
> mechanism for pseudo file systems. However I don't have any use cases
> for the majority of the xattr name spaces. The only thing we have at the
> moment that needs attention and only on sysfs is the security.* name
> space. So trying to implement full-blown xattrs on sysfs seems like a
> bunch of effort with no clear user for it.
>
>
>> And on a silly note. Rumor has it that selinux has provable security.
>> If so what impact does this change make to the proofs?
>>
>>
>
> I'm not a formal methods person so you would have to consult with the
> people who did that work to find out.
>
>
>
>> Eric
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
>
>
On Mon, 2009-07-13 at 17:29 -0700, Eric W. Biederman wrote:
> "David P. Quigley" <[email protected]> writes:
>
> > [ Inline Comments...]
> >
> > On Mon, 2009-07-13 at 09:50 -0700, Eric W. Biederman wrote:
> >> Taking the conversation back on the list.
> >>
> >> "David P. Quigley" <[email protected]> writes:
> >>
> >> > On Sun, 2009-07-12 at 07:51 -0700, Eric W. Biederman wrote:
> >> >> "David P. Quigley" <[email protected]> writes:
> >> >>
[Snip....]
> >
> > Looking at the sysfs code I can see where the inode gets its default
> > values for everything but uid and gid. Are those set somewhere higher up
> > in the vfs on the init_inode path? The approach does seem reasonable but
> > do we want to have to allocate an entire iattr structure inside the
> > sysfs_inode_attr structure you propose just to store the secid?
>
> For a non-default secid. I think so. I assume we want to track
> when the label was set and that requires timestamps.
>
> If wind up allocating a lot of these things perhaps it will make
> sense to do something different. But I expect the common case will be
> few nodes in sysfs having non-default attributes.
This sounds reasonable to me so I'll rework the patch to do this.
>
> >> >> > diff --git a/security/selinux/hooks.c b/security/selinux/hooks.c
> >> >> > index 2081055..395c36d 100644
> >> >> > --- a/security/selinux/hooks.c
> >> >> > +++ b/security/selinux/hooks.c
> >> >> > @@ -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;
> >> >>
> >> >> What is this about? My impression is that if we have generic xattr
> >> >> handling sysfs is not special so why do we have a special case?
> >> >>
> >> >> Is this interface appropriate for dealing with xattrs to all
> >> >> linux virtual filesystmes similar to sysfs that do not currently
> >> >> implement xattrs. aka debugfs, proc, etc?
> >> >
> >> > Even though sysfs has a setxattr handler the labeling behavior with
> >> > respect to SELinux needs special handling. The idea here is that by
> >> > default sysfs will be labeled across the board with the same label. The
> >> > reason we can't use a normal style xattr handler here is because there
> >> > is no original backing store to pull the label from. Only after a label
> >> > has been changed is there a semi-persistent value that can be used for
> >> > reinstantiating the inode in the case that it is pruned from the cache.
> >> > Otherwise it falls back to the base genfs labeling of sysfs entries as
> >> > sysfs_t.
> >>
> >> Sounds like we want a mount option or the like here. Something explicit
> >> in sysfs not something explicit in the security module.
> >>
> >> I am also a bit dubious about
> >
> > I don't think a mount option is the best thing here. Labeling behavior
> > is something that is LSM dependent and even file system dependent within
> > certain LSMs. Since I don't speak for Casey that the way SELinux handles
> > sysfs is the way he want's Smack handling sysfs, and we can't tell what
> > future label based LSMs will do I think leaving it to the module to
> > decide is best.
>
> Sure the source needs to be the lsm and not user space. The concept
> however of setting a default at the beginning of time and having no
> special cases beyond that seems reasonable.
>
> After that beginning of time default you would not need special cases.
So part of the issue here is that the labeling semantics for all file
systems aren't the same. For most in memory file systems we don't have
the need to be able to set the label after creation. For example proc
has all of its entries generated by the kernel and the kernel knows best
for how to label them. There isn't a need for a userspace interface to
change the labels on proc entries. Similarly something like devpts has
its labels set based on the creating process and also the inode can't be
evicted from under it. sysfs does actually seem to be a special case
since we have a mixture of two behaviors. We have the coarse genfs
labeling by default and then we have a situation where virtd needs to
set the label of the resource depending on the context of the qemu
process that will be accessing it.
>
> >> > Proc also has some special case handling in the SELinux module but I
> >> > haven't had a chance to look at it and try to understand why. I don't
> >> > think that this would be a general purpose solution for all pseudo file
> >> > systems like you mentioned above but it may work for some of them. I'll
> >> > look into them a bit more and then respond about them.
> >>
> >> Sounds good. If we are going to expand the LSM it would be good to design
> >> something decent instead of adding a nasty add-hoc case.
> >
> > A quick look over proc and debugfs leads me to believe that a generic
> > mechanism for all of them short of adding generic xattr support to all
> > pseudo file systems would be tricky at best and even more add-hoc than
> > what we already do. There isn't any uniformity in the data structures
> > that are used in these file systems so even if we came up with a lazy
> > update mechanism for these attributes it looks like the implementation
> > would vary greatly depending on the file system. Even then it doesn't
> > change the add-hoc nature of the functionality as we are only trying to
> > handle security attributes.
>
> So why should the lsm hooks care. That is what I am really after. A
> common set of lsm hooks, and the lsm hooks shouldn't need to see all of
> the data structures for those filesystems.
Well the hooks in this situation don't care. As long as the file system
has a backing data structure to store the secid if/when the inode gets
evicted then it can use the hooks just like sysfs does.
>
> > The real solution which is a lot of work and I don't exactly know how I
> > would go about putting it together is to try to provide a generic xattr
> > mechanism for pseudo file systems. However I don't have any use cases
> > for the majority of the xattr name spaces. The only thing we have at the
> > moment that needs attention and only on sysfs is the security.* name
> > space. So trying to implement full-blown xattrs on sysfs seems like a
> > bunch of effort with no clear user for it.
>
> Sort of. What we are taking about with this patch is generic xattr
> support facing user space and the security modules. With an optimized
> storage backend, that will only store well know attributes.
>
> I think it makes sense to talk about a way to keep from growing lsm
> special cases for each new virtual filesystem. Which means sysfs
> and debugfs at least should share the same lsm hooks. Ideally
> sysctl and proc would as well but they have special cases that
> likely would break userspace if we changed things at the moment.
The main issue here is that the reason we have all these tiny file
systems instead of some one creating a generic one a long time ago is
that they all have different semantics. This means there is potential
for the labeling behavior of each one to have different semantics which
means in certain cases there will be special cases. sysfs is one of
these special cases. Looking at it again I think the mechanism is
generalizable to any file system with some sort of backing store but the
fine details will have to be investigated on a per file system basis.
>
> Eric
So, I've been looking through the sysfs code and I can't find a
reference to netlink in there. I am assuming that it is other parts of
the kernel which make use of netlink which are calling the sysfs_*
functions. Any suggestions for where to look on how this is being used
and what the important users are?
Dave
On Tue, Jul 14, 2009 at 12:37:39PM -0400, David P. Quigley wrote:
> So, I've been looking through the sysfs code and I can't find a
> reference to netlink in there. I am assuming that it is other parts of
> the kernel which make use of netlink which are calling the sysfs_*
> functions. Any suggestions for where to look on how this is being used
> and what the important users are?
The netlink messages are coming from the kobject uevent code, look in
lib/kobject_uevent.c for the code that creates and sends them out. This
happens for every sysfs directory that is created that corresponds with
a kobject.
thanks,
greg k-h
On Tue, 2009-07-14 at 10:50 -0700, Greg KH wrote:
> On Tue, Jul 14, 2009 at 12:37:39PM -0400, David P. Quigley wrote:
> > So, I've been looking through the sysfs code and I can't find a
> > reference to netlink in there. I am assuming that it is other parts of
> > the kernel which make use of netlink which are calling the sysfs_*
> > functions. Any suggestions for where to look on how this is being used
> > and what the important users are?
>
> The netlink messages are coming from the kobject uevent code, look in
> lib/kobject_uevent.c for the code that creates and sends them out. This
> happens for every sysfs directory that is created that corresponds with
> a kobject.
>
> thanks,
>
> greg k-h
It is unclear to me what if anything we need to do to the kobject_uevent
code for these changes. Do you have a particular use case in mind? Is
there some sort of notification that should be sent up to user space
when the label is changed on a file?
Dave
On Tue, Jul 14, 2009 at 04:16:06PM -0400, David P. Quigley wrote:
> On Tue, 2009-07-14 at 10:50 -0700, Greg KH wrote:
> > On Tue, Jul 14, 2009 at 12:37:39PM -0400, David P. Quigley wrote:
> > > So, I've been looking through the sysfs code and I can't find a
> > > reference to netlink in there. I am assuming that it is other parts of
> > > the kernel which make use of netlink which are calling the sysfs_*
> > > functions. Any suggestions for where to look on how this is being used
> > > and what the important users are?
> >
> > The netlink messages are coming from the kobject uevent code, look in
> > lib/kobject_uevent.c for the code that creates and sends them out. This
> > happens for every sysfs directory that is created that corresponds with
> > a kobject.
> >
> > thanks,
> >
> > greg k-h
>
> It is unclear to me what if anything we need to do to the kobject_uevent
> code for these changes. Do you have a particular use case in mind? Is
> there some sort of notification that should be sent up to user space
> when the label is changed on a file?
No, the point is that userspace is notified when a kobject is created
and added to sysfs. You can use that notification to then put the
"correct" label on the sysfs directory and files, if they differ from
your "default" value you wanted them to have.
Hope this helps,
greg k-h
On Tue, 2009-07-14 at 13:35 -0700, Greg KH wrote:
> On Tue, Jul 14, 2009 at 04:16:06PM -0400, David P. Quigley wrote:
> > On Tue, 2009-07-14 at 10:50 -0700, Greg KH wrote:
> > > On Tue, Jul 14, 2009 at 12:37:39PM -0400, David P. Quigley wrote:
> > > > So, I've been looking through the sysfs code and I can't find a
> > > > reference to netlink in there. I am assuming that it is other parts of
> > > > the kernel which make use of netlink which are calling the sysfs_*
> > > > functions. Any suggestions for where to look on how this is being used
> > > > and what the important users are?
> > >
> > > The netlink messages are coming from the kobject uevent code, look in
> > > lib/kobject_uevent.c for the code that creates and sends them out. This
> > > happens for every sysfs directory that is created that corresponds with
> > > a kobject.
> > >
> > > thanks,
> > >
> > > greg k-h
> >
> > It is unclear to me what if anything we need to do to the kobject_uevent
> > code for these changes. Do you have a particular use case in mind? Is
> > there some sort of notification that should be sent up to user space
> > when the label is changed on a file?
>
> No, the point is that userspace is notified when a kobject is created
> and added to sysfs. You can use that notification to then put the
> "correct" label on the sysfs directory and files, if they differ from
> your "default" value you wanted them to have.
>
> Hope this helps,
>
> greg k-h
Ahh that makes sense. Thank you for the input. It seems like we can have
libvirtd listen for these messages and when the new devices are created
as a result of its actions it can then label them appropriately. I'll
pass it along to the svirt guys.
Dave