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 S1751500AbdGRMF2 (ORCPT <rfc822;w@1wt.eu>);
        Tue, 18 Jul 2017 08:05:28 -0400
Received: from mx0a-001b2d01.pphosted.com ([148.163.156.1]:54081 "EHLO
        mx0a-001b2d01.pphosted.com" rhost-flags-OK-OK-OK-OK)
        by vger.kernel.org with ESMTP id S1751365AbdGRMF0 (ORCPT
        <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 18 Jul 2017 08:05:26 -0400
Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces
To: Vivek Goyal <vgoyal@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>
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
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
Date: Tue, 18 Jul 2017 08:05:18 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101
 Thunderbird/45.4.0
MIME-Version: 1.0
In-Reply-To: <20170718114849.GA8233@redhat.com>
Content-Type: text/plain; charset=windows-1252; format=flowed
Content-Transfer-Encoding: 7bit
X-TM-AS-GCONF: 00
x-cbid: 17071812-0044-0000-0000-0000036FC3D1
X-IBM-SpamModules-Scores: 
X-IBM-SpamModules-Versions: BY=3.00007381; HX=3.00000241; KW=3.00000007;
 PH=3.00000004; SC=3.00000214; SDB=6.00889314; UDB=6.00444204; IPR=6.00669486;
 BA=6.00005478; NDR=6.00000001; ZLA=6.00000005; ZF=6.00000009; ZB=6.00000000;
 ZP=6.00000000; ZH=6.00000000; ZU=6.00000002; MB=3.00016260; XFM=3.00000015;
 UTC=2017-07-18 12:05:22
X-IBM-AV-DETECTION: SAVI=unused REMOTE=unused XFE=unused
x-cbparentid: 17071812-0045-0000-0000-0000079DC977
Message-Id: <55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com>
X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-07-18_04:,,
 signatures=0
X-Proofpoint-Spam-Details: rule=outbound_notspam policy=outbound score=0 spamscore=0 suspectscore=2
 malwarescore=0 phishscore=0 adultscore=0 bulkscore=0 classifier=spam
 adjust=0 reason=mlx scancount=1 engine=8.0.1-1706020000
 definitions=main-1707180188
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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. Not knowing exactly how big that 
buffer should be, I allocate 65k for it. From what I read there is a 64k 
limit on the vfs layer for xattrs, probably including xattr values. So 
65k would for sure be enough also if each one of the xattr names becomes 
bigger.

@@ -922,10 +947,20 @@ vfs_listxattr(struct dentry *dentry, char *list, 
size_t size, bool rewrite)
  {
      struct inode *inode = d_inode(dentry);
      ssize_t error;
+    bool getsize = false;

      error = security_inode_listxattr(dentry);
      if (error)
          return error;
+
+    if (!size) {
+        if (current_user_ns() != &init_user_ns) {
+            size = 65 * 1024;
+            list = kmalloc(size, GFP_KERNEL);
+        }
+        getsize = true;
+    }
+
      if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) {
          error = -EOPNOTSUPP;
          error = inode->i_op->listxattr(dentry, list, size);
@@ -937,6 +972,9 @@ vfs_listxattr(struct dentry *dentry, char *list, 
size_t size, bool rewrite)
      if (error > 0 && rewrite)
          error = xattr_list_userns_rewrite(list, error, size);

+    if (getsize)
+        kfree(list);
+
      return error;
  }
  EXPORT_SYMBOL_GPL(vfs_listxattr);


     Stefan

> xattr_list_userns_rewrite() was doing. But looks like this logic will not
> kick in for the case of size==0 due to "!list" condition.
>
> Also we could probably replace "!list" with "!size" wheverever required.
> Its little easy to read and understand.
>
> For the other case where some xattrs can get filtered out and we report
> a buffer size bigger than actually needed, I am hoping that its acceptable
> and none of the existing users are broken.
>
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-module" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

From mboxrd@z Thu Jan  1 00:00:00 1970
From: stefanb@linux.vnet.ibm.com (Stefan Berger)
Date: Tue, 18 Jul 2017 08:05:18 -0400
Subject: [PATCH v2] xattr: Enable security.capability in user namespaces
In-Reply-To: <20170718114849.GA8233@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>
Message-ID: <55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com>
To: linux-security-module@vger.kernel.org
List-Id: linux-security-module.vger.kernel.org

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. Not knowing exactly how big that 
buffer should be, I allocate 65k for it. From what I read there is a 64k 
limit on the vfs layer for xattrs, probably including xattr values. So 
65k would for sure be enough also if each one of the xattr names becomes 
bigger.

@@ -922,10 +947,20 @@ vfs_listxattr(struct dentry *dentry, char *list, 
size_t size, bool rewrite)
  {
      struct inode *inode = d_inode(dentry);
      ssize_t error;
+    bool getsize = false;

      error = security_inode_listxattr(dentry);
      if (error)
          return error;
+
+    if (!size) {
+        if (current_user_ns() != &init_user_ns) {
+            size = 65 * 1024;
+            list = kmalloc(size, GFP_KERNEL);
+        }
+        getsize = true;
+    }
+
      if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) {
          error = -EOPNOTSUPP;
          error = inode->i_op->listxattr(dentry, list, size);
@@ -937,6 +972,9 @@ vfs_listxattr(struct dentry *dentry, char *list, 
size_t size, bool rewrite)
      if (error > 0 && rewrite)
          error = xattr_list_userns_rewrite(list, error, size);

+    if (getsize)
+        kfree(list);
+
      return error;
  }
  EXPORT_SYMBOL_GPL(vfs_listxattr);


     Stefan

> xattr_list_userns_rewrite() was doing. But looks like this logic will not
> kick in for the case of size==0 due to "!list" condition.
>
> Also we could probably replace "!list" with "!size" wheverever required.
> Its little easy to read and understand.
>
> For the other case where some xattrs can get filtered out and we report
> a buffer size bigger than actually needed, I am hoping that its acceptable
> and none of the existing users are broken.
>
> 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
>

--
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="===============6510618024021933752=="
MIME-Version: 1.0
From: Stefan Berger <stefanb@linux.vnet.ibm.com>
To: lkp@lists.01.org
Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces
Date: Tue, 18 Jul 2017 08:05:18 -0400
Message-ID: <55971eea-fde2-439a-2fe5-d0ae5e80bc22@linux.vnet.ibm.com>
In-Reply-To: <20170718114849.GA8233@redhat.com>
List-Id: <oe-lkp.lists.linux.dev>

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

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 n=
amespaces
>>>> + *                             or determine needed size for attribute=
 list
>>>> + *                             in case size =3D=3D 0
>>>> + *
>>>> + * In a user namespace we do not present all extended attributes to t=
he
>>>> + * user. We filter out those that are in the list of userns supported=
 xattr.
>>>> + * Besides that we filter out those with @uid=3D<uid> when there is n=
o 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 l=
ist size
>>>> + * @list_maxlen: allocated buffer size of list
>>>> + */
>>>> +static ssize_t
>>>> +xattr_list_userns_rewrite(char *list, ssize_t size, size_t list_maxle=
n)
>>>> +{
>>>> +	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 function on=
ly
>>> 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=3D0 is a problem in userns. Depending on the mapping o=
f 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 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 buf=
fer
>> when calling from a userns. In this buffer where we gather the xattr nam=
es
>> 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=3D=3D0, 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=3D=3D0 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. Not knowing exactly how big that =

buffer should be, I allocate 65k for it. From what I read there is a 64k =

limit on the vfs layer for xattrs, probably including xattr values. So =

65k would for sure be enough also if each one of the xattr names becomes =

bigger.

@@ -922,10 +947,20 @@ vfs_listxattr(struct dentry *dentry, char *list, =

size_t size, bool rewrite)
  {
      struct inode *inode =3D d_inode(dentry);
      ssize_t error;
+    bool getsize =3D false;

      error =3D security_inode_listxattr(dentry);
      if (error)
          return error;
+
+    if (!size) {
+        if (current_user_ns() !=3D &init_user_ns) {
+            size =3D 65 * 1024;
+            list =3D kmalloc(size, GFP_KERNEL);
+        }
+        getsize =3D true;
+    }
+
      if (inode->i_op->listxattr && (inode->i_opflags & IOP_XATTR)) {
          error =3D -EOPNOTSUPP;
          error =3D inode->i_op->listxattr(dentry, list, size);
@@ -937,6 +972,9 @@ vfs_listxattr(struct dentry *dentry, char *list, =

size_t size, bool rewrite)
      if (error > 0 && rewrite)
          error =3D xattr_list_userns_rewrite(list, error, size);

+    if (getsize)
+        kfree(list);
+
      return error;
  }
  EXPORT_SYMBOL_GPL(vfs_listxattr);


     Stefan

> xattr_list_userns_rewrite() was doing. But looks like this logic will not
> kick in for the case of size=3D=3D0 due to "!list" condition.
>
> Also we could probably replace "!list" with "!size" wheverever required.
> Its little easy to read and understand.
>
> For the other case where some xattrs can get filtered out and we report
> a buffer size bigger than actually needed, I am hoping that its acceptable
> and none of the existing users are broken.
>
> Thanks
> Vivek
> --
> To unsubscribe from this list: send the line "unsubscribe linux-security-=
module" in
> the body of a message to majordomo(a)vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>


--===============6510618024021933752==--