Re: [PATCH 03/11] VFS: Add security label support to *notify

From: Josef 'Jeff' Sipek
Date: Thu Feb 28 2008 - 15:14:43 EST

Ignoring the security parts of it, here are a few comments about the VFS and
coding style related bits...

Josef 'Jeff' Sipek.

On Wed, Feb 27, 2008 at 03:39:38PM -0500, David P. Quigley wrote:
> This patch adds two new fields to the iattr structure. The first field holds a
> security label while the second contains the length of this label. In addition
> the patch adds a new helper function inode_setsecurity which calls the LSM to
> set the security label on the inode. Finally the patch modifies the necessary
> functions such that fsnotify_change can handle notification requests for
> dnotify and inotify.
>
> Signed-off-by: David P. Quigley <dpquigl@xxxxxxxxxxxxx>
> ---
> fs/attr.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> fs/xattr.c | 33 +++++++++++++++++++++++++++------
> include/linux/fcntl.h | 1 +
> include/linux/fs.h | 11 +++++++++++
> include/linux/fsnotify.h | 6 ++++++
> include/linux/inotify.h | 3 ++-
> include/linux/xattr.h | 1 +
> 7 files changed, 91 insertions(+), 7 deletions(-)
>
> diff --git a/fs/attr.c b/fs/attr.c
> index 966b73e..1df6603 100644
> --- a/fs/attr.c
> +++ b/fs/attr.c
> @@ -5,6 +5,7 @@
> * changes by Thomas Schoebel-Theuer
> */
>
> +#include <linux/fs.h>
> #include <linux/module.h>
> #include <linux/time.h>
> #include <linux/mm.h>
> @@ -14,9 +15,35 @@
> #include <linux/fcntl.h>
> #include <linux/quotaops.h>
> #include <linux/security.h>
> +#include <linux/xattr.h>
>
> /* Taken over from the old code... */
>
> +#ifdef CONFIG_SECURITY
> +/*
> + * Update the in core label.
> + */
> +int inode_setsecurity(struct inode *inode, struct iattr *attr)
> +{
> + const char *suffix = security_maclabel_getname()
> + + XATTR_SECURITY_PREFIX_LEN;
> + int error;
> +
> + if (!attr->ia_valid & ATTR_SECURITY_LABEL)
> + return -EINVAL;

You most likely want:

if (!(attr->ia_valid & ATTR_SECURITY_LABEL))

IOW, watch out for operator precedence.

> +
> + error = security_inode_setsecurity(inode, suffix, attr->ia_label,
> + attr->ia_label_len, 0);
> + if (error)
> + printk(KERN_ERR "%s() %s %d security_inode_setsecurity() %d\n"
> + , __func__, (char *)attr->ia_label, attr->ia_label_len
> + , error);

The commas should really be on the previous line.

> +
> + return error;
> +}
> +EXPORT_SYMBOL(inode_setsecurity);
> +#endif
> +
> /* POSIX UID/GID verification for setting inode attributes. */
> int inode_change_ok(struct inode *inode, struct iattr *attr)
> {
> @@ -94,6 +121,10 @@ int inode_setattr(struct inode * inode, struct iattr * attr)
> mode &= ~S_ISGID;
> inode->i_mode = mode;
> }
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL)
> + inode_setsecurity(inode, attr);
> +#endif
> mark_inode_dirty(inode);
>
> return 0;
> @@ -157,6 +188,18 @@ int notify_change(struct dentry * dentry, struct iattr * attr)
> if (ia_valid & ATTR_SIZE)
> down_write(&dentry->d_inode->i_alloc_sem);
>
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL) {
> + char *key = (char *)security_maclabel_getname();
> + vfs_setxattr_locked(dentry, key,
> + attr->ia_label, attr->ia_label_len, 0);
> + /* Avoid calling inode_setsecurity()
> + * via inode_setattr() below
> + */
> + attr->ia_valid &= ~ATTR_SECURITY_LABEL;
> + }
> +#endif
> +
> if (inode->i_op && inode->i_op->setattr) {
> error = security_inode_setattr(dentry, attr);
> if (!error)
> diff --git a/fs/xattr.c b/fs/xattr.c
> index 3acab16..b5a91e1 100644
> --- a/fs/xattr.c
> +++ b/fs/xattr.c
> @@ -67,9 +67,9 @@ xattr_permission(struct inode *inode, const char *name, int mask)
> return permission(inode, mask, NULL);
> }
>
> -int
> -vfs_setxattr(struct dentry *dentry, char *name, void *value,
> - size_t size, int flags)
> +static int
> +_vfs_setxattr(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags, int lock)

