Re: [PATCH] tmpfs: implement security.capability xattrs

Re: [PATCH] tmpfs: implement security.capability xattrs

[Eric Paris]

Bueller?  Bueller?  Any thoughts?  Any problems?

On Tue, Jan 11, 2011 at 4:07 PM, Eric Paris <eparis@redhat.com> wrote:
> This patch implements security.capability xattrs for tmpfs filesystems.  The
> feodra project, while trying to replace suid apps with file capabilities,
> realized that tmpfs, which is used on my build systems, does not support file
> capabilities and thus cannot be used to build packages which use file
> capabilities.  The patch only implements security.capability but there is no
> reason it could not be easily expanded to support *.* xattrs as most of the
> work is already done.  I don't know what other xattrs are in use in the world
> or if they necessarily make sense on tmpfs so I didn't make this
> implementation completely generic.
>
> The basic implementation is that I attach a
> struct shmem_xattr {
>        struct list_head list; /* anchored by shmem_inode_info->xattr_list */
>        char *name;
>        size_t size;
>        char value[0];
> };
> Into the struct shmem_inode_info for each xattr that is set.  Since I only
> allow security.capability obviously this list is only every 0 or 1 entry long.
> I could have been a little simpler, but then the next person having to
> implement an xattr would have to redo everything I did instead of me just
> doing 90% of their work  :)
>
> Signed-off-by: Eric Paris <eparis@redhat.com>
> ---
>
>  include/linux/shmem_fs.h |    8 +++
>  mm/shmem.c               |  112 ++++++++++++++++++++++++++++++++++++++++++++--
>  2 files changed, 116 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> index 399be5a..6f2ebb8 100644
> --- a/include/linux/shmem_fs.h
> +++ b/include/linux/shmem_fs.h
> @@ -9,6 +9,13 @@
>
>  #define SHMEM_NR_DIRECT 16
>
> +struct shmem_xattr {
> +       struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> +       char *name;
> +       size_t size;
> +       char value[0];
> +};
> +
>  struct shmem_inode_info {
>        spinlock_t              lock;
>        unsigned long           flags;
> @@ -19,6 +26,7 @@ struct shmem_inode_info {
>        struct page             *i_indirect;    /* top indirect blocks page */
>        swp_entry_t             i_direct[SHMEM_NR_DIRECT]; /* first blocks */
>        struct list_head        swaplist;       /* chain of maybes on swap */
> +       struct list_head        xattr_list;     /* list of shmem_xattr */
>        struct inode            vfs_inode;
>  };
>
> diff --git a/mm/shmem.c b/mm/shmem.c
> index 86cd21d..d2bacd6 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -822,6 +822,7 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
>  static void shmem_evict_inode(struct inode *inode)
>  {
>        struct shmem_inode_info *info = SHMEM_I(inode);
> +       struct shmem_xattr *xattr, *nxattr;
>
>        if (inode->i_mapping->a_ops == &shmem_aops) {
>                truncate_inode_pages(inode->i_mapping, 0);
> @@ -834,6 +835,9 @@ static void shmem_evict_inode(struct inode *inode)
>                        mutex_unlock(&shmem_swaplist_mutex);
>                }
>        }
> +
> +       list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list)
> +               kfree(xattr);
>        BUG_ON(inode->i_blocks);
>        shmem_free_inode(inode->i_sb);
>        end_writeback(inode);
> @@ -1597,6 +1601,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
>                spin_lock_init(&info->lock);
>                info->flags = flags & VM_NORESERVE;
>                INIT_LIST_HEAD(&info->swaplist);
> +               INIT_LIST_HEAD(&info->xattr_list);
>                cache_no_acl(inode);
>
>                switch (mode & S_IFMT) {
> @@ -2071,24 +2076,123 @@ static size_t shmem_xattr_security_list(struct dentry *dentry, char *list,
>                                        size_t list_len, const char *name,
>                                        size_t name_len, int handler_flags)
>  {
> -       return security_inode_listsecurity(dentry->d_inode, list, list_len);
> +       struct shmem_xattr *xattr;
> +       struct shmem_inode_info *shmem_i;
> +       size_t used;
> +       char *buf = NULL;
> +
> +       used = security_inode_listsecurity(dentry->d_inode, list, list_len);
> +
> +       shmem_i = SHMEM_I(dentry->d_inode);
> +       if (list)
> +               buf = list + used;
> +
> +       spin_lock(&dentry->d_inode->i_lock);
> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> +               size_t len = XATTR_SECURITY_PREFIX_LEN;
> +               len += strlen(xattr->name) + 1;
> +               if (list_len - (used + len) >= 0 && buf) {
> +                       strncpy(buf, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
> +                       buf += XATTR_SECURITY_PREFIX_LEN;
> +                       strncpy(buf, xattr->name, strlen(xattr->name) + 1);
> +                       buf += strlen(xattr->name) + 1;
> +               }
> +               used += len;
> +       }
> +       spin_unlock(&dentry->d_inode->i_lock);
> +
> +       return used;
>  }
>
>  static int shmem_xattr_security_get(struct dentry *dentry, const char *name,
>                void *buffer, size_t size, int handler_flags)
>  {
> +       struct shmem_inode_info *shmem_i;
> +       struct shmem_xattr *xattr;
> +       int ret;
> +
>        if (strcmp(name, "") == 0)
>                return -EINVAL;
> -       return xattr_getsecurity(dentry->d_inode, name, buffer, size);
> +
> +       ret = xattr_getsecurity(dentry->d_inode, name, buffer, size);
> +       if (ret != -EOPNOTSUPP)
> +               return ret;
> +
> +       /* if we make this generic this needs to go... */
> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
> +               return -EOPNOTSUPP;
> +
> +       ret = -ENODATA;
> +       shmem_i = SHMEM_I(dentry->d_inode);
> +
> +       spin_lock(&dentry->d_inode->i_lock);
> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> +               if (!strcmp(name, xattr->name)) {
> +                       ret = xattr->size;
> +                       if (buffer) {
> +                               if (size < xattr->size)
> +                                       ret = -ERANGE;
> +                               else
> +                                       memcpy(buffer, xattr->value, xattr->size);
> +                       }
> +                       break;
> +               }
> +       }
> +       spin_unlock(&dentry->d_inode->i_lock);
> +       return ret;
>  }
>
>  static int shmem_xattr_security_set(struct dentry *dentry, const char *name,
>                const void *value, size_t size, int flags, int handler_flags)
>  {
> +       int ret;
> +       struct inode *inode = dentry->d_inode;
> +       struct shmem_inode_info *shmem_i = SHMEM_I(inode);
> +       struct shmem_xattr *xattr;
> +       struct shmem_xattr *new_xattr;
> +       size_t len;
> +
>        if (strcmp(name, "") == 0)
>                return -EINVAL;
> -       return security_inode_setsecurity(dentry->d_inode, name, value,
> -                                         size, flags);
> +       ret = security_inode_setsecurity(inode, name, value, size, flags);
> +       if (ret != -EOPNOTSUPP)
> +               return ret;
> +
> +       /*
> +        * We only store fcaps for now, but this could be a lot more generic.
> +        * We could hold the prefix as well as the suffix in the xattr struct
> +        * We would also need to hold a copy of the suffix rather than a
> +        * pointer to XATTR_CAPS_SUFFIX
> +        */
> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
> +               return -EOPNOTSUPP;
> +
> +       /* wrap around? */
> +       len = sizeof(*new_xattr) + size;
> +       if (len <= sizeof(*new_xattr))
> +               return -ENOMEM;
> +
> +       new_xattr = kmalloc(GFP_NOFS, len);
> +       if (!new_xattr)
> +               return -ENOMEM;
> +
> +       new_xattr->name = XATTR_CAPS_SUFFIX;
> +       new_xattr->size = size;
> +       memcpy(new_xattr->value, value, size);
> +
> +       spin_lock(&inode->i_lock);
> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> +               if (!strcmp(name, xattr->name)) {
> +                       list_replace(&xattr->list, &new_xattr->list);
> +                       goto out;
> +               }
> +       }
> +       list_add(&new_xattr->list, &shmem_i->xattr_list);
> +       xattr = NULL;
> +out:
> +       spin_unlock(&inode->i_lock);
> +       kfree(xattr);
> +       return 0;
>  }
>
>  static const struct xattr_handler shmem_xattr_security_handler = {
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] tmpfs: implement security.capability xattrs

