| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20090904160326.GC15342@us.ibm.com>
Date: Fri, 4 Sep 2009 11:03:26 -0500
From: "Serge E. Hallyn" <serue@...ibm.com>
To: "David P. Quigley" <dpquigl@...ho.nsa.gov>
Cc: sds@...ho.nsa.gov, jmorris@...ei.org, casey@...aufler-ca.com,
gregkh@...e.de, ebiederm@...ssion.com,
linux-kernel@...r.kernel.org, linux-security-module@...r.kernel.org
Subject: Re: [PATCH 3/3] sysfs: Add labeling support for sysfs
Quoting David P. Quigley (dpquigl@...ho.nsa.gov):
> This patch adds a setxattr handler to the file, directory, and symlink
> inode_operations structures for sysfs. The patch uses hooks introduced in the
> previous patch to handle the getting and setting of security information for
> the sysfs inodes. As was suggested by Eric Biederman the struct iattr in the
> sysfs_dirent structure has been replaced by a structure which contains the
> iattr, secdata and secdata length to allow the changes to persist in the event
> that the inode representing the sysfs_dirent is evicted. Because sysfs only
> stores this information when a change is made all the optional data is moved
> into one dynamically allocated field.
>
> This patch addresses an issue where SELinux was denying virtd access to the PCI
> configuration entries in sysfs. The lack of setxattr handlers for sysfs
> required that a single label be assigned to all entries in sysfs. Granting virtd
> access to every entry in sysfs is not an acceptable solution so fine grained
> labeling of sysfs is required such that individual entries can be labeled
> appropriately.
>
> Signed-off-by: David P. Quigley <dpquigl@...ho.nsa.gov>
Hmm, all looks good to me...
Acked-by: Serge Hallyn <serue@...ibm.com>
> ---
> fs/sysfs/dir.c | 1 +
> fs/sysfs/inode.c | 135 +++++++++++++++++++++++++++++++++------------
> fs/sysfs/symlink.c | 2 +
> fs/sysfs/sysfs.h | 12 ++++-
> security/selinux/hooks.c | 4 ++
> 5 files changed, 117 insertions(+), 37 deletions(-)
>
> diff --git a/fs/sysfs/dir.c b/fs/sysfs/dir.c
> index 14f2d71..0050fc4 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..9889bf2 100644
> --- a/fs/sysfs/inode.c
> +++ b/fs/sysfs/inode.c
> @@ -18,6 +18,8 @@
> #include <linux/capability.h>
> #include <linux/errno.h>
> #include <linux/sched.h>
> +#include <linux/xattr.h>
> +#include <linux/security.h>
> #include "sysfs.h"
>
> extern struct super_block * sysfs_sb;
> @@ -35,6 +37,7 @@ static struct backing_dev_info sysfs_backing_dev_info = {
>
> static const struct inode_operations sysfs_inode_operations ={
> .setattr = sysfs_setattr,
> + .setxattr = sysfs_setxattr,
> };
>
> int __init sysfs_inode_init(void)
> @@ -42,18 +45,37 @@ int __init sysfs_inode_init(void)
> return bdi_init(&sysfs_backing_dev_info);
> }
>
> +struct sysfs_inode_attrs * sysfs_init_inode_attrs(struct sysfs_dirent * sd)
> +{
> + struct sysfs_inode_attrs * attrs;
> + struct iattr * iattrs;
> +
> + attrs = kzalloc(sizeof(struct sysfs_inode_attrs), GFP_KERNEL);
> + if(!attrs)
> + return NULL;
> + iattrs = &attrs->ia_iattr;
> +
> + /* assign default attributes */
> + iattrs->ia_mode = sd->s_mode;
> + iattrs->ia_uid = 0;
> + iattrs->ia_gid = 0;
> + iattrs->ia_atime = iattrs->ia_mtime = iattrs->ia_ctime = CURRENT_TIME;
> +
> + return attrs;
> +}
> int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> {
> struct inode * inode = dentry->d_inode;
> struct sysfs_dirent * sd = dentry->d_fsdata;
> - struct iattr * sd_iattr;
> + struct sysfs_inode_attrs * sd_attrs;
> + struct iattr * iattrs;
> unsigned int ia_valid = iattr->ia_valid;
> int error;
>
> if (!sd)
> return -EINVAL;
>
> - sd_iattr = sd->s_iattr;
> + sd_attrs = sd->s_iattr;
>
> error = inode_change_ok(inode, iattr);
> if (error)
> @@ -65,42 +87,78 @@ int sysfs_setattr(struct dentry * dentry, struct iattr * iattr)
> if (error)
> return error;
>
> - if (!sd_iattr) {
> + if (!sd_attrs) {
> /* setting attributes for the first time, allocate now */
> - sd_iattr = kzalloc(sizeof(struct iattr), GFP_KERNEL);
> - if (!sd_iattr)
> + sd_attrs = sysfs_init_inode_attrs(sd);
> + if (!sd_attrs)
> return -ENOMEM;
> - /* assign default attributes */
> - sd_iattr->ia_mode = sd->s_mode;
> - sd_iattr->ia_uid = 0;
> - sd_iattr->ia_gid = 0;
> - sd_iattr->ia_atime = sd_iattr->ia_mtime = sd_iattr->ia_ctime = CURRENT_TIME;
> - sd->s_iattr = sd_iattr;
> + sd->s_iattr = sd_attrs;
> + } else {
> + /* attributes were changed atleast once in past */
> + iattrs = &sd_attrs->ia_iattr;
> +
> + if (ia_valid & ATTR_UID)
> + iattrs->ia_uid = iattr->ia_uid;
> + if (ia_valid & ATTR_GID)
> + iattrs->ia_gid = iattr->ia_gid;
> + if (ia_valid & ATTR_ATIME)
> + iattrs->ia_atime = timespec_trunc(iattr->ia_atime,
> + inode->i_sb->s_time_gran);
> + if (ia_valid & ATTR_MTIME)
> + iattrs->ia_mtime = timespec_trunc(iattr->ia_mtime,
> + inode->i_sb->s_time_gran);
> + if (ia_valid & ATTR_CTIME)
> + iattrs->ia_ctime = timespec_trunc(iattr->ia_ctime,
> + inode->i_sb->s_time_gran);
> + if (ia_valid & ATTR_MODE) {
> + umode_t mode = iattr->ia_mode;
> +
> + if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> + mode &= ~S_ISGID;
> + iattrs->ia_mode = sd->s_mode = mode;
> + }
> }
> + return error;
> +}
>
> - /* attributes were changed atleast once in past */
> -
> - if (ia_valid & ATTR_UID)
> - sd_iattr->ia_uid = iattr->ia_uid;
> - if (ia_valid & ATTR_GID)
> - sd_iattr->ia_gid = iattr->ia_gid;
> - if (ia_valid & ATTR_ATIME)
> - sd_iattr->ia_atime = timespec_trunc(iattr->ia_atime,
> - inode->i_sb->s_time_gran);
> - if (ia_valid & ATTR_MTIME)
> - sd_iattr->ia_mtime = timespec_trunc(iattr->ia_mtime,
> - inode->i_sb->s_time_gran);
> - if (ia_valid & ATTR_CTIME)
> - sd_iattr->ia_ctime = timespec_trunc(iattr->ia_ctime,
> - inode->i_sb->s_time_gran);
> - if (ia_valid & ATTR_MODE) {
> - umode_t mode = iattr->ia_mode;
> -
> - if (!in_group_p(inode->i_gid) && !capable(CAP_FSETID))
> - mode &= ~S_ISGID;
> - sd_iattr->ia_mode = sd->s_mode = mode;
> - }
> +int sysfs_setxattr(struct dentry *dentry, const char *name, const void *value,
> + size_t size, int flags)
> +{
> + struct sysfs_dirent *sd = dentry->d_fsdata;
> + struct sysfs_inode_attrs *iattrs;
> + char *suffix;
> + char *secdata;
> + int error;
> + u32 secdata_len = 0;
> +
> + if (!sd)
> + return -EINVAL;
> + if (!sd->s_iattr)
> + sd->s_iattr = sysfs_init_inode_attrs(sd);
> + if (!sd->s_iattr)
> + return -ENOMEM;
> +
> + iattrs = sd->s_iattr;
> +
> + if (!strncmp(name, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN)) {
> + const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> + error = security_inode_setsecurity(dentry->d_inode, suffix,
> + value, size, flags);
> + if (error)
> + goto out;
> + error = security_inode_getsecctx(dentry->d_inode,
> + &secdata, &secdata_len);
> + if (error)
> + goto out;
> + if(iattrs->ia_secdata)
> + security_release_secctx(iattrs->ia_secdata,
> + iattrs->ia_secdata_len);
> + iattrs->ia_secdata = secdata;
> + iattrs->ia_secdata_len = secdata_len;
>
> + } else
> + return -EINVAL;
> +out:
> return error;
> }
>
> @@ -146,6 +204,7 @@ static int sysfs_count_nlink(struct sysfs_dirent *sd)
> static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
> {
> struct bin_attribute *bin_attr;
> + struct sysfs_inode_attrs * iattrs;
>
> inode->i_private = sysfs_get(sd);
> inode->i_mapping->a_ops = &sysfs_aops;
> @@ -154,16 +213,20 @@ static void sysfs_init_inode(struct sysfs_dirent *sd, struct inode *inode)
> inode->i_ino = sd->s_ino;
> lockdep_set_class(&inode->i_mutex, &sysfs_inode_imutex_key);
>
> - if (sd->s_iattr) {
> + iattrs = sd->s_iattr;
> + if (iattrs) {
> /* sysfs_dirent has non-default attributes
> * get them for the new inode from persistent copy
> * in sysfs_dirent
> */
> - set_inode_attr(inode, sd->s_iattr);
> + set_inode_attr(inode, &iattrs->ia_iattr);
> + if (iattrs->ia_secdata)
> + security_inode_notifysecctx(inode,
> + iattrs->ia_secdata,
> + iattrs->ia_secdata_len);
> } else
> set_default_inode_attr(inode, sd->s_mode);
>
> -
> /* initialize inode according to type */
> switch (sysfs_type(sd)) {
> case SYSFS_DIR:
> 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..af4c4e7 100644
> --- a/fs/sysfs/sysfs.h
> +++ b/fs/sysfs/sysfs.h
> @@ -8,6 +8,8 @@
> * This file is released under the GPLv2.
> */
>
> +#include <linux/fs.h>
> +
> struct sysfs_open_dirent;
>
> /* type-specific structures for sysfs_dirent->s_* union members */
> @@ -31,6 +33,12 @@ struct sysfs_elem_bin_attr {
> struct hlist_head buffers;
> };
>
> +struct sysfs_inode_attrs {
> + struct iattr ia_iattr;
> + void *ia_secdata;
> + u32 ia_secdata_len;
> +};
> +
> /*
> * sysfs_dirent - the building block of sysfs hierarchy. Each and
> * every sysfs node is represented by single sysfs_dirent.
> @@ -56,7 +64,7 @@ struct sysfs_dirent {
> unsigned int s_flags;
> ino_t s_ino;
> umode_t s_mode;
> - struct iattr *s_iattr;
> + struct sysfs_inode_attrs *s_iattr;
> };
>
> #define SD_DEACTIVATED_BIAS INT_MIN
> @@ -148,6 +156,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/security/selinux/hooks.c b/security/selinux/hooks.c
> index f1d5677..38ed894 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);
>
> --
> 1.5.6.6
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists