[05/24] devtmpfs: open code ksys_chdir and ksys_chroot
diff mbox series

Message ID 20200721162818.197315-6-hch@lst.de
State New
Headers show
Series
  • [01/24] fs: refactor do_mount
Related show

Commit Message

Christoph Hellwig July 21, 2020, 4:27 p.m. UTC
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(-)

Comments

Linus Torvalds July 21, 2020, 4:49 p.m. UTC | #1
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
Al Viro July 21, 2020, 5:16 p.m. UTC | #2
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.
Christoph Hellwig July 21, 2020, 6:26 p.m. UTC | #3
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.

Patch
diff mbox series

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);