[Eric Paris]

I know there exist thoughts on this patch somewhere on the internets.
Let 'em rip!  I can handle it!

-Eric

On Thu, Feb 17, 2011 at 4:27 PM, Eric Paris <eparis@parisplace.org> wrote:
> Bueller?  Bueller?  Any thoughts?  Any problems?
>
> On Tue, Jan 11, 2011 at 4:07 PM, Eric Paris <eparis@redhat.com> wrote:
>> This patch implements security.capability xattrs for tmpfs filesystems.  The
>> feodra project, while trying to replace suid apps with file capabilities,
>> realized that tmpfs, which is used on my build systems, does not support file
>> capabilities and thus cannot be used to build packages which use file
>> capabilities.  The patch only implements security.capability but there is no
>> reason it could not be easily expanded to support *.* xattrs as most of the
>> work is already done.  I don't know what other xattrs are in use in the world
>> or if they necessarily make sense on tmpfs so I didn't make this
>> implementation completely generic.
>>
>> The basic implementation is that I attach a
>> struct shmem_xattr {
>>        struct list_head list; /* anchored by shmem_inode_info->xattr_list */
>>        char *name;
>>        size_t size;
>>        char value[0];
>> };
>> Into the struct shmem_inode_info for each xattr that is set.  Since I only
>> allow security.capability obviously this list is only every 0 or 1 entry long.
>> I could have been a little simpler, but then the next person having to
>> implement an xattr would have to redo everything I did instead of me just
>> doing 90% of their work  :)
>>
>> Signed-off-by: Eric Paris <eparis@redhat.com>
>> ---
>>
>>  include/linux/shmem_fs.h |    8 +++
>>  mm/shmem.c               |  112 ++++++++++++++++++++++++++++++++++++++++++++--
>>  2 files changed, 116 insertions(+), 4 deletions(-)
>>
>> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
>> index 399be5a..6f2ebb8 100644
>> --- a/include/linux/shmem_fs.h
>> +++ b/include/linux/shmem_fs.h
>> @@ -9,6 +9,13 @@
>>
>>  #define SHMEM_NR_DIRECT 16
>>
>> +struct shmem_xattr {
>> +       struct list_head list; /* anchored by shmem_inode_info->xattr_list */
>> +       char *name;
>> +       size_t size;
>> +       char value[0];
>> +};
>> +
>>  struct shmem_inode_info {
>>        spinlock_t              lock;
>>        unsigned long           flags;
>> @@ -19,6 +26,7 @@ struct shmem_inode_info {
>>        struct page             *i_indirect;    /* top indirect blocks page */
>>        swp_entry_t             i_direct[SHMEM_NR_DIRECT]; /* first blocks */
>>        struct list_head        swaplist;       /* chain of maybes on swap */
>> +       struct list_head        xattr_list;     /* list of shmem_xattr */
>>        struct inode            vfs_inode;
>>  };
>>
>> diff --git a/mm/shmem.c b/mm/shmem.c
>> index 86cd21d..d2bacd6 100644
>> --- a/mm/shmem.c
>> +++ b/mm/shmem.c
>> @@ -822,6 +822,7 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
>>  static void shmem_evict_inode(struct inode *inode)
>>  {
>>        struct shmem_inode_info *info = SHMEM_I(inode);
>> +       struct shmem_xattr *xattr, *nxattr;
>>
>>        if (inode->i_mapping->a_ops == &shmem_aops) {
>>                truncate_inode_pages(inode->i_mapping, 0);
>> @@ -834,6 +835,9 @@ static void shmem_evict_inode(struct inode *inode)
>>                        mutex_unlock(&shmem_swaplist_mutex);
>>                }
>>        }
>> +
>> +       list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list)
>> +               kfree(xattr);
>>        BUG_ON(inode->i_blocks);
>>        shmem_free_inode(inode->i_sb);
>>        end_writeback(inode);
>> @@ -1597,6 +1601,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
>>                spin_lock_init(&info->lock);
>>                info->flags = flags & VM_NORESERVE;
>>                INIT_LIST_HEAD(&info->swaplist);
>> +               INIT_LIST_HEAD(&info->xattr_list);
>>                cache_no_acl(inode);
>>
>>                switch (mode & S_IFMT) {
>> @@ -2071,24 +2076,123 @@ static size_t shmem_xattr_security_list(struct dentry *dentry, char *list,
>>                                        size_t list_len, const char *name,
>>                                        size_t name_len, int handler_flags)
>>  {
>> -       return security_inode_listsecurity(dentry->d_inode, list, list_len);
>> +       struct shmem_xattr *xattr;
>> +       struct shmem_inode_info *shmem_i;
>> +       size_t used;
>> +       char *buf = NULL;
>> +
>> +       used = security_inode_listsecurity(dentry->d_inode, list, list_len);
>> +
>> +       shmem_i = SHMEM_I(dentry->d_inode);
>> +       if (list)
>> +               buf = list + used;
>> +
>> +       spin_lock(&dentry->d_inode->i_lock);
>> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
>> +               size_t len = XATTR_SECURITY_PREFIX_LEN;
>> +               len += strlen(xattr->name) + 1;
>> +               if (list_len - (used + len) >= 0 && buf) {
>> +                       strncpy(buf, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
>> +                       buf += XATTR_SECURITY_PREFIX_LEN;
>> +                       strncpy(buf, xattr->name, strlen(xattr->name) + 1);
>> +                       buf += strlen(xattr->name) + 1;
>> +               }
>> +               used += len;
>> +       }
>> +       spin_unlock(&dentry->d_inode->i_lock);
>> +
>> +       return used;
>>  }
>>
>>  static int shmem_xattr_security_get(struct dentry *dentry, const char *name,
>>                void *buffer, size_t size, int handler_flags)
>>  {
>> +       struct shmem_inode_info *shmem_i;
>> +       struct shmem_xattr *xattr;
>> +       int ret;
>> +
>>        if (strcmp(name, "") == 0)
>>                return -EINVAL;
>> -       return xattr_getsecurity(dentry->d_inode, name, buffer, size);
>> +
>> +       ret = xattr_getsecurity(dentry->d_inode, name, buffer, size);
>> +       if (ret != -EOPNOTSUPP)
>> +               return ret;
>> +
>> +       /* if we make this generic this needs to go... */
>> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
>> +               return -EOPNOTSUPP;
>> +
>> +       ret = -ENODATA;
>> +       shmem_i = SHMEM_I(dentry->d_inode);
>> +
>> +       spin_lock(&dentry->d_inode->i_lock);
>> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
>> +               if (!strcmp(name, xattr->name)) {
>> +                       ret = xattr->size;
>> +                       if (buffer) {
>> +                               if (size < xattr->size)
>> +                                       ret = -ERANGE;
>> +                               else
>> +                                       memcpy(buffer, xattr->value, xattr->size);
>> +                       }
>> +                       break;
>> +               }
>> +       }
>> +       spin_unlock(&dentry->d_inode->i_lock);
>> +       return ret;
>>  }
>>
>>  static int shmem_xattr_security_set(struct dentry *dentry, const char *name,
>>                const void *value, size_t size, int flags, int handler_flags)
>>  {
>> +       int ret;
>> +       struct inode *inode = dentry->d_inode;
>> +       struct shmem_inode_info *shmem_i = SHMEM_I(inode);
>> +       struct shmem_xattr *xattr;
>> +       struct shmem_xattr *new_xattr;
>> +       size_t len;
>> +
>>        if (strcmp(name, "") == 0)
>>                return -EINVAL;
>> -       return security_inode_setsecurity(dentry->d_inode, name, value,
>> -                                         size, flags);
>> +       ret = security_inode_setsecurity(inode, name, value, size, flags);
>> +       if (ret != -EOPNOTSUPP)
>> +               return ret;
>> +
>> +       /*
>> +        * We only store fcaps for now, but this could be a lot more generic.
>> +        * We could hold the prefix as well as the suffix in the xattr struct
>> +        * We would also need to hold a copy of the suffix rather than a
>> +        * pointer to XATTR_CAPS_SUFFIX
>> +        */
>> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
>> +               return -EOPNOTSUPP;
>> +
>> +       /* wrap around? */
>> +       len = sizeof(*new_xattr) + size;
>> +       if (len <= sizeof(*new_xattr))
>> +               return -ENOMEM;
>> +
>> +       new_xattr = kmalloc(GFP_NOFS, len);
>> +       if (!new_xattr)
>> +               return -ENOMEM;
>> +
>> +       new_xattr->name = XATTR_CAPS_SUFFIX;
>> +       new_xattr->size = size;
>> +       memcpy(new_xattr->value, value, size);
>> +
>> +       spin_lock(&inode->i_lock);
>> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
>> +               if (!strcmp(name, xattr->name)) {
>> +                       list_replace(&xattr->list, &new_xattr->list);
>> +                       goto out;
>> +               }
>> +       }
>> +       list_add(&new_xattr->list, &shmem_i->xattr_list);
>> +       xattr = NULL;
>> +out:
>> +       spin_unlock(&inode->i_lock);
>> +       kfree(xattr);
>> +       return 0;
>>  }
>>
>>  static const struct xattr_handler shmem_xattr_security_handler = {
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>> Please read the FAQ at  http://www.tux.org/lkml/
>>
>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] tmpfs: implement security.capability xattrs

