Messages in this thread | | | Subject | Re: [PATCH v2] xattr: Enable security.capability in user namespaces | From | Stefan Berger <> | Date | Wed, 12 Jul 2017 15:19:46 -0400 |
| |
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 >
| |