The convention is to use __ or do_ as the prefix.

> {
> struct inode *inode = dentry->d_inode;
> int error;
> @@ -78,7 +78,8 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> if (error)
> return error;
>
> - mutex_lock(&inode->i_mutex);
> + if (lock)
> + mutex_lock(&inode->i_mutex);
> error = security_inode_setxattr(dentry, name, value, size, flags);
> if (error)
> goto out;
> @@ -95,15 +96,35 @@ vfs_setxattr(struct dentry *dentry, char *name, void *value,
> const char *suffix = name + XATTR_SECURITY_PREFIX_LEN;
> error = security_inode_setsecurity(inode, suffix, value,
> size, flags);
> - if (!error)
> + if (!error) {
> +#ifdef CONFIG_SECURITY
> + fsnotify_change(dentry, ATTR_SECURITY_LABEL);
> +#endif
> fsnotify_xattr(dentry);
> + }
> }
> out:
> - mutex_unlock(&inode->i_mutex);
> + if (lock)
> + mutex_unlock(&inode->i_mutex);
> return error;
> }
> +
> +int
> +vfs_setxattr(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags)
> +{
> + return _vfs_setxattr(dentry, name, value, size, flags, 1);
> +}
> EXPORT_SYMBOL_GPL(vfs_setxattr);
>
> +int
> +vfs_setxattr_locked(struct dentry *dentry, char *name, void *value,
> + size_t size, int flags)
> +{
> + return _vfs_setxattr(dentry, name, value, size, flags, 0);
> +}
> +EXPORT_SYMBOL_GPL(vfs_setxattr_locked);

Alright...so, few things...

1) why do you need the locked/unlocked versions?

2) instead of passing a flag to a common function, why not have:

vfs_setxattr_locked(....)
{
// original code minus the lock/unlock calls
}

vfs_setxattr(....)
{
mutex_lock(...);
vfs_setxattr_locked(...);
mutex_unlock(...);
}