[Bruno Prémont]

On Wed, 02 March 2011 Eric Paris <eparis@parisplace.org> wrote:
> I know there exist thoughts on this patch somewhere on the internets.
> Let 'em rip!  I can handle it!

Hi Eric,

I have not read the code behind CONFIG_TMPFS_POSIX_ACL in depth but it
does seem to already use some XATTR support for making posix acls
available.

Your patch looks like not touching/using that support, maybe there is
already some of your work previously done (according to comment in
mm/shmem.c offered for free by VFS).

Did I miss something essential?

Regards,
Bruno

> -Eric
> 
> On Thu, Feb 17, 2011 at 4:27 PM, Eric Paris <eparis@parisplace.org> wrote:
> > Bueller?  Bueller?  Any thoughts?  Any problems?
> >
> > On Tue, Jan 11, 2011 at 4:07 PM, Eric Paris <eparis@redhat.com> wrote:
> >> This patch implements security.capability xattrs for tmpfs filesystems.  The
> >> feodra project, while trying to replace suid apps with file capabilities,
> >> realized that tmpfs, which is used on my build systems, does not support file
> >> capabilities and thus cannot be used to build packages which use file
> >> capabilities.  The patch only implements security.capability but there is no
> >> reason it could not be easily expanded to support *.* xattrs as most of the
> >> work is already done.  I don't know what other xattrs are in use in the world
> >> or if they necessarily make sense on tmpfs so I didn't make this
> >> implementation completely generic.
> >>
> >> The basic implementation is that I attach a
> >> struct shmem_xattr {
> >>        struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> >>        char *name;
> >>        size_t size;
> >>        char value[0];
> >> };
> >> Into the struct shmem_inode_info for each xattr that is set.  Since I only
> >> allow security.capability obviously this list is only every 0 or 1 entry long.
> >> I could have been a little simpler, but then the next person having to
> >> implement an xattr would have to redo everything I did instead of me just
> >> doing 90% of their work  :)
> >>
> >> Signed-off-by: Eric Paris <eparis@redhat.com>
> >> ---
> >>
> >>  include/linux/shmem_fs.h |    8 +++
> >>  mm/shmem.c               |  112 ++++++++++++++++++++++++++++++++++++++++++++--
> >>  2 files changed, 116 insertions(+), 4 deletions(-)
> >>
> >> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> >> index 399be5a..6f2ebb8 100644
> >> --- a/include/linux/shmem_fs.h
> >> +++ b/include/linux/shmem_fs.h
> >> @@ -9,6 +9,13 @@
> >>
> >>  #define SHMEM_NR_DIRECT 16
> >>
> >> +struct shmem_xattr {
> >> +       struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> >> +       char *name;
> >> +       size_t size;
> >> +       char value[0];
> >> +};
> >> +
> >>  struct shmem_inode_info {
> >>        spinlock_t              lock;
> >>        unsigned long           flags;
> >> @@ -19,6 +26,7 @@ struct shmem_inode_info {
> >>        struct page             *i_indirect;    /* top indirect blocks page */
> >>        swp_entry_t             i_direct[SHMEM_NR_DIRECT]; /* first blocks */
> >>        struct list_head        swaplist;       /* chain of maybes on swap */
> >> +       struct list_head        xattr_list;     /* list of shmem_xattr */
> >>        struct inode            vfs_inode;
> >>  };
> >>
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index 86cd21d..d2bacd6 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -822,6 +822,7 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
> >>  static void shmem_evict_inode(struct inode *inode)
> >>  {
> >>        struct shmem_inode_info *info = SHMEM_I(inode);
> >> +       struct shmem_xattr *xattr, *nxattr;
> >>
> >>        if (inode->i_mapping->a_ops == &shmem_aops) {
> >>                truncate_inode_pages(inode->i_mapping, 0);
> >> @@ -834,6 +835,9 @@ static void shmem_evict_inode(struct inode *inode)
> >>                        mutex_unlock(&shmem_swaplist_mutex);
> >>                }
> >>        }
> >> +
> >> +       list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list)
> >> +               kfree(xattr);
> >>        BUG_ON(inode->i_blocks);
> >>        shmem_free_inode(inode->i_sb);
> >>        end_writeback(inode);
> >> @@ -1597,6 +1601,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
> >>                spin_lock_init(&info->lock);
> >>                info->flags = flags & VM_NORESERVE;
> >>                INIT_LIST_HEAD(&info->swaplist);
> >> +               INIT_LIST_HEAD(&info->xattr_list);
> >>                cache_no_acl(inode);
> >>
> >>                switch (mode & S_IFMT) {
> >> @@ -2071,24 +2076,123 @@ static size_t shmem_xattr_security_list(struct dentry *dentry, char *list,
> >>                                        size_t list_len, const char *name,
> >>                                        size_t name_len, int handler_flags)
> >>  {
> >> -       return security_inode_listsecurity(dentry->d_inode, list, list_len);
> >> +       struct shmem_xattr *xattr;
> >> +       struct shmem_inode_info *shmem_i;
> >> +       size_t used;
> >> +       char *buf = NULL;
> >> +
> >> +       used = security_inode_listsecurity(dentry->d_inode, list, list_len);
> >> +
> >> +       shmem_i = SHMEM_I(dentry->d_inode);
> >> +       if (list)
> >> +               buf = list + used;
> >> +
> >> +       spin_lock(&dentry->d_inode->i_lock);
> >> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> +               size_t len = XATTR_SECURITY_PREFIX_LEN;
> >> +               len += strlen(xattr->name) + 1;
> >> +               if (list_len - (used + len) >= 0 && buf) {
> >> +                       strncpy(buf, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
> >> +                       buf += XATTR_SECURITY_PREFIX_LEN;
> >> +                       strncpy(buf, xattr->name, strlen(xattr->name) + 1);
> >> +                       buf += strlen(xattr->name) + 1;
> >> +               }
> >> +               used += len;
> >> +       }
> >> +       spin_unlock(&dentry->d_inode->i_lock);
> >> +
> >> +       return used;
> >>  }
> >>
> >>  static int shmem_xattr_security_get(struct dentry *dentry, const char *name,
> >>                void *buffer, size_t size, int handler_flags)
> >>  {
> >> +       struct shmem_inode_info *shmem_i;
> >> +       struct shmem_xattr *xattr;
> >> +       int ret;
> >> +
> >>        if (strcmp(name, "") == 0)
> >>                return -EINVAL;
> >> -       return xattr_getsecurity(dentry->d_inode, name, buffer, size);
> >> +
> >> +       ret = xattr_getsecurity(dentry->d_inode, name, buffer, size);
> >> +       if (ret != -EOPNOTSUPP)
> >> +               return ret;
> >> +
> >> +       /* if we make this generic this needs to go... */
> >> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       ret = -ENODATA;
> >> +       shmem_i = SHMEM_I(dentry->d_inode);
> >> +
> >> +       spin_lock(&dentry->d_inode->i_lock);
> >> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> +               if (!strcmp(name, xattr->name)) {
> >> +                       ret = xattr->size;
> >> +                       if (buffer) {
> >> +                               if (size < xattr->size)
> >> +                                       ret = -ERANGE;
> >> +                               else
> >> +                                       memcpy(buffer, xattr->value, xattr->size);
> >> +                       }
> >> +                       break;
> >> +               }
> >> +       }
> >> +       spin_unlock(&dentry->d_inode->i_lock);
> >> +       return ret;
> >>  }
> >>
> >>  static int shmem_xattr_security_set(struct dentry *dentry, const char *name,
> >>                const void *value, size_t size, int flags, int handler_flags)
> >>  {
> >> +       int ret;
> >> +       struct inode *inode = dentry->d_inode;
> >> +       struct shmem_inode_info *shmem_i = SHMEM_I(inode);
> >> +       struct shmem_xattr *xattr;
> >> +       struct shmem_xattr *new_xattr;
> >> +       size_t len;
> >> +
> >>        if (strcmp(name, "") == 0)
> >>                return -EINVAL;
> >> -       return security_inode_setsecurity(dentry->d_inode, name, value,
> >> -                                         size, flags);
> >> +       ret = security_inode_setsecurity(inode, name, value, size, flags);
> >> +       if (ret != -EOPNOTSUPP)
> >> +               return ret;
> >> +
> >> +       /*
> >> +        * We only store fcaps for now, but this could be a lot more generic.
> >> +        * We could hold the prefix as well as the suffix in the xattr struct
> >> +        * We would also need to hold a copy of the suffix rather than a
> >> +        * pointer to XATTR_CAPS_SUFFIX
> >> +        */
> >> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       /* wrap around? */
> >> +       len = sizeof(*new_xattr) + size;
> >> +       if (len <= sizeof(*new_xattr))
> >> +               return -ENOMEM;
> >> +
> >> +       new_xattr = kmalloc(GFP_NOFS, len);
> >> +       if (!new_xattr)
> >> +               return -ENOMEM;
> >> +
> >> +       new_xattr->name = XATTR_CAPS_SUFFIX;
> >> +       new_xattr->size = size;
> >> +       memcpy(new_xattr->value, value, size);
> >> +
> >> +       spin_lock(&inode->i_lock);
> >> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> +               if (!strcmp(name, xattr->name)) {
> >> +                       list_replace(&xattr->list, &new_xattr->list);
> >> +                       goto out;
> >> +               }
> >> +       }
> >> +       list_add(&new_xattr->list, &shmem_i->xattr_list);
> >> +       xattr = NULL;
> >> +out:
> >> +       spin_unlock(&inode->i_lock);
> >> +       kfree(xattr);
> >> +       return 0;
> >>  }
> >>
> >>  static const struct xattr_handler shmem_xattr_security_handler = {
> >>

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] tmpfs: implement security.capability xattrs

