| Message ID | 20200721162818.197315-6-hch@lst.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/24] fs: refactor do_mount | expand |
On Tue, Jul 21, 2020 at 9:28 AM Christoph Hellwig <hch@lst.de> wrote: > > + > + /* traverse into overmounted root and then chroot to it */ > + if (!kern_path("/..", LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path) && > + !inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR) && > + ns_capable(current_user_ns(), CAP_SYS_CHROOT) && > + !security_path_chroot(&path)) { > + set_fs_pwd(current->fs, &path); > + set_fs_root(current->fs, &path); > + } > + path_put(&path); This looks wrong. You're doing "path_put()" even if kern_path() didn't succeed. As far as I can tell, that will either put some uninitialized garbage and cause an oops, or put something that has already been released by the failure path. Maybe that doesn't happen in practice in this case, but it's still very very wrong. Plus you shouldn't have those kinds of insanely complex if-statements in the first place. That was what caused the bug - trying to be clever, instead of writing clear code. I'm not liking how I'm finding fundamental mistakes in patches that _should_ be trivial conversions with no semantic changes. Linus
On Tue, Jul 21, 2020 at 09:49:17AM -0700, Linus Torvalds wrote: > On Tue, Jul 21, 2020 at 9:28 AM Christoph Hellwig <hch@lst.de> wrote: > > > > + > > + /* traverse into overmounted root and then chroot to it */ > > + if (!kern_path("/..", LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path) && > > + !inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR) && > > + ns_capable(current_user_ns(), CAP_SYS_CHROOT) && > > + !security_path_chroot(&path)) { > > + set_fs_pwd(current->fs, &path); > > + set_fs_root(current->fs, &path); > > + } > > + path_put(&path); > > This looks wrong. It is wrong. kern_path() leaves *path unmodified in case of error, and that struct path is uninitialized here.
On Tue, Jul 21, 2020 at 06:16:27PM +0100, Al Viro wrote: > On Tue, Jul 21, 2020 at 09:49:17AM -0700, Linus Torvalds wrote: > > On Tue, Jul 21, 2020 at 9:28 AM Christoph Hellwig <hch@lst.de> wrote: > > > > > > + > > > + /* traverse into overmounted root and then chroot to it */ > > > + if (!kern_path("/..", LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path) && > > > + !inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR) && > > > + ns_capable(current_user_ns(), CAP_SYS_CHROOT) && > > > + !security_path_chroot(&path)) { > > > + set_fs_pwd(current->fs, &path); > > > + set_fs_root(current->fs, &path); > > > + } > > > + path_put(&path); > > > > This looks wrong. > > It is wrong. kern_path() leaves *path unmodified in case of error, and > that struct path is uninitialized here. Yep. Only saving grace is that the error just doesn't happen during early init.
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c index 5e8d677ee783bc..f798d3976b4052 100644 --- a/drivers/base/devtmpfs.c +++ b/drivers/base/devtmpfs.c @@ -25,6 +25,7 @@ #include <linux/sched.h> #include <linux/slab.h> #include <linux/kthread.h> +#include <linux/fs_struct.h> #include <uapi/linux/mount.h> #include "base.h" @@ -393,6 +394,7 @@ static int handle(const char *name, umode_t mode, kuid_t uid, kgid_t gid, static int devtmpfs_setup(void *p) { + struct path path; int err; err = ksys_unshare(CLONE_NEWNS); @@ -401,8 +403,16 @@ static int devtmpfs_setup(void *p) err = devtmpfs_do_mount("/"); if (err) goto out; - ksys_chdir("/.."); /* will traverse into overmounted root */ - ksys_chroot("."); + + /* traverse into overmounted root and then chroot to it */ + if (!kern_path("/..", LOOKUP_FOLLOW | LOOKUP_DIRECTORY, &path) && + !inode_permission(path.dentry->d_inode, MAY_EXEC | MAY_CHDIR) && + ns_capable(current_user_ns(), CAP_SYS_CHROOT) && + !security_path_chroot(&path)) { + set_fs_pwd(current->fs, &path); + set_fs_root(current->fs, &path); + } + path_put(&path); out: *(int *)p = err; complete(&setup_done);
devtmpfs is the only non-early init caller of ksys_chdir and ksys_chroot with kernel pointers. Just open code the two operations which only really need a single path lookup anyway in devtmpfs_setup instead. The open coded verson doesn't need any of the stale dentry revalidation logic from the full blown version as those can't happen on tmpfs and ramfs. Signed-off-by: Christoph Hellwig <hch@lst.de> --- drivers/base/devtmpfs.c | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-)