> +
> ssize_t
> xattr_getsecurity(struct inode *inode, const char *name, void *value,
> size_t size)
> diff --git a/include/linux/fcntl.h b/include/linux/fcntl.h
> index 8603740..fae1e59 100644
> --- a/include/linux/fcntl.h
> +++ b/include/linux/fcntl.h
> @@ -30,6 +30,7 @@
> #define DN_DELETE 0x00000008 /* File removed */
> #define DN_RENAME 0x00000010 /* File renamed */
> #define DN_ATTRIB 0x00000020 /* File changed attibutes */
> +#define DN_LABEL 0x00000040 /* File (re)labeled */
> #define DN_MULTISHOT 0x80000000 /* Don't remove notifier */
>
> #define AT_FDCWD -100 /* Special value used to indicate
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index b84b848..757add6 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -335,6 +335,10 @@ typedef void (dio_iodone_t)(struct kiocb *iocb, loff_t offset,
> #define ATTR_KILL_PRIV 16384
> #define ATTR_OPEN 32768 /* Truncating from open(O_TRUNC) */
>
> +#ifdef CONFIG_SECURITY
> +#define ATTR_SECURITY_LABEL 65536
> +#endif
> +
> /*
> * This is the Inode Attributes structure, used for notify_change(). It
> * uses the above definitions as flags, to know which values have changed.
> @@ -360,6 +364,10 @@ struct iattr {
> * check for (ia_valid & ATTR_FILE), and not for (ia_file != NULL).
> */
> struct file *ia_file;
> +#ifdef CONFIG_SECURITY
> + void *ia_label;
> + u32 ia_label_len;
> +#endif
> };
>
> /*
> @@ -1971,6 +1979,9 @@ extern int buffer_migrate_page(struct address_space *,
> #define buffer_migrate_page NULL
> #endif
>
> +#ifdef CONFIG_SECURITY
> +extern int inode_setsecurity(struct inode *inode, struct iattr *attr);
> +#endif
> extern int inode_change_ok(struct inode *, struct iattr *);
> extern int __must_check inode_setattr(struct inode *, struct iattr *);
>
> diff --git a/include/linux/fsnotify.h b/include/linux/fsnotify.h
> index d4b7c4a..b54a3cb 100644
> --- a/include/linux/fsnotify.h
> +++ b/include/linux/fsnotify.h
> @@ -253,6 +253,12 @@ static inline void fsnotify_change(struct dentry *dentry, unsigned int ia_valid)
> in_mask |= IN_ATTRIB;
> dn_mask |= DN_ATTRIB;
> }
> +#ifdef CONFIG_SECURITY
> + if (ia_valid & ATTR_SECURITY_LABEL) {
> + in_mask |= IN_LABEL;
> + dn_mask |= DN_LABEL;
> + }
> +#endif
>
> if (dn_mask)
> dnotify_parent(dentry, dn_mask);
> diff --git a/include/linux/inotify.h b/include/linux/inotify.h
> index 742b917..10f3ace 100644
> --- a/include/linux/inotify.h
> +++ b/include/linux/inotify.h
> @@ -36,6 +36,7 @@ struct inotify_event {
> #define IN_DELETE 0x00000200 /* Subfile was deleted */
> #define IN_DELETE_SELF 0x00000400 /* Self was deleted */
> #define IN_MOVE_SELF 0x00000800 /* Self was moved */
> +#define IN_LABEL 0x00001000 /* Self was (re)labeled */
>
> /* the following are legal events. they are sent as needed to any watch */
> #define IN_UNMOUNT 0x00002000 /* Backing fs was unmounted */
> @@ -61,7 +62,7 @@ struct inotify_event {
> #define IN_ALL_EVENTS (IN_ACCESS | IN_MODIFY | IN_ATTRIB | IN_CLOSE_WRITE | \
> IN_CLOSE_NOWRITE | IN_OPEN | IN_MOVED_FROM | \
> IN_MOVED_TO | IN_DELETE | IN_CREATE | IN_DELETE_SELF | \
> - IN_MOVE_SELF)
> + IN_MOVE_SELF | IN_LABEL)
>
> #ifdef __KERNEL__
>
> diff --git a/include/linux/xattr.h b/include/linux/xattr.h
> index df6b95d..1169963 100644
> --- a/include/linux/xattr.h
> +++ b/include/linux/xattr.h
> @@ -50,6 +50,7 @@ ssize_t xattr_getsecurity(struct inode *, const char *, void *, size_t);
> ssize_t vfs_getxattr(struct dentry *, char *, void *, size_t);
> ssize_t vfs_listxattr(struct dentry *d, char *list, size_t size);
> int vfs_setxattr(struct dentry *, char *, void *, size_t, int);
> +int vfs_setxattr_locked(struct dentry *, char *, void *, size_t, int);
> int vfs_removexattr(struct dentry *, char *);
>
> ssize_t generic_getxattr(struct dentry *dentry, const char *name, void *buffer, size_t size);
> --
> 1.5.3.8
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/

--
Don't drink and derive. Alcohol and algebra don't mix.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/