linux-kernel - Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 4 Sep 2013 06:35:59 -0400
From:	Jeff Layton <jlayton@...hat.com>
To:	Ian Kent <raven@...maw.net>
Cc:	Al Viro <viro@...IV.linux.org.uk>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	linux-fsdevel <linux-fsdevel@...r.kernel.org>,
	rui.xiang@...wei.com, autofs mailing list <autofs@...r.kernel.org>,
	Kernel Mailing List <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 1/3] autofs4 - fix device ioctl mount lookup

On Wed, 04 Sep 2013 12:07:28 +0800
Ian Kent <raven@...maw.net> wrote:

> On Wed, 2013-09-04 at 11:53 +0800, Ian Kent wrote:
> > On Wed, 2013-09-04 at 03:42 +0100, Al Viro wrote:
> > > On Wed, Sep 04, 2013 at 03:26:17AM +0100, Al Viro wrote:
> > > > I've applied slightly modified variant of Jeff's "vfs: allow umount to handle
> > > > mountpoints without revalidating them" (modified by just leaving the
> > > > struct path filled with mountpoint and leaving the equivalent of follow_mount()
> > > > to caller) to the local queue and I'm pretty sure that it's what we want
> > > > here as well.
> > > 
> > > ... and killed the modifications since the result ends up uglier for
> > > caller(s) anyway.  Reapplied as-is.
> > 

Thanks Al. Note that there's a follow-on fix for the kerneldoc header
on one of the functions that will be floating around in akpm's tree
soon too. No code changes there though.

> > But isn't that what's needed anyway?
> 
> Except for the question of following symlinks which is still bugging me.
> 
> Granted, the initial implementation (8d7b48e0) didn't check for a
> symlink but probably should have and returned EINVAL if it found one.
> 
> That's because the whole process is driven by automount maps that have
> specific paths, so needing to follow a symlink is an indication the
> caller is doing something wrong.
> 
> It's probably not actually harmful to cater for it in kernel .... though
> the user will likely crash and burn some time later.
> 

If you don't want to allow the last component to be a symlink then the
easiest fix is probably to turn user_path_umountat() into a wrapper
around a new kern_path_umountat() function and simply call that without
LOOKUP_FOLLOW set. Then check to see if the last component is a symlink
before you use it. Of course, that means that you can't case symlinks
in intermediate components either. I'm not sure if that's an issue for
you though.

Doing that wrapperization would be a good opportunity to rename the
function as well, but I'm struck with my usual lack of creativity in
that department.

> > 
> > Looks like it fits with the existing code that walks back down looking
> > for a match.
> > 
> > And that fits in with fixing the bug where we want the first match just
> > means breaking out early. The match should be close to the top, if not
> > the first then the second, for the common case.
> > 
> > Ian
> 
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-fsdevel" in
> the body of a message to majordomo@...r.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html


-- 
Jeff Layton <jlayton@...hat.com>
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/