| Message ID | 20170125145251.GB51023@ubuntu-hedt (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. --
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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Seth Forshee <seth.forshee@canonical.com> writes: > 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. So I agree that for the case of submounts that we don't need an additional permission check as the initial mount of the filesystem is that permission check already. I can see some interesting corner cases where a permission check might be required at the point when we have unprivileged mounts with submounts in the mix. As sget might find a filesystem that was already mounted elsewhere. But we don't have to face that case today. What is needs to be fixed are the permission checks cause problems for existing filesystems with submounts. What we need to fix those is knowledge the mount is happening for a submount/automount. At which point it is simple to turn off the permission check. Knowledge that a mount is a submount and not the ordinary kind of mount is going to require something like Seth's patch. So after having gone off to a corner and thought about this I believe I have come up with a minimal patch that makes things good for everyone. Eric -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/fs/afs/mntpt.c b/fs/afs/mntpt.c index 81dd075356b9..4455e64610d3 100644 --- a/fs/afs/mntpt.c +++ b/fs/afs/mntpt.c @@ -202,7 +202,7 @@ static struct vfsmount *afs_mntpt_do_automount(struct dentry *mntpt) /* try and do the mount */ _debug("--- attempting mount %s -o %s ---", devname, options); - mnt = vfs_kern_mount(&afs_fs_type, 0, devname, options); + mnt = vfs_kern_automount(mntpt, &afs_fs_type, 0, devname, options); _debug("--- mount result %p ---", mnt); free_page((unsigned long) devname); diff --git a/fs/cifs/cifs_dfs_ref.c b/fs/cifs/cifs_dfs_ref.c index ec9dbbcca3b9..d378f88e8630 100644 --- a/fs/cifs/cifs_dfs_ref.c +++ b/fs/cifs/cifs_dfs_ref.c @@ -245,9 +245,10 @@ char *cifs_compose_mount_options(const char *sb_mountdata, * @fullpath: full path in UNC format * @ref: server's referral */ -static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb, +static struct vfsmount *cifs_dfs_do_refmount(struct dentry *mntpt, const char *fullpath, const struct dfs_info3_param *ref) { + struct cifs_sb_info *cifs_sb = CIFS_SB(mntpt->d_sb); struct vfsmount *mnt; char *mountdata; char *devname = NULL; @@ -259,7 +260,7 @@ static struct vfsmount *cifs_dfs_do_refmount(struct cifs_sb_info *cifs_sb, if (IS_ERR(mountdata)) return (struct vfsmount *)mountdata; - mnt = vfs_kern_mount(&cifs_fs_type, 0, devname, mountdata); + mnt = vfs_kern_automount(mntpt, &cifs_fs_type, 0, devname, mountdata); kfree(mountdata); kfree(devname); return mnt; @@ -334,7 +335,7 @@ static struct vfsmount *cifs_dfs_do_automount(struct dentry *mntpt) mnt = ERR_PTR(-EINVAL); break; } - mnt = cifs_dfs_do_refmount(cifs_sb, + mnt = cifs_dfs_do_refmount(mntpt, full_path, referrals + i); cifs_dbg(FYI, "%s: cifs_dfs_do_refmount:%s , mnt:%p\n", __func__, referrals[i].node_name, mnt); diff --git a/fs/debugfs/inode.c b/fs/debugfs/inode.c index f17fcf89e18e..d45f131af869 100644 --- a/fs/debugfs/inode.c +++ b/fs/debugfs/inode.c @@ -187,9 +187,9 @@ static const struct super_operations debugfs_super_operations = { static struct vfsmount *debugfs_automount(struct path *path) { - struct vfsmount *(*f)(void *); - f = (struct vfsmount *(*)(void *))path->dentry->d_fsdata; - return f(d_inode(path->dentry)->i_private); + struct vfsmount *(*f)(struct dentry *, void *); + f = (struct vfsmount *(*)(struct dentry *, void *))path->dentry->d_fsdata; + return f(path->dentry, d_inode(path->dentry)->i_private); } static const struct dentry_operations debugfs_dops = { @@ -504,7 +504,7 @@ EXPORT_SYMBOL_GPL(debugfs_create_dir); */ struct dentry *debugfs_create_automount(const char *name, struct dentry *parent, - struct vfsmount *(*f)(void *), + struct vfsmount *(*f)(struct dentry *, void *), void *data) { struct dentry *dentry = start_creating(name, parent); 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); + static struct mount *clone_mnt(struct mount *old, struct dentry *root, int flag) { diff --git a/fs/nfs/namespace.c b/fs/nfs/namespace.c index 5551e8ef67fd..dce529c50cbd 100644 --- a/fs/nfs/namespace.c +++ b/fs/nfs/namespace.c @@ -222,11 +222,11 @@ void nfs_release_automount_timer(void) /* * Clone a mountpoint of the appropriate type */ -static struct vfsmount *nfs_do_clone_mount(struct nfs_server *server, +static struct vfsmount *nfs_do_clone_mount(struct dentry *dentry, const char *devname, struct nfs_clone_mount *mountdata) { - return vfs_kern_mount(&nfs_xdev_fs_type, 0, devname, mountdata); + return vfs_kern_automount(dentry, &nfs_xdev_fs_type, 0, devname, mountdata); } /** @@ -261,7 +261,7 @@ struct vfsmount *nfs_do_submount(struct dentry *dentry, struct nfs_fh *fh, mnt = (struct vfsmount *)devname; if (IS_ERR(devname)) goto free_page; - mnt = nfs_do_clone_mount(NFS_SB(dentry->d_sb), devname, &mountdata); + mnt = nfs_do_clone_mount(dentry, devname, &mountdata); free_page: free_page((unsigned long)page); out: diff --git a/fs/super.c b/fs/super.c index 1709ed029a2c..b33b7d606aec 100644 --- a/fs/super.c +++ b/fs/super.c @@ -166,6 +166,7 @@ static void destroy_super(struct super_block *s) list_lru_destroy(&s->s_inode_lru); security_sb_free(s); WARN_ON(!list_empty(&s->s_mounts)); + put_cred(s->s_cred); put_user_ns(s->s_user_ns); kfree(s->s_subtype); kfree(s->s_options); @@ -193,6 +194,7 @@ static struct super_block *alloc_super(struct file_system_type *type, int flags, INIT_LIST_HEAD(&s->s_mounts); s->s_user_ns = get_user_ns(user_ns); + s->s_cred = get_current_cred(); if (security_sb_alloc(s)) goto fail; diff --git a/include/linux/debugfs.h b/include/linux/debugfs.h index 014cc564d1c4..67ac9f10e7b7 100644 --- a/include/linux/debugfs.h +++ b/include/linux/debugfs.h @@ -99,7 +99,7 @@ struct dentry *debugfs_create_symlink(const char *name, struct dentry *parent, struct dentry *debugfs_create_automount(const char *name, struct dentry *parent, - struct vfsmount *(*f)(void *), + struct vfsmount *(*f)(struct dentry *, void *), void *data); void debugfs_remove(struct dentry *dentry); @@ -211,7 +211,7 @@ static inline struct dentry *debugfs_create_symlink(const char *name, static inline struct dentry *debugfs_create_automount(const char *name, struct dentry *parent, - struct vfsmount *(*f)(void *), + struct vfsmount *(*f)(struct dentry *, void *), void *data) { return ERR_PTR(-ENODEV); diff --git a/include/linux/fs.h b/include/linux/fs.h index 2ba074328894..cae845944d1d 100644 --- a/include/linux/fs.h +++ b/include/linux/fs.h @@ -1374,6 +1374,9 @@ struct super_block { */ struct user_namespace *s_user_ns; + /* Credentials of the user who mounted the filesystem */ + const struct cred *s_cred; + /* * Keep the lru lists last in the structure so they always sit on their * own individual cachelines. diff --git a/include/linux/mount.h b/include/linux/mount.h index c6f55158d5e5..b9cdca0c6b1a 100644 --- a/include/linux/mount.h +++ b/include/linux/mount.h @@ -90,6 +90,9 @@ struct file_system_type; extern struct vfsmount *vfs_kern_mount(struct file_system_type *type, int flags, const char *name, void *data); +struct vfsmount *vfs_kern_automount(struct dentry *dentry, + struct file_system_type *type, + int flags, const char *name, void *data); extern void mnt_set_expiry(struct vfsmount *mnt, struct list_head *expiry_list); extern void mark_mounts_for_expiry(struct list_head *mounts); diff --git a/kernel/trace/trace.c b/kernel/trace/trace.c index d7449783987a..7bb959abd5f1 100644 --- a/kernel/trace/trace.c +++ b/kernel/trace/trace.c @@ -7503,7 +7503,7 @@ init_tracer_tracefs(struct trace_array *tr, struct dentry *d_tracer) ftrace_init_tracefs(tr, d_tracer); } -static struct vfsmount *trace_automount(void *ingore) +static struct vfsmount *trace_automount(struct dentry *dentry, void *ingore) { struct vfsmount *mnt; struct file_system_type *type; @@ -7516,7 +7516,7 @@ static struct vfsmount *trace_automount(void *ingore) type = get_fs_type("tracefs"); if (!type) return NULL; - mnt = vfs_kern_mount(type, 0, "tracefs", NULL); + mnt = vfs_kern_automount(dentry, type, 0, "tracefs", NULL); put_filesystem(type); if (IS_ERR(mnt)) return NULL;