lkml.org 
[lkml]   [2017]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [PATCH v2] xattr: Enable security.capability in user namespaces
From
Date
On 07/12/2017 01:53 PM, Vivek Goyal wrote:
> On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote:
>
> [..]
>> @@ -301,14 +721,39 @@ ssize_t
>> __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
>> void *value, size_t size)
>> {
>> - const struct xattr_handler *handler;
>> + const struct xattr_handler *handler = NULL;
>> + char *newname = NULL;
>> + int ret, userns_supt_xattr;
>> + struct user_namespace *userns = current_user_ns();
>> +
>> + userns_supt_xattr = (xattr_is_userns_supported(name, false) >= 0);
>> +
> Hi Stephan,
>
>> + do {
>> + kfree(newname);
>> +
>> + newname = xattr_userns_name(name, userns);
> ^^^
> Will name be pointing to a freed string in second iteration of loop.

Fixing for v3.

>
>> + if (IS_ERR(newname))
>> + return PTR_ERR(newname);
>> +
>> + if (!handler) {
>> + name = newname;
> Here we assign name and at the beginning of second iteration we free
> newname.
>
> Also I am not sure why do we do this assignment only if handler is NULL.

The handler shouldn't change but this optimization isn't helpful. Fixed
through this patch:

https://github.com/stefanberger/linux/commit/10828401b29a13f8c56f8fad0c0fb2690e4af878


>
> BTW, I set cap_sys_admin on a file outside usernamespace and then launched
> user namespace (mapping 1000 to 0). And then tried to do getcap on file
> and I am not seeing security.capability set by host. Not sure what am I
> doing wrong. getxattr() seems to return -ENODATA. Still debugging it.

This was a regression due to the bug in the loop. I didn't have a test
case (with runc) for it, now I do.

>
> Also, have we resovled the question of stacked filesystem like overlayfs.
> There we are switching creds to mounter's creds when doing operations on
> underlying filesystem. I am concenrned does that mean, we will get and
> return security.capability to caller in usernamespace instead of
> security.capability@uid=1000.

I would have to test this, otherwise I don't know. I'll try it out with
Docker.

Stefan

>
> Vivek
>
>> + handler = xattr_resolve_name(inode, &name);
>> + if (IS_ERR(handler)) {
>> + ret = PTR_ERR(handler);
>> + goto out;
>> + }
>> + if (!handler->get) {
>> + ret = -EOPNOTSUPP;
>> + goto out;
>> + }
>> + }
>> + ret = handler->get(handler, dentry, inode, name, value, size);
>> + userns = userns->parent;
>> + } while ((ret == -ENODATA) && userns && userns_supt_xattr);
>>
>> - handler = xattr_resolve_name(inode, &name);
>> - if (IS_ERR(handler))
>> - return PTR_ERR(handler);
>> - if (!handler->get)
>> - return -EOPNOTSUPP;
>> - return handler->get(handler, dentry, inode, name, value, size);
>> +out:
>> + kfree(newname);
>> + return ret;
>> }
>> EXPORT_SYMBOL(__vfs_getxattr);
>>
> Thanks
> Vivek
>

\
 
 \ /
  Last update: 2017-07-12 21:20    [W:0.223 / U:0.600 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site