linux-kernel - Re: [PATCH v3 1/3] namei: implement O_BENEATH-style AT

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:   Sat, 13 Oct 2018 19:20:03 +1100
From:   Aleksa Sarai <cyphar@...har.com>
To:     Al Viro <viro@...IV.linux.org.uk>
Cc:     Eric Biederman <ebiederm@...ssion.com>,
        Christian Brauner <christian@...uner.io>,
        Jeff Layton <jlayton@...nel.org>,
        "J. Bruce Fields" <bfields@...ldses.org>,
        Arnd Bergmann <arnd@...db.de>,
        Andy Lutomirski <luto@...nel.org>,
        David Howells <dhowells@...hat.com>,
        Jann Horn <jannh@...gle.com>, Tycho Andersen <tycho@...ho.ws>,
        David Drysdale <drysdale@...gle.com>, dev@...ncontainers.org,
        containers@...ts.linux-foundation.org,
        linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
        linux-arch@...r.kernel.org, linux-api@...r.kernel.org
Subject: Re: [PATCH v3 1/3] namei: implement O_BENEATH-style AT_* flags

On 2018-10-13, Al Viro <viro@...IV.linux.org.uk> wrote:
> > Pardon me, but... huh?  The reason for your two calls of dirfd_path_init() is,
> > AFAICS, the combination of absolute pathname with both LOOKUP_XDEV and
> > LOOKUP_BENEATH at the same time.  That combination is treated as if the pathname
> > had been relative.  Note that LOOKUP_BENEATH alone is ignored for absolute ones
> > (and with a good reason - it's a no-op on path_init() level in that case).
> > 
> > What the hell?  It complicates your code and doesn't seem to provide any benefits
> > whatsoever -- you could bloody well have passed the relative pathname to start with.
> > 
> > IDGI...  Without that kludge it becomes simply "do as we currently do for absolute
> > pathnames, call dirfd_path_init() for relative ones".  And I would argue that
> > taking LOOKUP_BENEATH handling out of dirfd_path_init() into path_init() (relative)
> > case would be a good idea.
> > 
> > As it is, the logics is very hard to follow.
> 
> 	... and it fails on LOOKUP_BENEATH anyway.  Egads...  So that's for your
> LOOKUP_CHROOT ;-/  IMO that's awful, especially with the way you've spread those
> LOOKUP_CHROOT cases between these two.

Yeah, the ->root setting in dirfd_path_init() is ugly. :/

> 	Why not simply have O_THISROOT pick root by dirfd and call file_open_root()?

Wouldn't this require replicating the dirfd_path_init()-like code inside
all of the path_*at() callers which use path_init()? Or is there another
common place we could put it?

> And if something wants it for stat(), etc. just have them use it combined with
> O_PATH and pass the result to ...at()...

This works for stat and quite a few other things (which is why I only
added openat(2) support for the moment), but I think we'd eventually
need something like this for renameat2(2) as well as a few other choice
*at(2) syscalls. Though I also think that more AT_EMPTY_PATH support
would removed the need for _most_ *at(2) implementations to use this.

-- 
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)