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 08:33:19 +0100
From:   Al Viro <viro@...IV.linux.org.uk>
To:     Aleksa Sarai <cyphar@...har.com>
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 Tue, Oct 09, 2018 at 06:02:28PM +1100, Aleksa Sarai wrote:

First of all, dirfd_path_init() part should be in a separate commit.  And I'm
really not happy with the logics in there.  dirfd_path_init() itself is
kinda-sorta reasonable.  It is equivalent to setting the starting point for
relative pathnames + setting ->root for LOOKUP_BENEATH, right?

But the part in path_init() is too bloody convoluted for its own good.  Let me
try to translate:

> +	if (unlikely(flags & LOOKUP_XDEV)) {
> +		error = dirfd_path_init(nd);
> +		if (unlikely(error))
> +			return ERR_PTR(error);
> +	}

* if LOOKUP_XDEV is set, set the starting point as if it was a relative
  pathname.  If LOOKUP_BENEATH was set as well, set ->root to the same
  point.
* if it's an absolute pathname, 
>  	if (*s == '/') {
... and we hadn't come here with LOOKUP_XDEV + LOOKUP_BENEATH, set ->root.
> +		if (likely(!nd->root.mnt))
> +			set_root(nd);
* if it's an absolute pathname, set the starting point to ->root.  Note that
if we came here with LOOKUP_XDEV, we'll discard the starting point we'd
calculated.
> +		error = nd_jump_root(nd);
> +		if (unlikely(error))
> +			s = ERR_PTR(error);
>  		return s;
>  	}
> +	if (likely(!nd->path.mnt)) {
* if we didn't have LOOKUP_XDEV, set the starting point as if it was a relative
pathname (which it is) and, if LOOKUP_BENEATH is also there, set ->root there
as well.
> +		error = dirfd_path_init(nd);
> +		if (unlikely(error))
> +			return ERR_PTR(error);
> +	}
> +	return s;
>  }

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.