Re: [PATCH] sunrpc: Use current_real_cred() when looking up rpc credentials

[Seth Forshee <seth.forshee@canonical.com>]

On Wed, Jan 25, 2017 at 03:51:30PM +0000, Trond Myklebust wrote:
> On Wed, 2017-01-25 at 08:52 -0600, Seth Forshee wrote:
> > On Wed, Jan 25, 2017 at 12:14:26AM +0000, Trond Myklebust wrote:
> > > Adding David Howells and Steve French as I believe both AFS and
> > > CIFS
> > > have the exact same requirements and NFS here.
> > > 
> > > On Wed, 2017-01-25 at 12:56 +1300, Eric W. Biederman wrote:
> > > > Trond Myklebust <trondmy@primarydata.com> writes:
> > > > 
> > > > > On Wed, 2017-01-25 at 12:28 +1300, Eric W. Biederman wrote:
> > > > > > With respect to nfs and automounts.
> > > > > > 
> > > > > > Does NFS have different automount behavior based on the user
> > > > > > performing the automount?
> > > > > > 
> > > > > > If NFS does not have different automount behavior depending
> > > > > > on
> > > > > > the
> > > > > > user
> > > > > > we just use the creds of the original mounter of NFS?
> > > > > > 
> > > > > > If NFS does have different automount behavior depending on
> > > > > > the
> > > > > > user
> > > > > > (ouch!) we need to go through the call path and see where it
> > > > > > makes
> > > > > > sense to over ride things and where it does not.
> > > > > 
> > > > > The reason why the NFS client creates a mountpoint is because
> > > > > on
> > > > > entering a directory, it detects that there is either a similar
> > > > > mountpoint on the server, or there is a referral (which acts
> > > > > sort
> > > > > of
> > > > > like a symlink, except it points to a path on one or more
> > > > > different
> > > > > NFS
> > > > > servers).
> > > > > Without that mountpoint, most things would work, but the user
> > > > > would
> > > > > end
> > > > > up seeing nasty non-posix features like duplicate inode
> > > > > numbers.
> > > > > 
> > > > > We do not want to use any creds other than user creds here,
> > > > > because
> > > > > as
> > > > > far as the security model is concerned, the process is just
> > > > > crossing
> > > > > into an existing directory.
> > > > 
> > > > But sget needs different creds.
> > > > 
> > > > Because the user who authorizes the mounting of the filesystem is
> > > > different than the user who is crossing into the new filesystem.
> > > > 
> > > > The local system now cares about that distinction even if the nfs
> > > > server
> > > > does not.
> > > 
> > > Why? The filesystem is already mounted. We're creating a new
> > > mountpoint, but we could equally well just say 'sod that' and
> > > create an
> > > ordinary directory. The penalty would be aforementioned non-posix
> > > weirdness.
> > > 
> > > > 
> > > > > > Seth the fundamental problem with your patch was that you
> > > > > > were
> > > > > > patching
> > > > > > a location that is used for more just mounts.
> > > > > > 
> > > > > > I am strongly wishing that we could just change
> > > > > > follow_automount
> > > > > > from:
> > > > > > 
> > > > > > 
> > > > > > 	old_cred = override_creds(&init_cred);
> > > > > > 	mnt = path->dentry->d_op->d_automount(path);
> > > > > > 	revert_creds(old_cred);
> > > > > > 
> > > > > > to:
> > > > > > 
> > > > > > 	old_cred = override_creds(path->mnt->mnt_sb->s_cred);
> > > > > > 	mnt = path->dentry->d_op->d_automount(path);
> > > > > > 	revert_creds(old_cred);
> > > > > > 
> > > > > > And all will be well with nfs.  That does remain possible.
> > > > > 
> > > > > No. That would break permissions checking. See above.
> > > > 
> > > > Then we need to look much harder at the permission checking
> > > > model of d_automount because we need to permission check against
> > > > both sets of creds.
> > 
> > How about something like this? Essentially, stash the creds used at
> > mount time in the super block, then create a vfs_kern_automount()
> > function which overrides the currend creds then calls
> > vfs_kern_mount().
> > 
> > Only compile tested so far, and probably it should be split up into
> > several patches.
> 
> > diff --git a/fs/namespace.c b/fs/namespace.c
> > index 487ba30bb5c6..da7e6dfa56cb 100644
> > --- a/fs/namespace.c
> > +++ b/fs/namespace.c
> > @@ -989,6 +989,21 @@ vfs_kern_mount(struct file_system_type *type,
> > int flags, const char *name, void
> >  }
> >  EXPORT_SYMBOL_GPL(vfs_kern_mount);
> >  
> > +struct vfsmount *
> > +vfs_kern_automount(struct dentry *dentry, struct file_system_type
> > *type,
> > +		   int flags, const char *name, void *data)
> > +{
> > +	const struct cred *old_cred;
> > +	struct vfsmount *mnt;
> > +
> > +	old_cred = override_creds(dentry->d_sb->s_cred);
> > +	mnt = vfs_kern_mount(type, flags, name, data);
> > +	revert_creds(old_cred);
> > +
> > +	return mnt;
> > +}
> > +EXPORT_SYMBOL_GPL(vfs_kern_automount);
> > +
> 
> Nope. As I said, the call into the filesystem needs the _user_ creds.

Okay. I thought that perhaps the user's creds were only needed when
looking up the mountpoint, and that after that it might be okay to
switch the creds. I guess not.

Thanks,
Seth