Re: [PATCH] Add security.selinux XATTR support for the UBIFS. Alsofix couple of bugs in UBIFS extended attribute length calculation.

From: Subodh Nijsure
Date: Mon Apr 09 2012 - 02:40:03 EST

Next message: werner: "Re: v3.4-rc2 out-of-memory problems (was Re: 3.4-rc1sticks-and-crashs)"
Previous message: Jens Axboe: "Re: [PATCH v2 0/7] mtip32xx: Add support for device status and otherchanges"
In reply to: Casey Schaufler: "Re: [PATCH] Add security.selinux XATTR support for the UBIFS. Alsofix couple of bugs in UBIFS extended attribute length calculation."
Next in thread: Casey Schaufler: "Re: [PATCH] Add security.selinux XATTR support for the UBIFS. Alsofix couple of bugs in UBIFS extended attribute length calculation."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On Sun, Apr 8, 2012 at 7:37 PM, Casey Schaufler <casey@xxxxxxxxxxxxxxxx> wrote:
> On 4/8/2012 6:47 AM, Subodh Nijsure wrote:
>> TESTING: Tested on ÂMX28 based platforms using Micron MT29F2G08ABAEAH4 NAND
>> Â Â Â Â ÂWith these change we are able to label UBIFS filesystem with security.selinux
>> Â Â Â Â Âand run system with selinux enabled.
>> Â Â Â Â ÂAlso ran integck test on UBI filesystem.
>>
>> Signed-off-by: Subodh Nijsure <snijsure@xxxxxxxxxxxx>
>
> There is no reason to restrict xattr support to SELinux.
> SELinux specific behavior belongs within the SELinux LSM.
> This code needs to support the full xattr semantics.
>
> Nacked-by: Casey Schaufler <casey@xxxxxxxxxxxxxxxx>
>
>> ---
>> Âfs/ubifs/dir.c Â Â | Â Â4 ++
>> Âfs/ubifs/file.c Â Â| Â Â6 ++
>> Âfs/ubifs/journal.c | Â 12 +++-
>> Âfs/ubifs/super.c Â | Â Â3 +
>> Âfs/ubifs/ubifs.h Â | Â Â9 +++
>> Âfs/ubifs/xattr.c Â | Â147 ++++++++++++++++++++++++++++++++++++++++++++++++----
>> Â6 files changed, 167 insertions(+), 14 deletions(-)
>>
>> diff --git a/fs/ubifs/dir.c b/fs/ubifs/dir.c
>> index ec9f187..f4e06c4 100644
>> --- a/fs/ubifs/dir.c
>> +++ b/fs/ubifs/dir.c
>> @@ -293,6 +293,7 @@ static int ubifs_create(struct inode *dir, struct dentry *dentry, umode_t mode,
>> Â Â Â ubifs_release_budget(c, &req);
>> Â Â Â insert_inode_hash(inode);
>> Â Â Â d_instantiate(dentry, inode);
>> + Â Â ubifs_init_security(dir, inode, &dentry->d_name);
>> Â Â Â return 0;
>>
>> Âout_cancel:
>> @@ -754,6 +755,7 @@ static int ubifs_mkdir(struct inode *dir, struct dentry *dentry, umode_t mode)
>>
>> Â Â Â ubifs_release_budget(c, &req);
>> Â Â Â d_instantiate(dentry, inode);
>> + Â Â ubifs_init_security(dir, inode, &dentry->d_name);
>> Â Â Â return 0;
>>
>> Âout_cancel:
>> @@ -831,6 +833,7 @@ static int ubifs_mknod(struct inode *dir, struct dentry *dentry,
>> Â Â Â ubifs_release_budget(c, &req);
>> Â Â Â insert_inode_hash(inode);
>> Â Â Â d_instantiate(dentry, inode);
>> + Â Â ubifs_init_security(dir, inode, &dentry->d_name);
>> Â Â Â return 0;
>>
>> Âout_cancel:
>> @@ -907,6 +910,7 @@ static int ubifs_symlink(struct inode *dir, struct dentry *dentry,
>> Â Â Â ubifs_release_budget(c, &req);
>> Â Â Â insert_inode_hash(inode);
>> Â Â Â d_instantiate(dentry, inode);
>> + Â Â ubifs_init_security(dir, inode, &dentry->d_name);
>> Â Â Â return 0;
>>
>> Âout_cancel:
>> diff --git a/fs/ubifs/file.c b/fs/ubifs/file.c
>> index 5c8f6dc..b8e9d66 100644
>> --- a/fs/ubifs/file.c
>> +++ b/fs/ubifs/file.c
>> @@ -1575,6 +1575,12 @@ const struct inode_operations ubifs_symlink_inode_operations = {
>> Â Â Â .follow_link = ubifs_follow_link,
>> Â Â Â .setattr Â Â = ubifs_setattr,
>> Â Â Â .getattr Â Â = ubifs_getattr,
>> +#ifdef CONFIG_UBIFS_FS_XATTR
>> + Â Â .setxattr Â Â= ubifs_symlink_setxattr,
>> + Â Â .getxattr Â Â= ubifs_symlink_getxattr,
>> + Â Â .listxattr Â = ubifs_listxattr,
>> + Â Â .removexattr = ubifs_removexattr,
>> +#endif
>> Â};
>>
>> Âconst struct file_operations ubifs_file_operations = {
>> diff --git a/fs/ubifs/journal.c b/fs/ubifs/journal.c
>> index 2f438ab..725dc17 100644
>> --- a/fs/ubifs/journal.c
>> +++ b/fs/ubifs/journal.c
>> @@ -553,7 +553,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
>>
>> Â Â Â dbg_jnl("ino %lu, dent '%.*s', data len %d in dir ino %lu",
>> Â Â Â Â Â Â Â inode->i_ino, nm->len, nm->name, ui->data_len, dir->i_ino);
>> - Â Â ubifs_assert(dir_ui->data_len == 0);
>> + Â Â if (!xent)
>> + Â Â Â Â Â Â ubifs_assert(dir_ui->data_len == 0);
>> Â Â Â ubifs_assert(mutex_is_locked(&dir_ui->ui_mutex));
>>
>> Â Â Â dlen = UBIFS_DENT_NODE_SZ + nm->len + 1;
>> @@ -572,7 +573,11 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
>>
>> Â Â Â aligned_dlen = ALIGN(dlen, 8);
>> Â Â Â aligned_ilen = ALIGN(ilen, 8);
>> - Â Â len = aligned_dlen + aligned_ilen + UBIFS_INO_NODE_SZ;
>> + Â Â /* Make sure to account for dir_ui+data_len in length calculation
>> + Â Â Â* in case there is extended attribute.
>> + Â Â Â*/
>> + Â Â len = aligned_dlen + aligned_ilen +
>> + Â Â Â Â Â UBIFS_INO_NODE_SZ + dir_ui->data_len;
>> Â Â Â dent = kmalloc(len, GFP_NOFS);
>> Â Â Â if (!dent)
>> Â Â Â Â Â Â Â return -ENOMEM;
>> @@ -649,7 +654,8 @@ int ubifs_jnl_update(struct ubifs_info *c, const struct inode *dir,
>>
>> Â Â Â ino_key_init(c, &ino_key, dir->i_ino);
>> Â Â Â ino_offs += aligned_ilen;
>> - Â Â err = ubifs_tnc_add(c, &ino_key, lnum, ino_offs, UBIFS_INO_NODE_SZ);
>> + Â Â err = ubifs_tnc_add(c, &ino_key, lnum, ino_offs,
>> + Â Â Â Â Â Â Â Â Â Â Â Â UBIFS_INO_NODE_SZ + dir_ui->data_len);
>> Â Â Â if (err)
>> Â Â Â Â Â Â Â goto out_ro;
>>
>> diff --git a/fs/ubifs/super.c b/fs/ubifs/super.c
>> index 76e4e05..c28ac04 100644
>> --- a/fs/ubifs/super.c
>> +++ b/fs/ubifs/super.c
>> @@ -2061,6 +2061,9 @@ static int ubifs_fill_super(struct super_block *sb, void *data, int silent)
>> Â Â Â if (c->max_inode_sz > MAX_LFS_FILESIZE)
>> Â Â Â Â Â Â Â sb->s_maxbytes = c->max_inode_sz = MAX_LFS_FILESIZE;
>> Â Â Â sb->s_op = &ubifs_super_operations;
>> +#ifdef CONFIG_UBIFS_FS_XATTR
>> + Â Â sb->s_xattr = ubifs_xattr_handlers;
>> +#endif
>>
>> Â Â Â mutex_lock(&c->umount_mutex);
>> Â Â Â err = mount_ubifs(c);
>> diff --git a/fs/ubifs/ubifs.h b/fs/ubifs/ubifs.h
>> index 93d59ac..60b57f7 100644
>> --- a/fs/ubifs/ubifs.h
>> +++ b/fs/ubifs/ubifs.h
>> @@ -36,6 +36,7 @@
>> Â#include <linux/mtd/ubi.h>
>> Â#include <linux/pagemap.h>
>> Â#include <linux/backing-dev.h>
>> +#include <linux/security.h>
>> Â#include "ubifs-media.h"
>>
>> Â/* Version of this UBIFS implementation */
>> @@ -1454,6 +1455,7 @@ extern spinlock_t ubifs_infos_lock;
>> Âextern atomic_long_t ubifs_clean_zn_cnt;
>> Âextern struct kmem_cache *ubifs_inode_slab;
>> Âextern const struct super_operations ubifs_super_operations;
>> +extern const struct xattr_handler *ubifs_xattr_handlers[];
>> Âextern const struct address_space_operations ubifs_file_address_operations;
>> Âextern const struct file_operations ubifs_file_operations;
>> Âextern const struct inode_operations ubifs_file_inode_operations;
>> @@ -1736,10 +1738,17 @@ int ubifs_getattr(struct vfsmount *mnt, struct dentry *dentry,
>> Â Â Â Â Â Â Â Â struct kstat *stat);
>>
>> Â/* xattr.c */
>> +
>> Âint ubifs_setxattr(struct dentry *dentry, const char *name,
>> Â Â Â Â Â Â Â Â Âconst void *value, size_t size, int flags);
>> +int ubifs_init_security(struct inode *dentry, struct inode *inode,
>> + Â Â Â Â Â Â Â Â Â Â const struct qstr *qstr);
>> +int ubifs_symlink_setxattr(struct dentry *dentry, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Âconst void *value, size_t size, int flags);
>> Âssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
>> Â Â Â Â Â Â Â Â Â Â Âsize_t size);
>> +ssize_t ubifs_symlink_getxattr(struct dentry *dentry, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Âvoid *buf, size_t size);
>> Âssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size);
>> Âint ubifs_removexattr(struct dentry *dentry, const char *name);
>>
>> diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
>> index 85b2722..a402c7d 100644
>> --- a/fs/ubifs/xattr.c
>> +++ b/fs/ubifs/xattr.c
>> @@ -209,11 +209,11 @@ static int change_xattr(struct ubifs_info *c, struct inode *host,
>> Â Â Â Â Â Â Â goto out_free;
>> Â Â Â }
>> Â Â Â inode->i_size = ui->ui_size = size;
>> - Â Â ui->data_len = size;
>>
>> Â Â Â mutex_lock(&host_ui->ui_mutex);
>> Â Â Â host->i_ctime = ubifs_current_time(host);
>> Â Â Â host_ui->xattr_size -= CALC_XATTR_BYTES(ui->data_len);
>> + Â Â ui->data_len = size;
>> Â Â Â host_ui->xattr_size += CALC_XATTR_BYTES(size);
>>
>> Â Â Â /*
>> @@ -293,18 +293,16 @@ static struct inode *iget_xattr(struct ubifs_info *c, ino_t inum)
>> Â Â Â return ERR_PTR(-EINVAL);
>> Â}
>>
>> -int ubifs_setxattr(struct dentry *dentry, const char *name,
>> - Â Â Â Â Â Â Â Âconst void *value, size_t size, int flags)
>> +static int __ubifs_setxattr(struct inode *host, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Â const void *value, size_t size, int flags)
>> Â{
>> - Â Â struct inode *inode, *host = dentry->d_inode;
>> + Â Â struct inode *inode;
>> Â Â Â struct ubifs_info *c = host->i_sb->s_fs_info;
>> Â Â Â struct qstr nm = { .name = name, .len = strlen(name) };
>> Â Â Â struct ubifs_dent_node *xent;
>> Â Â Â union ubifs_key key;
>> Â Â Â int err, type;
>>
>> - Â Â dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name,
>> - Â Â Â Â Â Â host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
>> Â Â Â ubifs_assert(mutex_is_locked(&host->i_mutex));
>>
>> Â Â Â if (size > UBIFS_MAX_INO_DATA)
>> @@ -356,10 +354,29 @@ out_free:
>> Â Â Â return err;
>> Â}
>>
>> -ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
>> - Â Â Â Â Â Â Â Â Â Âsize_t size)
>> +int ubifs_symlink_setxattr(struct dentry *dentry, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Âconst void *value, size_t size, int flags)
>> Â{
>> - Â Â struct inode *inode, *host = dentry->d_inode;
>> + Â Â struct inode *host = dentry->d_inode;
>> + Â Â dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name,
>> + Â Â Â Â Â Â host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
>> + Â Â return __ubifs_setxattr(host, name, value, size, flags);
>> +}
>> +
>> +int ubifs_setxattr(struct dentry *dentry, const char *name,
>> + Â Â Â Â Â Â Â Âconst void *value, size_t size, int flags)
>> +{
>> + Â Â struct inode *host = dentry->d_inode;
>> + Â Â dbg_gen("xattr '%s', host ino %lu ('%.*s'), size %zd", name,
>> + Â Â Â Â Â Â host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
>> + Â Â return __ubifs_setxattr(host, name, value, size, flags);
>> +}
>> +
>> +static
>> +ssize_t __ubifs_getxattr(struct inode *host, const char *name, void *buf,
>> + Â Â Â Â Â Â Â Â Â Â Âsize_t size)
>> +{
>> + Â Â struct inode *inode;
>> Â Â Â struct ubifs_info *c = host->i_sb->s_fs_info;
>> Â Â Â struct qstr nm = { .name = name, .len = strlen(name) };
>> Â Â Â struct ubifs_inode *ui;
>> @@ -367,8 +384,8 @@ ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
>> Â Â Â union ubifs_key key;
>> Â Â Â int err;
>>
>> - Â Â dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name,
>> - Â Â Â Â Â Â host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
>> + Â Â dbg_gen("xattr '%s', ino %lu Âbuf size %zd", name,
>> + Â Â Â Â Â Â host->i_ino, size);
>>
>> Â Â Â err = check_namespace(&nm);
>> Â Â Â if (err < 0)
>> @@ -416,6 +433,25 @@ out_unlock:
>> Â Â Â return err;
>> Â}
>>
>> +ssize_t ubifs_symlink_getxattr(struct dentry *dentry, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Â Â Âvoid *buf, size_t size)
>> +{
>> + Â Â struct inode *host = dentry->d_inode;
>> + Â Â dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name,
>> + Â Â Â Â Â Â host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
>> + Â Â return __ubifs_getxattr(host, name, buf, size);
>> +}
>> +
>> +ssize_t ubifs_getxattr(struct dentry *dentry, const char *name, void *buf,
>> + Â Â Â Â Â Â Â Â Â Âsize_t size)
>> +{
>> + Â Â struct inode *host = dentry->d_inode;
>> + Â Â dbg_gen("xattr '%s', ino %lu ('%.*s'), buf size %zd", name,
>> + Â Â Â Â Â Â host->i_ino, dentry->d_name.len, dentry->d_name.name, size);
>> + Â Â return __ubifs_getxattr(host, name, buf, size);
>> +}
>> +
>> +
>> Âssize_t ubifs_listxattr(struct dentry *dentry, char *buffer, size_t size)
>> Â{
>> Â Â Â union ubifs_key key;
>> @@ -568,3 +604,92 @@ out_free:
>> Â Â Â kfree(xent);
>> Â Â Â return err;
>> Â}
>> +
>> +size_t
>> +ubifs_security_listxattr(struct dentry *d, char *list, size_t list_size,
>> + Â Â Â Â Â Â Â Â Â Â Âconst char *name, size_t name_len, int flags)
>> +{
>> + Â Â const int prefix_len = XATTR_SECURITY_PREFIX_LEN;
>> + Â Â const size_t total_len = prefix_len + name_len + 1;
>> + Â Â if (list && total_len <= list_size) {
>> + Â Â Â Â Â Â memcpy(list, XATTR_SECURITY_PREFIX, prefix_len);
>> + Â Â Â Â Â Â memcpy(list+prefix_len, name, name_len);
>> + Â Â Â Â Â Â list[prefix_len + name_len] = '\0';
>> + Â Â }
>> + Â Â return total_len;
>> +}
>> +
>> +
>> +int ubifs_security_getxattr(struct dentry *d, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Â void *buffer, size_t size, int flags)
>> +{
>> + Â Â if (strcmp(name, "") == 0)
>> + Â Â Â Â Â Â return -EINVAL;
>> + Â Â return __ubifs_getxattr(d->d_inode, XATTR_NAME_SELINUX, buffer, size);
>> +}
>> +
>> +
>> +int ubifs_security_setxattr(struct dentry *d, const char *name,
>> + Â Â Â Â Â Â Â Â Â Â Â Â const void *value, size_t size,
>> + Â Â Â Â Â Â Â Â Â Â Â Â int flags, int handler_flags)
>> +{
>> + Â Â if (strcmp(name, "") == 0)
>> + Â Â Â Â Â Â return -EINVAL;
>> + Â Â return __ubifs_setxattr(d->d_inode, XATTR_NAME_SELINUX, value,
>
> This is silly. If you're supporting xattrs there is no reason
> single out a particular LSM. Make this general so that Smack
> and any other LSM that wants to use xattrs will work.
>
> Move the changes into the SELinux LSM if you are going to
> hard code it.
>

Yup, my bad, following change fixes that specific issue and one can
set xattr for all security.* namespace. Will submit patch v2
shortly...

diff --git a/fs/ubifs/xattr.c b/fs/ubifs/xattr.c
index a402c7d..c8be7cd 100644
--- a/fs/ubifs/xattr.c
+++ b/fs/ubifs/xattr.c
@@ -625,7 +625,7 @@ int ubifs_security_getxattr(struct dentry *d,
const char *name,
{
if (strcmp(name, "") == 0)
return -EINVAL;
- return __ubifs_getxattr(d->d_inode, XATTR_NAME_SELINUX, buffer, size);
+ return __ubifs_getxattr(d->d_inode, name, buffer, size);
}

@@ -635,7 +635,7 @@ int ubifs_security_setxattr(struct dentry *d,
const char *name,
{
if (strcmp(name, "") == 0)
return -EINVAL;
- return __ubifs_setxattr(d->d_inode, XATTR_NAME_SELINUX, value,
+ return __ubifs_setxattr(d->d_inode, name, value,
size, flags);
}
--
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/

Next message: werner: "Re: v3.4-rc2 out-of-memory problems (was Re: 3.4-rc1sticks-and-crashs)"
Previous message: Jens Axboe: "Re: [PATCH v2 0/7] mtip32xx: Add support for device status and otherchanges"
In reply to: Casey Schaufler: "Re: [PATCH] Add security.selinux XATTR support for the UBIFS. Alsofix couple of bugs in UBIFS extended attribute length calculation."
Next in thread: Casey Schaufler: "Re: [PATCH] Add security.selinux XATTR support for the UBIFS. Alsofix couple of bugs in UBIFS extended attribute length calculation."
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]