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

[Bruno PrÃmont]

On Wed, 02 March 2011 Eric Paris <eparis@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> wrote:
> > Bueller? ÂBueller? ÂAny thoughts? ÂAny problems?
> >
> > On Tue, Jan 11, 2011 at 4:07 PM, Eric Paris <eparis@xxxxxxxxxx> 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@xxxxxxxxxx>
> >> ---
> >>
> >> Â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@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/