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

Casey Schaufler casey at schaufler-ca.com
Mon Apr 9 11:08:31 EDT 2012


On 4/8/2012 11:40 PM, Subodh Nijsure wrote:
> On Sun, Apr 8, 2012 at 7:37 PM, Casey Schaufler <casey at schaufler-ca.com> 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 at grid-net.com>
>> 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 at scahufler-ca.com>
>>
>>> ---
>>>  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...

Thank you. The update looks fine, although I haven't tried it.

>
> 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);
>  }
>




More information about the linux-mtd mailing list