Re: [PATCH 2/2] f2fs: support xattr security labels

[Casey Schaufler <casey@schaufler-ca.com>]

On 6/6/2013 10:55 PM, Jaegeuk Kim wrote:
> This patch adds the support of security labels for f2fs, which will be used
> by SElinux.

Please be inclusive. Security xattrs are used by LSMs other than SELinux.

> Signed-off-by: Jaegeuk Kim <jaegeuk.kim@samsung.com>
> ---
>  fs/f2fs/Kconfig |  9 +++++++++
>  fs/f2fs/dir.c   |  5 +++++
>  fs/f2fs/xattr.c | 59 +++++++++++++++++++++++++++++++++++++++++++++++++++++++--
>  fs/f2fs/xattr.h | 12 +++++++++++-
>  4 files changed, 82 insertions(+), 3 deletions(-)
>
> diff --git a/fs/f2fs/Kconfig b/fs/f2fs/Kconfig
> index fd27e7e..2214cc9 100644
> --- a/fs/f2fs/Kconfig
> +++ b/fs/f2fs/Kconfig
> @@ -51,3 +51,12 @@ config F2FS_FS_POSIX_ACL
>  	  Linux website <http://acl.bestbits.at/>.
>  
>  	  If you don't know what Access Control Lists are, say N
> +
> +config F2FS_FS_SECURITY
> +	bool "F2FS Security Labels"
> +	depends on F2FS_FS_XATTR
> +	help
> +	  Security labels provide acls used by the security modules
> +	  like SELinux. This option should be used with the xattr mode.

This description missuses the term "acl". Security labels are not
Access Control Lists (ACLs). What is the "xattr mode"? If this option
depends on xattr support "should" is not correct.

