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 S1756166AbdGLDpG (ORCPT <rfc822;w@1wt.eu>);
        Tue, 11 Jul 2017 23:45:06 -0400
Received: from h2.hallyn.com ([78.46.35.8]:59166 "EHLO h2.hallyn.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751270AbdGLDpF (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Tue, 11 Jul 2017 23:45:05 -0400
Date: Tue, 11 Jul 2017 22:45:03 -0500
From: "Serge E. Hallyn" <serge@hallyn.com>
To: Stefan Berger <stefanb@linux.vnet.ibm.com>
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, vgoyal@redhat.com,
        christian.brauner@mailbox.org, amir73il@gmail.com,
        linux-security-module@vger.kernel.org, casey@schaufler-ca.com,
        Stefan Berger <stefanb@linux.vnet.ibm.com>
Subject: Re: [PATCH v2] xattr: Enable security.capability in user namespaces
Message-ID: <20170712034503.GA8270@mail.hallyn.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.5.21 (2010-09-15)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Quoting Stefan Berger (Stefan Bergerstefanb@linux.vnet.ibm.com):
> +/*
> + * 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)
> +		return size;
> +
> +	if (size) {
> +		nlist = kmalloc(list_maxlen, GFP_KERNEL);
> +		if (!nlist)
> +			return -ENOMEM;
> +	}
> +
> +	s_off = d_off = 0;
> +	while (s_off < size || size == 0) {
> +		name = &list[s_off];
> +
> +		len = strlen(name);
> +		if (!len)
> +			break;
> +
> +		if (xattr_is_userns_supported(name, false) >= 0)
> +			newname = name;
> +		else {
> +			newname = xattr_rewrite_userns_xattr(name);

Why are you doing this here?  If we get here it means that
xattr_is_userns_supported() returned < 0, meaning name is
not userns-supported.  So xattr_rewrite_userns_xattr() will
just return name.  Am I missing something?

> +			if (IS_ERR(newname)) {
> +				d_off = PTR_ERR(newname);
> +				goto out_free;
> +			}
> +		}
> +		if (newname && !xattr_list_contains(nlist, d_off, newname)) {

Now here, if name was recalculated to @newname, and @newname is
found in the nlist, that should raise an error right?  Something
fishy is going on?

> +			nlen = strlen(newname);
> +
> +			if (nlist) {
> +				if (nlen + 1 > list_maxlen)

d_off needs to be set to -ERANGE here.

> +					break;
> +				strcpy(&nlist[d_off], newname);
> +			}
> +
> +			d_off += nlen + 1;
> +			if (newname != name)
> +				kfree(newname);
> +		}
> +		s_off += len + 1;
> +	}
> +	if (nlist)
> +		memcpy(list, nlist, d_off);
> +out_free:
> +	kfree(nlist);
> +
> +	return d_off;
> +}

From mboxrd@z Thu Jan  1 00:00:00 1970
From: serge@hallyn.com (Serge E. Hallyn)
Date: Tue, 11 Jul 2017 22:45:03 -0500
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: <20170712034503.GA8270@mail.hallyn.com>
To: linux-security-module@vger.kernel.org
List-Id: linux-security-module.vger.kernel.org

Quoting Stefan Berger (Stefan Bergerstefanb at linux.vnet.ibm.com):
> +/*
> + * 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)
> +		return size;
> +
> +	if (size) {
> +		nlist = kmalloc(list_maxlen, GFP_KERNEL);
> +		if (!nlist)
> +			return -ENOMEM;
> +	}
> +
> +	s_off = d_off = 0;
> +	while (s_off < size || size == 0) {
> +		name = &list[s_off];
> +
> +		len = strlen(name);
> +		if (!len)
> +			break;
> +
> +		if (xattr_is_userns_supported(name, false) >= 0)
> +			newname = name;
> +		else {
> +			newname = xattr_rewrite_userns_xattr(name);

Why are you doing this here?  If we get here it means that
xattr_is_userns_supported() returned < 0, meaning name is
not userns-supported.  So xattr_rewrite_userns_xattr() will
just return name.  Am I missing something?

> +			if (IS_ERR(newname)) {
> +				d_off = PTR_ERR(newname);
> +				goto out_free;
> +			}
> +		}
> +		if (newname && !xattr_list_contains(nlist, d_off, newname)) {

Now here, if name was recalculated to @newname, and @newname is
found in the nlist, that should raise an error right?  Something
fishy is going on?

> +			nlen = strlen(newname);
> +
> +			if (nlist) {
> +				if (nlen + 1 > list_maxlen)

d_off needs to be set to -ERANGE here.

> +					break;
> +				strcpy(&nlist[d_off], newname);
> +			}
> +
> +			d_off += nlen + 1;
> +			if (newname != name)
> +				kfree(newname);
> +		}
> +		s_off += len + 1;
> +	}
> +	if (nlist)
> +		memcpy(list, nlist, d_off);
> +out_free:
> +	kfree(nlist);
> +
> +	return d_off;
> +}
--
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