From mboxrd@z Thu Jan 1 00:00:00 1970 From: Linus Torvalds Subject: Re: [PATCH 05/24] devtmpfs: open code ksys_chdir and ksys_chroot Date: Tue, 21 Jul 2020 09:49:17 -0700 Message-ID: References: <20200721162818.197315-1-hch@lst.de> <20200721162818.197315-6-hch@lst.de> Mime-Version: 1.0 Content-Type: text/plain; charset="UTF-8" Return-path: In-Reply-To: <20200721162818.197315-6-hch-jcswGhMUV9g@public.gmane.org> Sender: linux-api-owner-u79uwXL29TY76Z2rM5mHXA@public.gmane.org To: Christoph Hellwig Cc: Al Viro , Greg Kroah-Hartman , "Rafael J. Wysocki" , Linux Kernel Mailing List , linux-raid-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-fsdevel , Linux API List-Id: linux-raid.ids On Tue, Jul 21, 2020 at 9:28 AM Christoph Hellwig 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