> +
> +	  If you are not using a security module, say N.
> diff --git a/fs/f2fs/dir.c b/fs/f2fs/dir.c
> index 67e2d13..81a1d6f 100644
> --- a/fs/f2fs/dir.c
> +++ b/fs/f2fs/dir.c
> @@ -13,6 +13,7 @@
>  #include "f2fs.h"
>  #include "node.h"
>  #include "acl.h"
> +#include "xattr.h"
>  
>  static unsigned long dir_blocks(struct inode *inode)
>  {
> @@ -334,6 +335,10 @@ static struct page *init_inode_metadata(struct inode *inode,
>  		if (err)
>  			goto error;
>  
> +		err = f2fs_init_security(inode, dir, name);
> +		if (err)
> +			goto error;
> +
>  		wait_on_page_writeback(page);
>  	} else {
>  		page = get_node_page(F2FS_SB(dir->i_sb), inode->i_ino);
> diff --git a/fs/f2fs/xattr.c b/fs/f2fs/xattr.c
> index ae61f35..b5292fa 100644
> --- a/fs/f2fs/xattr.c
> +++ b/fs/f2fs/xattr.c
> @@ -20,6 +20,7 @@
>   */
>  #include <linux/rwsem.h>
>  #include <linux/f2fs_fs.h>
> +#include <linux/security.h>
>  #include "f2fs.h"
>  #include "xattr.h"
>  
> @@ -43,6 +44,10 @@ static size_t f2fs_xattr_generic_list(struct dentry *dentry, char *list,
>  		prefix = XATTR_TRUSTED_PREFIX;
>  		prefix_len = XATTR_TRUSTED_PREFIX_LEN;
>  		break;
> +	case F2FS_XATTR_INDEX_SECURITY:
> +		prefix = XATTR_SECURITY_PREFIX;
> +		prefix_len = XATTR_SECURITY_PREFIX_LEN;
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -70,13 +75,14 @@ static int f2fs_xattr_generic_get(struct dentry *dentry, const char *name,
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
>  		break;
> +	case F2FS_XATTR_INDEX_SECURITY:
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
>  	if (strcmp(name, "") == 0)
>  		return -EINVAL;
> -	return f2fs_getxattr(dentry->d_inode, type, name,
> -			buffer, size);
> +	return f2fs_getxattr(dentry->d_inode, type, name, buffer, size);
>  }
>  
>  static int f2fs_xattr_generic_set(struct dentry *dentry, const char *name,
> @@ -93,6 +99,8 @@ static int f2fs_xattr_generic_set(struct dentry *dentry, const char *name,
>  		if (!capable(CAP_SYS_ADMIN))
>  			return -EPERM;
>  		break;
> +	case F2FS_XATTR_INDEX_SECURITY:
> +		break;
>  	default:
>  		return -EINVAL;
>  	}
> @@ -145,6 +153,40 @@ static int f2fs_xattr_advise_set(struct dentry *dentry, const char *name,
>  	return 0;
>  }
>  
> +#ifdef CONFIG_F2FS_FS_SECURITY
> +static int f2fs_initxattrs(struct inode *inode, const struct xattr *xattr_array,
> +		void *fs_info)
> +{
> +	const struct xattr *xattr;
> +	char *name;
> +	int err = 0;
> +
> +	for (xattr = xattr_array; xattr->name != NULL; xattr++) {
> +		name = kmalloc(XATTR_SECURITY_PREFIX_LEN +
> +			       strlen(xattr->name) + 1, GFP_NOFS);
> +		if (!name) {
> +			err = -ENOMEM;
> +			break;
> +		}
> +		strcpy(name, XATTR_SECURITY_PREFIX);
> +		strcpy(name + XATTR_SECURITY_PREFIX_LEN, xattr->name);

		sprintf(name, XATTR_SECURITY_PREFIX "%s", xattr->name);

might look simpler.

> +		err = f2fs_setxattr(inode, F2FS_XATTR_INDEX_SECURITY, name,
> +					xattr->value, xattr->value_len);
> +		kfree(name);
> +		if (err < 0)
> +			break;
> +	}
> +	return err;
> +}
> +
> +int f2fs_init_security(struct inode *inode, struct inode *dir,
> +				const struct qstr *qstr)
> +{
> +	return security_inode_init_security(inode, dir, qstr,
> +				&f2fs_initxattrs, NULL);
> +}
> +#endif
> +
>  const struct xattr_handler f2fs_xattr_user_handler = {
>  	.prefix	= XATTR_USER_PREFIX,
>  	.flags	= F2FS_XATTR_INDEX_USER,
> @@ -169,6 +211,13 @@ const struct xattr_handler f2fs_xattr_advise_handler = {
>  	.set    = f2fs_xattr_advise_set,
>  };
>  
> +const struct xattr_handler f2fs_xattr_security_handler = {
> +	.prefix	= XATTR_SECURITY_PREFIX,
> +	.list	= f2fs_xattr_generic_list,
> +	.get	= f2fs_xattr_generic_get,
> +	.set	= f2fs_xattr_generic_set,
> +};
> +
>  static const struct xattr_handler *f2fs_xattr_handler_map[] = {
>  	[F2FS_XATTR_INDEX_USER] = &f2fs_xattr_user_handler,
>  #ifdef CONFIG_F2FS_FS_POSIX_ACL
> @@ -177,6 +226,9 @@ static const struct xattr_handler *f2fs_xattr_handler_map[] = {
>  #endif
>  	[F2FS_XATTR_INDEX_TRUSTED] = &f2fs_xattr_trusted_handler,
>  	[F2FS_XATTR_INDEX_ADVISE] = &f2fs_xattr_advise_handler,
> +#ifdef CONFIG_F2FS_FS_SECURITY
> +	[F2FS_XATTR_INDEX_SECURITY] = &f2fs_xattr_security_handler,
> +#endif
>  };
>  
>  const struct xattr_handler *f2fs_xattr_handlers[] = {
> @@ -187,6 +239,9 @@ const struct xattr_handler *f2fs_xattr_handlers[] = {
>  #endif
>  	&f2fs_xattr_trusted_handler,
>  	&f2fs_xattr_advise_handler,
> +#ifdef CONFIG_F2FS_FS_SECURITY
> +	&f2fs_xattr_security_handler,
> +#endif
>  	NULL,
>  };
>  
> diff --git a/fs/f2fs/xattr.h b/fs/f2fs/xattr.h
> index 49c9558..14e1329 100644
> --- a/fs/f2fs/xattr.h
> +++ b/fs/f2fs/xattr.h
> @@ -112,6 +112,7 @@ extern const struct xattr_handler f2fs_xattr_trusted_handler;
>  extern const struct xattr_handler f2fs_xattr_acl_access_handler;
>  extern const struct xattr_handler f2fs_xattr_acl_default_handler;
>  extern const struct xattr_handler f2fs_xattr_advise_handler;
> +extern const struct xattr_handler f2fs_xattr_security_handler;
>  
>  extern const struct xattr_handler *f2fs_xattr_handlers[];
>  
> @@ -121,7 +122,6 @@ extern int f2fs_getxattr(struct inode *inode, int name_index, const char *name,
>  		void *buffer, size_t buffer_size);
>  extern ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer,
>  		size_t buffer_size);
> -
>  #else
>  
>  #define f2fs_xattr_handlers	NULL
> @@ -142,4 +142,14 @@ static inline ssize_t f2fs_listxattr(struct dentry *dentry, char *buffer,
>  }
>  #endif
>  
> +#ifdef CONFIG_F2FS_FS_SECURITY
> +extern int f2fs_init_security(struct inode *inode, struct inode *dir,
> +				const struct qstr *qstr);
> +#else
> +static inline int f2fs_init_security(struct inode *inode, struct inode *dir,
> +				const struct qstr *qstr)
> +{
> +	return 0;
> +}
> +#endif
>  #endif /* __F2FS_XATTR_H__ */