From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751446AbdGRMgG (ORCPT <rfc822;w@1wt.eu>);
        Tue, 18 Jul 2017 08:36:06 -0400
Received: from mx1.redhat.com ([209.132.183.28]:55398 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751348AbdGRMgE (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 18 Jul 2017 08:36:04 -0400
DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 958D461E51
Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=vgoyal@redhat.com
DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 958D461E51
Date: Tue, 18 Jul 2017 08:36:03 -0400
From: Vivek Goyal <vgoyal@redhat.com>
To: Stefan Berger <stefanb@linux.vnet.ibm.com>
Cc: Stefan Berger <StefanBergerstefanb@linux.vnet.ibm.com>,
        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
Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces
Message-ID: <20170718123603.GC8233@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>
 <20170717185811.GC15794@redhat.com>
 <7a39e8a6-a33b-f6a8-3fd5-6211c075ab91@linux.vnet.ibm.com>
 <20170718114849.GA8233@redhat.com>
 <55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com>
 <20170718123009.GB8233@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20170718123009.GB8233@redhat.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.39]); Tue, 18 Jul 2017 12:36:04 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote:
> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
> > On 07/18/2017 07:48 AM, Vivek Goyal wrote:
> > > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote:
> > > > On 07/17/2017 02:58 PM, Vivek Goyal wrote:
> > > > > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote:
> > > > > 
> > > > > [..]
> > > > > > +/*
> > > > > > + * xattr_list_userns_rewrite - Rewrite list of xattr names for user namespaces
> > > > > > + *                             or determine needed size for attribute list
> > > > > > + *                             in case size == 0
> > > > > > + *
> > > > > > + * In a user namespace we do not present all extended attributes to the
> > > > > > + * user. We filter out those that are in the list of userns supported xattr.
> > > > > > + * Besides that we filter out those with @uid=<uid> when there is no mapping
> > > > > > + * for that uid in the current user namespace.
> > > > > > + *
> > > > > > + * @list:        list of 0-byte separated xattr names
> > > > > > + * @size:        the size of the list; may be 0 to determine needed list size
> > > > > > + * @list_maxlen: allocated buffer size of list
> > > > > > + */
> > > > > > +static ssize_t
> > > > > > +xattr_list_userns_rewrite(char *list, ssize_t size, size_t list_maxlen)
> > > > > > +{
> > > > > > +	char *nlist = NULL;
> > > > > > +	size_t s_off, len, nlen;
> > > > > > +	ssize_t d_off;
> > > > > > +	char *name, *newname;
> > > > > > +
> > > > > > +	if (!list || size < 0 || current_user_ns() == &init_user_ns)
> > > > > size will never be less than 0 here. Only caller calls this function only
> > > > > if size is >0. So can we remove this?
> > > > Correct.
> > > > 
> > > > > What about case of "!list". So if user space called listxattr(foo, NULL,
> > > > > 0), we want to return the size of buffer as if all the xattrs will be
> > > > > returned to user space. But in practice we probably will filter out some
> > > > > xattrs so actually returned string will be smaller than size reported
> > > > > previously.
> > > > This case of size=0 is a problem in userns. Depending on the mapping of the
> > > > userid's the list can expand. A security.foo@uid=100 can become
> > > > security.foo@uid=100000, if the mapping is set up so that uid 100 on the
> > > > host becomes uid 100000 inside the container. So for now we only have
> > > > security.capability and the way I solved this is by allocating a 65k buffer
> > > > when calling from a userns. In this buffer where we gather the xattr names
> > > > and then walk them to determine the size that's needed for the buffer by
> > > > simulating the rewriting. It's not nice but I don't know of any other
> > > > solution.
> > > Hi Stefan,
> > > 
> > > For the case of size==0, why don't we iterate through all the xattr,
> > > filter them, remap them and then return the size to process in user
> > > namespace. That should fix this? I thought that's what
> > 
> > 
> > For the size==0 we need a temp. buffer where the raw xattr names are written
> > to so that the xattr_list_userns_rewrite() can actually rewrite what the
> > filesystem drivers returned.
> 
> I am probably missing something, but for the case of size==0, we don't
> have to copy all xattrs. We just need to determine size. So we can walk
> through each xattr, remap it and add to the size. I mean there should not
> be a need to allocate this 65K buffer. Just enough space needed to be
> able to store remapped xattr.
> 
> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
> buffer containing remapped xattr. So we should be able to just determine
> the size and free the buffer. And do it for all the xattrs returned by
> filesystem.
> 
> What am I missing?

Oh, I think I get it. If I don't pass a buffer to underlying driver, then
it will just return the size (and not actual list). So that's why you are
allocating that big buffer and getting the whole list internally, doing
mapping and returning size to user space. Hmm...

Vivek

From mboxrd@z Thu Jan  1 00:00:00 1970
From: vgoyal@redhat.com (Vivek Goyal)
Date: Tue, 18 Jul 2017 08:36:03 -0400
Subject: [PATCH v2] xattr: Enable security.capability in user namespaces
In-Reply-To: <20170718123009.GB8233@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>
	<20170717185811.GC15794@redhat.com>
	<7a39e8a6-a33b-f6a8-3fd5-6211c075ab91@linux.vnet.ibm.com>
	<20170718114849.GA8233@redhat.com>
	<55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com>
	<20170718123009.GB8233@redhat.com>
Message-ID: <20170718123603.GC8233@redhat.com>
To: linux-security-module@vger.kernel.org
List-Id: linux-security-module.vger.kernel.org

On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote:
> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
> > On 07/18/2017 07:48 AM, Vivek Goyal wrote:
> > > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote:
> > > > On 07/17/2017 02:58 PM, Vivek Goyal wrote:
> > > > > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote:
> > > > > 
> > > > > [..]
> > > > > > +/*
> > > > > > + * xattr_list_userns_rewrite - Rewrite list of xattr names for user namespaces
> > > > > > + *                             or determine needed size for attribute list
> > > > > > + *                             in case size == 0
> > > > > > + *
> > > > > > + * In a user namespace we do not present all extended attributes to the
> > > > > > + * user. We filter out those that are in the list of userns supported xattr.
> > > > > > + * Besides that we filter out those with @uid=<uid> when there is no mapping
> > > > > > + * for that uid in the current user namespace.
> > > > > > + *
> > > > > > + * @list:        list of 0-byte separated xattr names
> > > > > > + * @size:        the size of the list; may be 0 to determine needed list size
> > > > > > + * @list_maxlen: allocated buffer size of list
> > > > > > + */
> > > > > > +static ssize_t
> > > > > > +xattr_list_userns_rewrite(char *list, ssize_t size, size_t list_maxlen)
> > > > > > +{
> > > > > > +	char *nlist = NULL;
> > > > > > +	size_t s_off, len, nlen;
> > > > > > +	ssize_t d_off;
> > > > > > +	char *name, *newname;
> > > > > > +
> > > > > > +	if (!list || size < 0 || current_user_ns() == &init_user_ns)
> > > > > size will never be less than 0 here. Only caller calls this function only
> > > > > if size is >0. So can we remove this?
> > > > Correct.
> > > > 
> > > > > What about case of "!list". So if user space called listxattr(foo, NULL,
> > > > > 0), we want to return the size of buffer as if all the xattrs will be
> > > > > returned to user space. But in practice we probably will filter out some
> > > > > xattrs so actually returned string will be smaller than size reported
> > > > > previously.
> > > > This case of size=0 is a problem in userns. Depending on the mapping of the
> > > > userid's the list can expand. A security.foo at uid=100 can become
> > > > security.foo at uid=100000, if the mapping is set up so that uid 100 on the
> > > > host becomes uid 100000 inside the container. So for now we only have
> > > > security.capability and the way I solved this is by allocating a 65k buffer
> > > > when calling from a userns. In this buffer where we gather the xattr names
> > > > and then walk them to determine the size that's needed for the buffer by
> > > > simulating the rewriting. It's not nice but I don't know of any other
> > > > solution.
> > > Hi Stefan,
> > > 
> > > For the case of size==0, why don't we iterate through all the xattr,
> > > filter them, remap them and then return the size to process in user
> > > namespace. That should fix this? I thought that's what
> > 
> > 
> > For the size==0 we need a temp. buffer where the raw xattr names are written
> > to so that the xattr_list_userns_rewrite() can actually rewrite what the
> > filesystem drivers returned.
> 
> I am probably missing something, but for the case of size==0, we don't
> have to copy all xattrs. We just need to determine size. So we can walk
> through each xattr, remap it and add to the size. I mean there should not
> be a need to allocate this 65K buffer. Just enough space needed to be
> able to store remapped xattr.
> 
> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
> buffer containing remapped xattr. So we should be able to just determine
> the size and free the buffer. And do it for all the xattrs returned by
> filesystem.
> 
> What am I missing?

Oh, I think I get it. If I don't pass a buffer to underlying driver, then
it will just return the size (and not actual list). So that's why you are
allocating that big buffer and getting the whole list internally, doing
mapping and returning size to user space. Hmm...

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="===============2427517170885270985=="
MIME-Version: 1.0
From: Vivek Goyal <vgoyal@redhat.com>
To: lkp@lists.01.org
Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces
Date: Tue, 18 Jul 2017 08:36:03 -0400
Message-ID: <20170718123603.GC8233@redhat.com>
In-Reply-To: <20170718123009.GB8233@redhat.com>
List-Id: <oe-lkp.lists.linux.dev>

--===============2427517170885270985==
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable

On Tue, Jul 18, 2017 at 08:30:09AM -0400, Vivek Goyal wrote:
> On Tue, Jul 18, 2017 at 08:05:18AM -0400, Stefan Berger wrote:
> > On 07/18/2017 07:48 AM, Vivek Goyal wrote:
> > > On Mon, Jul 17, 2017 at 04:50:22PM -0400, Stefan Berger wrote:
> > > > On 07/17/2017 02:58 PM, Vivek Goyal wrote:
> > > > > On Tue, Jul 11, 2017 at 11:05:11AM -0400, Stefan Berger wrote:
> > > > > =

> > > > > [..]
> > > > > > +/*
> > > > > > + * xattr_list_userns_rewrite - Rewrite list of xattr names for=
 user namespaces
> > > > > > + *                             or determine needed size for at=
tribute list
> > > > > > + *                             in case size =3D=3D 0
> > > > > > + *
> > > > > > + * In a user namespace we do not present all extended attribut=
es to the
> > > > > > + * user. We filter out those that are in the list of userns su=
pported xattr.
> > > > > > + * Besides that we filter out those with @uid=3D<uid> when the=
re is no mapping
> > > > > > + * for that uid in the current user namespace.
> > > > > > + *
> > > > > > + * @list:        list of 0-byte separated xattr names
> > > > > > + * @size:        the size of the list; may be 0 to determine n=
eeded list size
> > > > > > + * @list_maxlen: allocated buffer size of list
> > > > > > + */
> > > > > > +static ssize_t
> > > > > > +xattr_list_userns_rewrite(char *list, ssize_t size, size_t lis=
t_maxlen)
> > > > > > +{
> > > > > > +	char *nlist =3D NULL;
> > > > > > +	size_t s_off, len, nlen;
> > > > > > +	ssize_t d_off;
> > > > > > +	char *name, *newname;
> > > > > > +
> > > > > > +	if (!list || size < 0 || current_user_ns() =3D=3D &init_user_=
ns)
> > > > > size will never be less than 0 here. Only caller calls this funct=
ion only
> > > > > if size is >0. So can we remove this?
> > > > Correct.
> > > > =

> > > > > What about case of "!list". So if user space called listxattr(foo=
, NULL,
> > > > > 0), we want to return the size of buffer as if all the xattrs wil=
l be
> > > > > returned to user space. But in practice we probably will filter o=
ut some
> > > > > xattrs so actually returned string will be smaller than size repo=
rted
> > > > > previously.
> > > > This case of size=3D0 is a problem in userns. Depending on the mapp=
ing of the
> > > > userid's the list can expand. A security.foo(a)uid=3D100 can become
> > > > security.foo(a)uid=3D100000, if the mapping is set up so that uid 1=
00 on the
> > > > host becomes uid 100000 inside the container. So for now we only ha=
ve
> > > > security.capability and the way I solved this is by allocating a 65=
k buffer
> > > > when calling from a userns. In this buffer where we gather the xatt=
r names
> > > > and then walk them to determine the size that's needed for the buff=
er by
> > > > simulating the rewriting. It's not nice but I don't know of any oth=
er
> > > > solution.
> > > Hi Stefan,
> > > =

> > > For the case of size=3D=3D0, why don't we iterate through all the xat=
tr,
> > > filter them, remap them and then return the size to process in user
> > > namespace. That should fix this? I thought that's what
> > =

> > =

> > For the size=3D=3D0 we need a temp. buffer where the raw xattr names ar=
e written
> > to so that the xattr_list_userns_rewrite() can actually rewrite what the
> > filesystem drivers returned.
> =

> I am probably missing something, but for the case of size=3D=3D0, we don't
> have to copy all xattrs. We just need to determine size. So we can walk
> through each xattr, remap it and add to the size. I mean there should not
> be a need to allocate this 65K buffer. Just enough space needed to be
> able to store remapped xattr.
> =

> You are already doing it in xattr_parse_uid_from_kuid(). It returns the
> buffer containing remapped xattr. So we should be able to just determine
> the size and free the buffer. And do it for all the xattrs returned by
> filesystem.
> =

> What am I missing?

Oh, I think I get it. If I don't pass a buffer to underlying driver, then
it will just return the size (and not actual list). So that's why you are
allocating that big buffer and getting the whole list internally, doing
mapping and returning size to user space. Hmm...

Vivek

--===============2427517170885270985==--