Linux-mm Archive on lore.kernel.org
 help / color / Atom feed
From: Mark Salyzyn <salyzyn@android.com>
To: Jan Kara <jack@suse.cz>
Cc: linux-kernel@vger.kernel.org, kernel-team@android.com,
	"Tyler Hicks" <tyhicks@canonical.com>,
	"Dominique Martinet" <asmadeus@codewreck.org>,
	"David S. Miller" <davem@davemloft.net>,
	"Mathieu Malaterre" <malat@debian.org>,
	"Andreas Dilger" <adilger.kernel@dilger.ca>,
	devel@driverdev.osuosl.org,
	"Vyacheslav Dubeyko" <slava@dubeyko.com>,
	"Joel Becker" <jlbec@evilplan.org>,
	"Mark Fasheh" <mark@fasheh.com>, "Chris Mason" <clm@fb.com>,
	"Artem Bityutskiy" <dedekind1@gmail.com>,
	"Eric Van Hensbergen" <ericvh@gmail.com>,
	"Ernesto A. Fernández" <ernesto.mnd.fernandez@gmail.com>,
	"Ilya Dryomov" <idryomov@gmail.com>,
	"Hugh Dickins" <hughd@google.com>,
	"Serge Hallyn" <serge@hallyn.com>,
	"Trond Myklebust" <trond.myklebust@hammerspace.com>,
	"Gao Xiang" <gaoxiang25@huawei.com>,
	"Chao Yu" <yuchao0@huawei.com>,
	"David Woodhouse" <dwmw2@infradead.org>,
	"Adrian Hunter" <adrian.hunter@intel.com>,
	"Latchesar Ionkov" <lucho@ionkov.net>,
	"Jaegeuk Kim" <jaegeuk@kernel.org>,
	"Jeff Layton" <jlayton@kernel.org>,
	"Dave Kleikamp" <shaggy@kernel.org>, "Tejun Heo" <tj@kernel.org>,
	linux-mm@kvack.org, "Andrew Morton" <akpm@linux-foundation.org>,
	"Joseph Qi" <joseph.qi@linux.alibaba.com>,
	"Mimi Zohar" <zohar@linux.ibm.com>,
	"Greg Kroah-Hartman" <gregkh@linuxfoundation.org>,
	linux-afs@lists.infradead.org, linux-mtd@lists.infradead.org,
	devel@lists.orangefs.org, linux-erofs@lists.ozlabs.org,
	samba-technical@lists.samba.org,
	jfs-discussion@lists.sourceforge.net,
	linux-f2fs-devel@lists.sourceforge.net,
	v9fs-developer@lists.sourceforge.net,
	"Theodore Ts\'o" <tytso@mit.edu>,
	"James Morris" <jmorris@namei.org>,
	"Anna Schumaker" <anna.schumaker@netapp.com>,
	"Richard Weinberger" <richard@nod.at>,
	"Mike Marshall" <hubcap@omnibond.com>,
	"Martin Brandenburg" <martin@omnibond.com>,
	"Darrick J. Wong" <darrick.wong@oracle.com>,
	ocfs2-devel@oss.oracle.com, "Eric Paris" <eparis@parisplace.org>,
	"Paul Moore" <paul@paul-moore.com>,
	"Andreas Gruenbacher" <agruenba@redhat.com>,
	cluster-devel@redhat.com, "David Howells" <dhowells@redhat.com>,
	"Bob Peterson" <rpeterso@redhat.com>,
	"Sage Weil" <sage@redhat.com>, "Steve French" <sfrench@samba.org>,
	"Casey Schaufler" <casey@schaufler-ca.com>,
	"Phillip Lougher" <phillip@squashfs.org.uk>,
	"David Sterba" <dsterba@suse.com>, "Jan Kara" <jack@suse.com>,
	"Miklos Szeredi" <miklos@szeredi.hu>,
	"Josef Bacik" <josef@toxicpanda.com>,
	"Stephen Smalley" <sds@tycho.nsa.gov>,
	ceph-devel@vger.kernel.org, ecryptfs@vger.kernel.org,
	linux-btrfs@vger.kernel.org, linux-cifs@vger.kernel.org,
	linux-ext4@vger.kernel.org, linux-fsdevel@vger.kernel.org,
	linux-integrity@vger.kernel.org, linux-nfs@vger.kernel.org,
	linux-security-module@vger.kernel.org,
	linux-unionfs@vger.kernel.org, linux-xfs@vger.kernel.org,
	netdev@vger.kernel.org, reiserfs-devel@vger.kernel.org,
	selinux@vger.kernel.org, stable@vger.kernel.org,
	"Alexander Viro" <viro@zeniv.linux.org.uk>