[Jason L Tibbitts III]

>>>>> "EP" == Eric Paris <eparis@parisplace.org> writes:

EP> I know there exist thoughts on this patch somewhere on the
EP> internets. Let 'em rip!  I can handle it!

Well, I've been running it for a while (currently patched into Fedora
15's 2.6.38 package) in order to be able to init Fedora chroots in tmpfs
(for doing fast mock builds).  Seems to work fine for me.  Unfortunately
I'm not able to comment on the patch itself, which I guess is what it
really needs in order to make it upstream.

 - J<

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

Re: [PATCH] tmpfs: implement security.capability xattrs

[Hugh Dickins]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 12763 bytes --]

On Wed, 2 Mar 2011, Eric Paris wrote:
> I know there exist thoughts on this patch somewhere on the internets.
> Let 'em rip!  I can handle it!
> 
> -Eric
> 
> On Thu, Feb 17, 2011 at 4:27 PM, Eric Paris <eparis@parisplace.org> wrote:
> > Bueller?  Bueller?  Any thoughts?  Any problems?
> >

Sorry, Eric, I did spot it months ago, kept on picking it up and
putting it down, never quite got to grips with it.  I did try it out,
and so far as I could tell, it was working correctly.

> > On Tue, Jan 11, 2011 at 4:07 PM, Eric Paris <eparis@redhat.com> wrote:
> >> This patch implements security.capability xattrs for tmpfs filesystems.  The
> >> feodra project, while trying to replace suid apps with file capabilities,
> >> realized that tmpfs, which is used on my build systems, does not support file
> >> capabilities and thus cannot be used to build packages which use file
> >> capabilities.  The patch only implements security.capability but there is no
> >> reason it could not be easily expanded to support *.* xattrs as most of the
> >> work is already done.  I don't know what other xattrs are in use in the world
> >> or if they necessarily make sense on tmpfs so I didn't make this
> >> implementation completely generic.
> >>
> >> The basic implementation is that I attach a
> >> struct shmem_xattr {
> >>        struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> >>        char *name;
> >>        size_t size;
> >>        char value[0];
> >> };
> >> Into the struct shmem_inode_info for each xattr that is set.  Since I only
> >> allow security.capability obviously this list is only every 0 or 1 entry long.
> >> I could have been a little simpler, but then the next person having to
> >> implement an xattr would have to redo everything I did instead of me just
> >> doing 90% of their work  :)
> >>
> >> Signed-off-by: Eric Paris <eparis@redhat.com>

