linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] vfs: Partially revert addition of cred override in follow_automount()
@ 2017-02-22 13:36 David Howells
  2017-02-22 15:17 ` David Howells
  2017-02-22 15:17 ` Seth Forshee
  0 siblings, 2 replies; 3+ messages in thread
From: David Howells @ 2017-02-22 13:36 UTC (permalink / raw)
  To: viro
  Cc: linux-kernel, dhowells, Seth Forshee, Eric W. Biederman,
	linux-fsdevel, linux-afs

The following commit:

	commit aeaa4a79ff6a5ed912b7362f206cf8576fca538b
	Author: Eric W. Biederman <ebiederm@xmission.com>
	Date:   Sat Jul 23 11:20:44 2016 -0500
	fs: Call d_automount with the filesystems creds

brackets the call to ->d_automount() with override_creds() and
revert_creds(), setting the initial credentials for use whilst
automounting.

This, however, breaks AFS as it's no longer able to access the calling
process's keyrings to read the destination on a mountpoint.  This has gone
unnoticed till now because, to this point, stat'ing or validating the inode
caused the body of the mountpoint to be read into the pagecache (so that we
could determine whether what we were looking at was really a mountpoint).

However, the page containing the mountpoint destination is merely *cached*
and not pinned.  If it gets discarded and we try to read it in d_automount,
we may fail because we have no authentication tokens available.

So, for the moment, revert the addition of override_creds() and
revert_creds() and their variable.

Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Seth Forshee <seth.forshee@canonical.com>
cc: "Eric W. Biederman" <ebiederm@xmission.com>
cc: Al Viro <viro@ZenIV.linux.org.uk>
---

 fs/namei.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/fs/namei.c b/fs/namei.c
index ad74877e1442..dff5cd3b556f 100644
--- a/fs/namei.c
+++ b/fs/namei.c
@@ -1100,7 +1100,6 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 			    bool *need_mntput)
 {
 	struct vfsmount *mnt;
-	const struct cred *old_cred;
 	int err;
 
 	if (!path->dentry->d_op || !path->dentry->d_op->d_automount)
@@ -1129,9 +1128,7 @@ static int follow_automount(struct path *path, struct nameidata *nd,
 	if (nd->total_link_count >= 40)
 		return -ELOOP;
 
-	old_cred = override_creds(&init_cred);
 	mnt = path->dentry->d_op->d_automount(path);
-	revert_creds(old_cred);
 	if (IS_ERR(mnt)) {
 		/*
 		 * The filesystem is allowed to return -EISDIR here to indicate

^ permalink raw reply related	[flat|nested] 3+ messages in thread

* Re: [PATCH] vfs: Partially revert addition of cred override in follow_automount()
  2017-02-22 13:36 [PATCH] vfs: Partially revert addition of cred override in follow_automount() David Howells
@ 2017-02-22 15:17 ` David Howells
  2017-02-22 15:17 ` Seth Forshee
  1 sibling, 0 replies; 3+ messages in thread
From: David Howells @ 2017-02-22 15:17 UTC (permalink / raw)
  Cc: dhowells, viro, linux-kernel, Seth Forshee, Eric W. Biederman,
	linux-fsdevel, linux-afs

David Howells <dhowells@redhat.com> wrote:

> The following commit:
> 
> 	commit aeaa4a79ff6a5ed912b7362f206cf8576fca538b
> 	Author: Eric W. Biederman <ebiederm@xmission.com>
> 	Date:   Sat Jul 23 11:20:44 2016 -0500
> 	fs: Call d_automount with the filesystems creds
> 
> brackets the call to ->d_automount() with override_creds() and
> revert_creds(), setting the initial credentials for use whilst
> automounting.
> 
> This, however, breaks AFS as it's no longer able to access the calling
> process's keyrings to read the destination on a mountpoint.  This has gone
> unnoticed till now because, to this point, stat'ing or validating the inode
> caused the body of the mountpoint to be read into the pagecache (so that we
> could determine whether what we were looking at was really a mountpoint).
> 
> However, the page containing the mountpoint destination is merely *cached*
> and not pinned.  If it gets discarded and we try to read it in d_automount,
> we may fail because we have no authentication tokens available.
> 
> So, for the moment, revert the addition of override_creds() and
> revert_creds() and their variable.
> 
> Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Seth Forshee <seth.forshee@canonical.com>
> cc: "Eric W. Biederman" <ebiederm@xmission.com>
> cc: Al Viro <viro@ZenIV.linux.org.uk>

This is also resolved by:

	https://git.kernel.org/cgit/linux/kernel/git/ebiederm/user-namespace.git/commit/?h=for-next&id=93faccbbfa958a9668d3ab4e30f38dd205cee8d8

David

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] vfs: Partially revert addition of cred override in follow_automount()
  2017-02-22 13:36 [PATCH] vfs: Partially revert addition of cred override in follow_automount() David Howells
  2017-02-22 15:17 ` David Howells
@ 2017-02-22 15:17 ` Seth Forshee
  1 sibling, 0 replies; 3+ messages in thread
From: Seth Forshee @ 2017-02-22 15:17 UTC (permalink / raw)
  To: David Howells
  Cc: viro, linux-kernel, Eric W. Biederman, linux-fsdevel, linux-afs

On Wed, Feb 22, 2017 at 01:36:32PM +0000, David Howells wrote:
> The following commit:
> 
> 	commit aeaa4a79ff6a5ed912b7362f206cf8576fca538b
> 	Author: Eric W. Biederman <ebiederm@xmission.com>
> 	Date:   Sat Jul 23 11:20:44 2016 -0500
> 	fs: Call d_automount with the filesystems creds
> 
> brackets the call to ->d_automount() with override_creds() and
> revert_creds(), setting the initial credentials for use whilst
> automounting.
> 
> This, however, breaks AFS as it's no longer able to access the calling
> process's keyrings to read the destination on a mountpoint.  This has gone
> unnoticed till now because, to this point, stat'ing or validating the inode
> caused the body of the mountpoint to be read into the pagecache (so that we
> could determine whether what we were looking at was really a mountpoint).
> 
> However, the page containing the mountpoint destination is merely *cached*
> and not pinned.  If it gets discarded and we try to read it in d_automount,
> we may fail because we have no authentication tokens available.
> 
> So, for the moment, revert the addition of override_creds() and
> revert_creds() and their variable.
> 
> Fixes: aeaa4a79ff6a ("fs: Call d_automount with the filesystems creds")
> Signed-off-by: David Howells <dhowells@redhat.com>
> cc: Seth Forshee <seth.forshee@canonical.com>
> cc: "Eric W. Biederman" <ebiederm@xmission.com>
> cc: Al Viro <viro@ZenIV.linux.org.uk>

Eric's already applied a patch that should fix this problem. As far as I
know it's only been tested against NFS though, so you might want to test
that it also fixes the issue with AFS.

https://git.kernel.org/cgit/linux/kernel/git/next/linux-next.git/commit/?id=93faccbbfa958a9668d3ab4e30f38dd205cee8d8

Thanks,
Seth

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2017-02-22 15:18 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-22 13:36 [PATCH] vfs: Partially revert addition of cred override in follow_automount() David Howells
2017-02-22 15:17 ` David Howells
2017-02-22 15:17 ` Seth Forshee

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).