Subject: Re: [PATCH v2] Add flags option to get xattr method paired to __vfs_getxattr
Date: Wed, 14 Aug 2019 07:54:16 -0700
Message-ID: <71d66fd1-cc94-fd0c-dfa7-115ba8a6b95a@android.com> (raw)
In-Reply-To: <20190814110022.GB26273@quack2.suse.cz>

On 8/14/19 4:00 AM, Jan Kara wrote:
> On Tue 13-08-19 07:55:06, Mark Salyzyn wrote:
> ...
>> diff --git a/fs/xattr.c b/fs/xattr.c
>> index 90dd78f0eb27..71f887518d6f 100644
>> --- a/fs/xattr.c
>> +++ b/fs/xattr.c
> ...
>>   ssize_t
>>   __vfs_getxattr(struct dentry *dentry, struct inode *inode, const char *name,
>> -	       void *value, size_t size)
>> +	       void *value, size_t size, int flags)
>>   {
>>   	const struct xattr_handler *handler;
>> -
>> -	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);
>> -}
>> -EXPORT_SYMBOL(__vfs_getxattr);
>> -
>> -ssize_t
>> -vfs_getxattr(struct dentry *dentry, const char *name, void *value, size_t size)
>> -{
>> -	struct inode *inode = dentry->d_inode;
>>   	int error;
>>   
>> +	if (flags & XATTR_NOSECURITY)
>> +		goto nolsm;
> Hum, is it OK for XATTR_NOSECURITY to skip even the xattr_permission()
> check? I understand that for reads of security xattrs it actually does not
> matter in practice but conceptually that seems wrong to me as
> XATTR_NOSECURITY is supposed to skip just security-module checks to avoid
> recursion AFAIU.

Good catch I think.

I was attempting to make this change purely inert, no change in 
functionality, only a change in API. Adding a call to xattr_permission 
would incur a change in overall functionality, as it would introduce 
into the current and original __vfs_getxattr a call to xattr_permission 
that was not there before.

(I will have to defer the real answer and requirements to the security 
folks)

AFAIK you are correct, and to make the call would reduce the attack 
surface, trading a very small amount of CPU utilization, for a much 
larger amount of trust.

Given the long history of this patch set (for overlayfs) and the large 
amount of stakeholders, I would _prefer_ to submit a followup 
independent functionality/security change to _vfs_get_xattr _after_ this 
makes it in.

>
>> diff --git a/include/uapi/linux/xattr.h b/include/uapi/linux/xattr.h
>> index c1395b5bd432..1216d777d210 100644
>> --- a/include/uapi/linux/xattr.h
>> +++ b/include/uapi/linux/xattr.h
>> @@ -17,8 +17,9 @@
>>   #if __UAPI_DEF_XATTR
>>   #define __USE_KERNEL_XATTR_DEFS
>>   
>> -#define XATTR_CREATE	0x1	/* set value, fail if attr already exists */
>> -#define XATTR_REPLACE	0x2	/* set value, fail if attr does not exist */
>> +#define XATTR_CREATE	 0x1	/* set value, fail if attr already exists */
>> +#define XATTR_REPLACE	 0x2	/* set value, fail if attr does not exist */
>> +#define XATTR_NOSECURITY 0x4	/* get value, do not involve security check */
>>   #endif
> It seems confusing to export XATTR_NOSECURITY definition to userspace when
> that is kernel-internal flag. I'd just define it in include/linux/xattr.h
> somewhere from the top of flags space (like 0x40000000).
>
> Otherwise the patch looks OK to me (cannot really comment on the security
> module aspect of this whole thing though).

Good point. However, we do need to keep these flags together to reduce 
maintenance risk, I personally abhor two locations for flags bits even 
if one comes from the opposite bit-side; collisions are undetectable at 
build time. Although I have not gone through the entire thought 
experiment, I am expecting that fuse could possibly benefit from this 
flag (if exposed) since it also has a security recursion. That said, 
fuse is probably the example of a gaping wide attack surface if user 
space had access to it ... your xattr_permissions call addition 
requested above would be realistically, not just pedantically, required!

I have to respin the patch because of yet another hole in filesystem 
coverage (I blew the mechanical ubifs adjustment, adjusted the wrong 
function), so please do tell if my rationalizations above hit a note, or 
if I _need_ to make the changes in this patch (change it to a series?).

Thanks -- Mark Salyzyn




  reply index

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-13 14:55 Mark Salyzyn
2019-08-14  5:57 ` kbuild test robot
2019-08-14 11:00 ` Jan Kara
2019-08-14 14:54   ` Mark Salyzyn [this message]
2019-08-15 15:30     ` Jan Kara
2019-08-14 11:59 ` kbuild test robot

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=71d66fd1-cc94-fd0c-dfa7-115ba8a6b95a@android.com \
    --to=salyzyn@android.com \
    --cc=adilger.kernel@dilger.ca \
    --cc=adrian.hunter@intel.com \
    --cc=agruenba@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=anna.schumaker@netapp.com \
    --cc=asmadeus@codewreck.org \
    --cc=casey@schaufler-ca.com \
    --cc=ceph-devel@vger.kernel.org \
    --cc=clm@fb.com \
    --cc=cluster-devel@redhat.com \
    --cc=darrick.wong@oracle.com \
    --cc=davem@davemloft.net \
    --cc=dedekind1@gmail.com \
    --cc=devel@driverdev.osuosl.org \
    --cc=devel@lists.orangefs.org \
    --cc=dhowells@redhat.com \
    --cc=dsterba@suse.com \
    --cc=dwmw2@infradead.org \
    --cc=ecryptfs@vger.kernel.org \
    --cc=eparis@parisplace.org \
    --cc=ericvh@gmail.com \
    --cc=ernesto.mnd.fernandez@gmail.com \
    --cc=gaoxiang25@huawei.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hubcap@omnibond.com \
    --cc=hughd@google.com \
    --cc=idryomov@gmail.com \
    --cc=jack@suse.com \
    --cc=jack@suse.cz \
    --cc=jaegeuk@kernel.org \
    --cc=jfs-discussion@lists.sourceforge.net \
    --cc=jlayton@kernel.org \
    --cc=jlbec@evilplan.org \
    --cc=jmorris@namei.org \
    --cc=josef@toxicpanda.com \
    --cc=joseph.qi@linux.alibaba.com \
    --cc=kernel-team@android.com \
    --cc=linux-afs@lists.infradead.org \
    --cc=linux-btrfs@vger.kernel.org \
    --cc=linux-cifs@vger.kernel.org \
    --cc=linux-erofs@lists.ozlabs.org \
    --cc=linux-ext4@vger.kernel.org \
    --cc=linux-f2fs-devel@lists.sourceforge.net \
    --cc=linux-fsdevel@vger.kernel.org \
    --cc=linux-integrity@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    --cc=linux-mtd@lists.infradead.org \
    --cc=linux-nfs@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=linux-unionfs@vger.kernel.org \
    --cc=linux-xfs@vger.kernel.org \
    --cc=lucho@ionkov.net \
    --cc=malat@debian.org \
    --cc=mark@fasheh.com \
    --cc=martin@omnibond.com \
    --cc=miklos@szeredi.hu \
    --cc=netdev@vger.kernel.org \
    --cc=ocfs2-devel@oss.oracle.com \
    --cc=paul@paul-moore.com \
    --cc=phillip@squashfs.org.uk \
    --cc=reiserfs-devel@vger.kernel.org \
    --cc=richard@nod.at \
    --cc=rpeterso@redhat.com \
    --cc=sage@redhat.com \
    --cc=samba-technical@lists.samba.org \
    --cc=sds@tycho.nsa.gov \
    --cc=selinux@vger.kernel.org \
    --cc=serge@hallyn.com \
    --cc=sfrench@samba.org \
    --cc=shaggy@kernel.org \
    --cc=slava@dubeyko.com \
    --cc=stable@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=trond.myklebust@hammerspace.com \
    --cc=tyhicks@canonical.com \
    --cc=tytso@mit.edu \
    --cc=v9fs-developer@lists.sourceforge.net \
    --cc=viro@zeniv.linux.org.uk \
    --cc=yuchao0@huawei.com \
    --cc=zohar@linux.ibm.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-mm Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-mm/0 linux-mm/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-mm linux-mm/ https://lore.kernel.org/linux-mm \
		linux-mm@kvack.org
	public-inbox-index linux-mm

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kvack.linux-mm


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git