I'm unfamiliar with xattrs, and found the security hooks, the way we
dip into and out of them, quite confusing: not to mean that you need
to add lots of comments, no, so long as it works, and is what people
familiar the territory expect, that's okay.

We do like tmpfs to be useful, but it was unclear to me from your
comments above, whether this is just a toy implementation good for
packaging, or a real implementation of security.capability.  I hope
the latter - we do not want something half-baked that will cause
trouble by breaking expectations down the line.

If you can get Acks from James and Christoph, both of whom have been
here before, then it's mostly fine by me; but a few comments below.

> >> ---
> >>
> >>  include/linux/shmem_fs.h |    8 +++
> >>  mm/shmem.c               |  112 ++++++++++++++++++++++++++++++++++++++++++++--
> >>  2 files changed, 116 insertions(+), 4 deletions(-)

No change to fs/Kconfig?  You seem to smuggle the xattr and security
support in under CONFIG_TMPFS_POSIX_ACL, and leave it unsupported
without.  It's probably a fair assumption that the people with that
option selected are the people who will be interested in this, so
no need for the maze of separate config options which a grownup
filesystem would have here.  But at the very least you need to say
more in the TMPFS_POSIX_ACL Kconfig entry (a new name may be more
trouble than it's worth).

> >>
> >> diff --git a/include/linux/shmem_fs.h b/include/linux/shmem_fs.h
> >> index 399be5a..6f2ebb8 100644
> >> --- a/include/linux/shmem_fs.h
> >> +++ b/include/linux/shmem_fs.h
> >> @@ -9,6 +9,13 @@
> >>
> >>  #define SHMEM_NR_DIRECT 16
> >>
> >> +struct shmem_xattr {
> >> +       struct list_head list; /* anchored by shmem_inode_info->xattr_list */
> >> +       char *name;
> >> +       size_t size;
> >> +       char value[0];
> >> +};
> >> +
> >>  struct shmem_inode_info {
> >>        spinlock_t              lock;
> >>        unsigned long           flags;
> >> @@ -19,6 +26,7 @@ struct shmem_inode_info {
> >>        struct page             *i_indirect;    /* top indirect blocks page */
> >>        swp_entry_t             i_direct[SHMEM_NR_DIRECT]; /* first blocks */
> >>        struct list_head        swaplist;       /* chain of maybes on swap */
> >> +       struct list_head        xattr_list;     /* list of shmem_xattr */
> >>        struct inode            vfs_inode;
> >>  };
> >>
> >> diff --git a/mm/shmem.c b/mm/shmem.c
> >> index 86cd21d..d2bacd6 100644
> >> --- a/mm/shmem.c
> >> +++ b/mm/shmem.c
> >> @@ -822,6 +822,7 @@ static int shmem_notify_change(struct dentry *dentry, struct iattr *attr)
> >>  static void shmem_evict_inode(struct inode *inode)
> >>  {
> >>        struct shmem_inode_info *info = SHMEM_I(inode);
> >> +       struct shmem_xattr *xattr, *nxattr;
> >>
> >>        if (inode->i_mapping->a_ops == &shmem_aops) {
> >>                truncate_inode_pages(inode->i_mapping, 0);
> >> @@ -834,6 +835,9 @@ static void shmem_evict_inode(struct inode *inode)
> >>                        mutex_unlock(&shmem_swaplist_mutex);
> >>                }
> >>        }
> >> +
> >> +       list_for_each_entry_safe(xattr, nxattr, &info->xattr_list, list)
> >> +               kfree(xattr);
> >>        BUG_ON(inode->i_blocks);
> >>        shmem_free_inode(inode->i_sb);
> >>        end_writeback(inode);
> >> @@ -1597,6 +1601,7 @@ static struct inode *shmem_get_inode(struct super_block *sb, const struct inode
> >>                spin_lock_init(&info->lock);
> >>                info->flags = flags & VM_NORESERVE;
> >>                INIT_LIST_HEAD(&info->swaplist);
> >> +               INIT_LIST_HEAD(&info->xattr_list);
> >>                cache_no_acl(inode);
> >>
> >>                switch (mode & S_IFMT) {
> >> @@ -2071,24 +2076,123 @@ static size_t shmem_xattr_security_list(struct dentry *dentry, char *list,
> >>                                        size_t list_len, const char *name,
> >>                                        size_t name_len, int handler_flags)
> >>  {
> >> -       return security_inode_listsecurity(dentry->d_inode, list, list_len);
> >> +       struct shmem_xattr *xattr;
> >> +       struct shmem_inode_info *shmem_i;

It's a nit, but (almost) everywhere else in shmem.c the shmem_inode_info
pointer is known as "info": easy for me to fix up if I care, but nicer
if you follow local custom.

> >> +       size_t used;
> >> +       char *buf = NULL;
> >> +
> >> +       used = security_inode_listsecurity(dentry->d_inode, list, list_len);
> >> +
> >> +       shmem_i = SHMEM_I(dentry->d_inode);
> >> +       if (list)
> >> +               buf = list + used;

This is the place that caused me most trouble.  On a minor note:
it worried me that security_inode_listsecurity() might return an
error, whereas I think you know and assume that the worst it can
return is 0 - might be worth a comment.

But more major: I found it very odd that you collect one set of things
from security_inode_listsecurity(), then proceed to tack on some more
below from the shmem inode.  I looked at other filesystems (well, ext2!)
and couldn't find a precedent.  What's this about?  Is it because other
filesystems have an on-disk format which determines what they're capable
of, whereas tmpfs is plastic and can reflect what the running system has?
Or is it to allow for future xattrs which might be added to tmpfs, but
frankly I'd rather do without until they're defined?

If it needs to be like this, then please, I do want a comment on
what's going on here.  If it need not be like this, then please
delete what's not needed.

> >> +
> >> +       spin_lock(&dentry->d_inode->i_lock);
> >> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> +               size_t len = XATTR_SECURITY_PREFIX_LEN;
> >> +               len += strlen(xattr->name) + 1;
> >> +               if (list_len - (used + len) >= 0 && buf) {
> >> +                       strncpy(buf, XATTR_SECURITY_PREFIX, XATTR_SECURITY_PREFIX_LEN);
> >> +                       buf += XATTR_SECURITY_PREFIX_LEN;
> >> +                       strncpy(buf, xattr->name, strlen(xattr->name) + 1);
> >> +                       buf += strlen(xattr->name) + 1;
> >> +               }
> >> +               used += len;
> >> +       }
> >> +       spin_unlock(&dentry->d_inode->i_lock);
> >> +
> >> +       return used;
> >>  }
> >>
> >>  static int shmem_xattr_security_get(struct dentry *dentry, const char *name,
> >>                void *buffer, size_t size, int handler_flags)
> >>  {
> >> +       struct shmem_inode_info *shmem_i;

"info" as above.

> >> +       struct shmem_xattr *xattr;
> >> +       int ret;
> >> +
> >>        if (strcmp(name, "") == 0)
> >>                return -EINVAL;
> >> -       return xattr_getsecurity(dentry->d_inode, name, buffer, size);
> >> +
> >> +       ret = xattr_getsecurity(dentry->d_inode, name, buffer, size);
> >> +       if (ret != -EOPNOTSUPP)
> >> +               return ret;
> >> +
> >> +       /* if we make this generic this needs to go... */
> >> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       ret = -ENODATA;
> >> +       shmem_i = SHMEM_I(dentry->d_inode);
> >> +
> >> +       spin_lock(&dentry->d_inode->i_lock);
> >> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> +               if (!strcmp(name, xattr->name)) {
> >> +                       ret = xattr->size;
> >> +                       if (buffer) {
> >> +                               if (size < xattr->size)
> >> +                                       ret = -ERANGE;
> >> +                               else
> >> +                                       memcpy(buffer, xattr->value, xattr->size);
> >> +                       }
> >> +                       break;
> >> +               }
> >> +       }
> >> +       spin_unlock(&dentry->d_inode->i_lock);
> >> +       return ret;
> >>  }
> >>
> >>  static int shmem_xattr_security_set(struct dentry *dentry, const char *name,
> >>                const void *value, size_t size, int flags, int handler_flags)
> >>  {
> >> +       int ret;
> >> +       struct inode *inode = dentry->d_inode;
> >> +       struct shmem_inode_info *shmem_i = SHMEM_I(inode);

"info" as above.

> >> +       struct shmem_xattr *xattr;
> >> +       struct shmem_xattr *new_xattr;
> >> +       size_t len;
> >> +
> >>        if (strcmp(name, "") == 0)
> >>                return -EINVAL;
> >> -       return security_inode_setsecurity(dentry->d_inode, name, value,
> >> -                                         size, flags);
> >> +       ret = security_inode_setsecurity(inode, name, value, size, flags);
> >> +       if (ret != -EOPNOTSUPP)
> >> +               return ret;
> >> +
> >> +       /*
> >> +        * We only store fcaps for now, but this could be a lot more generic.
> >> +        * We could hold the prefix as well as the suffix in the xattr struct
> >> +        * We would also need to hold a copy of the suffix rather than a
> >> +        * pointer to XATTR_CAPS_SUFFIX
> >> +        */
> >> +       if (strcmp(name, XATTR_CAPS_SUFFIX))
> >> +               return -EOPNOTSUPP;
> >> +
> >> +       /* wrap around? */
> >> +       len = sizeof(*new_xattr) + size;
> >> +       if (len <= sizeof(*new_xattr))
> >> +               return -ENOMEM;
> >> +
> >> +       new_xattr = kmalloc(GFP_NOFS, len);
> >> +       if (!new_xattr)
> >> +               return -ENOMEM;
> >> +
> >> +       new_xattr->name = XATTR_CAPS_SUFFIX;
> >> +       new_xattr->size = size;
> >> +       memcpy(new_xattr->value, value, size);
> >> +
> >> +       spin_lock(&inode->i_lock);
> >> +       list_for_each_entry(xattr, &shmem_i->xattr_list, list) {
> >> +               if (!strcmp(name, xattr->name)) {
> >> +                       list_replace(&xattr->list, &new_xattr->list);
> >> +                       goto out;
> >> +               }
> >> +       }
> >> +       list_add(&new_xattr->list, &shmem_i->xattr_list);
> >> +       xattr = NULL;
> >> +out:
> >> +       spin_unlock(&inode->i_lock);
> >> +       kfree(xattr);
> >> +       return 0;
> >>  }
> >>
> >>  static const struct xattr_handler shmem_xattr_security_handler = {

I'm sorry if my incomprehension depresses you: it did me!

You'll laugh or cry if I admit to you that I was naive enough
to believe that comment above shmem_xattr_security_list() which says
 * Superblocks without xattr inode operations will get security.* xattr
 * support from the VFS "for free".
and wondered why you had to add any code at all.  Maybe you could say
something better there.

Thanks,
Hugh

Re: [PATCH] tmpfs: implement security.capability xattrs

[Eric Paris]

On Sun, 2011-03-20 at 22:17 -0700, Hugh Dickins wrote:
> On Wed, 2 Mar 2011, Eric Paris wrote:

> > >>  include/linux/shmem_fs.h |    8 +++
> > >>  mm/shmem.c               |  112 ++++++++++++++++++++++++++++++++++++++++++++--
> > >>  2 files changed, 116 insertions(+), 4 deletions(-)
> 
> No change to fs/Kconfig?  You seem to smuggle the xattr and security
> support in under CONFIG_TMPFS_POSIX_ACL, and leave it unsupported
> without.  It's probably a fair assumption that the people with that
> option selected are the people who will be interested in this, so
> no need for the maze of separate config options which a grownup
> filesystem would have here.  But at the very least you need to say
> more in the TMPFS_POSIX_ACL Kconfig entry (a new name may be more
> trouble than it's worth).

Will update text.

> > >> @@ -2071,24 +2076,123 @@ static size_t shmem_xattr_security_list(struct dentry *dentry, char *list,
> > >>                                        size_t list_len, const char *name,
> > >>                                        size_t name_len, int handler_flags)
> > >>  {
> > >> -       return security_inode_listsecurity(dentry->d_inode, list, list_len);
> > >> +       struct shmem_xattr *xattr;
> > >> +       struct shmem_inode_info *shmem_i;
> 
> It's a nit, but (almost) everywhere else in shmem.c the shmem_inode_info
> pointer is known as "info": easy for me to fix up if I care, but nicer
> if you follow local custom.

Will fix.  I certainly don't like breaking conventions needlessly.

> > >> +       size_t used;
> > >> +       char *buf = NULL;
> > >> +
> > >> +       used = security_inode_listsecurity(dentry->d_inode, list, list_len);
> > >> +
> > >> +       shmem_i = SHMEM_I(dentry->d_inode);
> > >> +       if (list)
> > >> +               buf = list + used;
> 
> This is the place that caused me most trouble.  On a minor note:
> it worried me that security_inode_listsecurity() might return an
> error, whereas I think you know and assume that the worst it can
> return is 0 - might be worth a comment.

Will fix.

> But more major: I found it very odd that you collect one set of things
> from security_inode_listsecurity(), then proceed to tack on some more
> below from the shmem inode.  I looked at other filesystems (well, ext2!)
> and couldn't find a precedent.  What's this about?  Is it because other
> filesystems have an on-disk format which determines what they're capable
> of, whereas tmpfs is plastic and can reflect what the running system has?
> Or is it to allow for future xattrs which might be added to tmpfs, but
> frankly I'd rather do without until they're defined?

So there is a belief (based I think on an erroneous comment in the code
somewhere which I plan to find and fix) that the VFS provides some form
of generic xattr support if filesystems do not provide their own xattr
support.  This isn't true.  What actually happens is that the VFS will
directly call special LSM functions, if the filesystem provides no xattr
methods.  If the filesystem provides any xattr methods, which tmpfs does
when CONFIG_TMPFS_POSIX_ACL is set, then the VFS does different
stupi^wmagic things with regard to xattr calls.  I only really know the
SELinux LSM and will use it as an example, but I believe that SMACK does
very similar things.  When you run with SELinux enabled it will provide
support for ONLY security.selinux.  It provides this support by storing
the security.selinux xattr information inside the inode->i_security->sid
field.  I might be making this already weird interface even worse
because I'm trying to take advantage of the SELinux handling for
security.selinux rather than be required to store those in the
shmem_inode_info as well.  Other filesystems don't play this trick.

I'll try to rewrite the code to just be completely generic for
security.* and see if it is cleaner even if we waste some space storing
strings in ram we don't technically need to....

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Fight unfair telecom internet charges in Canada: sign http://stopthemeter.ca/
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>