From mboxrd@z Thu Jan 1 00:00:00 1970 From: Vivek Goyal Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces Date: Wed, 12 Jul 2017 13:53:57 -0400 Message-ID: <20170712175357.GA32609@redhat.com> References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <1499785511-17192-2-git-send-email-stefanb-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: containers-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Stefan Berger Cc: lkp-JC7UmRfGjtg@public.gmane.org, containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org, linux-security-module-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, ebiederm-aS9lmoZGLiVWk0Htik3J/w@public.gmane.org, casey-iSGtlc1asvQWG2LlvL+J4A@public.gmane.org, zohar-23VcF4HTsmIX0ybBhKVfKdBPR1lH4CV8@public.gmane.org List-Id: containers.vger.kernel.org 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. > + 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. 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. 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. 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: vgoyal@redhat.com (Vivek Goyal) Date: Wed, 12 Jul 2017 13:53:57 -0400 Subject: [PATCH v2] xattr: Enable security.capability in user namespaces In-Reply-To: <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> Message-ID: <20170712175357.GA32609@redhat.com> To: linux-security-module@vger.kernel.org List-Id: linux-security-module.vger.kernel.org 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. > + 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. 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. 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 at uid=1000. 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 -- To unsubscribe from this list: send the line "unsubscribe linux-security-module" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============7188031180612052394==" MIME-Version: 1.0 From: Vivek Goyal To: lkp@lists.01.org Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces Date: Wed, 12 Jul 2017 13:53:57 -0400 Message-ID: <20170712175357.GA32609@redhat.com> In-Reply-To: <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> List-Id: --===============7188031180612052394== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable 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 *n= ame, > void *value, size_t size) > { > - const struct xattr_handler *handler; > + const struct xattr_handler *handler =3D NULL; > + char *newname =3D NULL; > + int ret, userns_supt_xattr; > + struct user_namespace *userns =3D current_user_ns(); > + > + userns_supt_xattr =3D (xattr_is_userns_supported(name, false) >=3D 0); > + Hi Stephan, > + do { > + kfree(newname); > + > + newname =3D xattr_userns_name(name, userns); ^^^ Will name be pointing to a freed string in second iteration of loop. = > + if (IS_ERR(newname)) > + return PTR_ERR(newname); > + > + if (!handler) { > + name =3D 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. 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. 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(a)uid=3D1000. = Vivek > + handler =3D xattr_resolve_name(inode, &name); > + if (IS_ERR(handler)) { > + ret =3D PTR_ERR(handler); > + goto out; > + } > + if (!handler->get) { > + ret =3D -EOPNOTSUPP; > + goto out; > + } > + } > + ret =3D handler->get(handler, dentry, inode, name, value, size); > + userns =3D userns->parent; > + } while ((ret =3D=3D -ENODATA) && userns && userns_supt_xattr); > = > - handler =3D 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 --===============7188031180612052394==-- From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752301AbdGLRyA (ORCPT ); Wed, 12 Jul 2017 13:54:00 -0400 Received: from mx1.redhat.com ([209.132.183.28]:48370 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750987AbdGLRx7 (ORCPT ); Wed, 12 Jul 2017 13:53:59 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 4D3324ACC7 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=vgoyal@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 4D3324ACC7 Date: Wed, 12 Jul 2017 13:53:57 -0400 From: Vivek Goyal To: Stefan Berger Cc: ebiederm@xmission.com, containers@lists.linux-foundation.org, lkp@01.org, linux-kernel@vger.kernel.org, zohar@linux.vnet.ibm.com, tycho@docker.com, serge@hallyn.com, James.Bottomley@HansenPartnership.com, christian.brauner@mailbox.org, amir73il@gmail.com, linux-security-module@vger.kernel.org, casey@schaufler-ca.com, Stefan Berger Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces Message-ID: <20170712175357.GA32609@redhat.com> References: <1499785511-17192-1-git-send-email-stefanb@linux.vnet.ibm.com> <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1499785511-17192-2-git-send-email-stefanb@linux.vnet.ibm.com> User-Agent: Mutt/1.7.1 (2016-10-04) X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Wed, 12 Jul 2017 17:53:58 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > + 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. 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. 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. 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