linux-kernel - Re: [git pull] apparmor fix for __d

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:	Tue, 6 Dec 2011 16:39:40 -0800
From:	Linus Torvalds <torvalds@...ux-foundation.org>
To:	Al Viro <viro@...iv.linux.org.uk>
Cc:	John Johansen <john.johansen@...onical.com>,
	linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] apparmor fix for __d_path() misuse

On Tue, Dec 6, 2011 at 4:16 PM, Al Viro <viro@...iv.linux.org.uk> wrote:
>
> You get broken /proc/self/mountinfo for chrooted processes with that patch.
> You also get /proc/mounts contents change for the same.

I have no objections to your

+		if (!p)
+			return SEQ_SKIP;

part of the patch, although it would just become "if (*p == '?')" in my version.

It's the "bastard" thing I dislike, because I don't think there
could/should *ever* be any valid use of that interface. The one user
(AppArmor) was just broken.

And no, I'm not married to the '?' either. I do think that giving
*some* value for the broken case is quite healthy, because it allows
debug output (as in "I'm giving you this path, but I know it's crap")

> I don't know... playing with magical substrings in what it returns is,
> IMO, a bad idea.  I really wonder if we'd be better off with just
> this:
>        __d_path(path, root, buf, buflen) - expects non-NULL in
> root->mnt, never changes root, returns NULL if path is not under root

I'm ok with that. *Most* users want that.

I suspect some users really might want a path for debugging, though.

>        d_absolute_path(path, ancestor, buf, buflen) - grabs the
> reference to the most remote ancestor it can find, puts pathname
> into buf, never returns NULL.

But why do you want that ancestor thing?

Nobody *ever* wants the ancestor. Even AppArmor didn't really want it,
it's just being terminally confused.

Even the thing that AppArmor actually checks ("is the ancestor in
/proc/sys") is totally broken. It doesn't even *work*. It's entirely
possible that /proc/sys was unmounted, but it's also possible that
it's mounted outside of the root, and then the ancestor is something
totally different. Testing the ancestor is *crazy*. Even *asking* for
it is a sign of terminal confusion. It's useless. It's meaningless.

So stop doing it. It doesn't become any better from having been
renamed "ancestor" instead of "bastard". The concept is broken and
wrong.

The "deepest ancestor I can reach" is simply not useful information.
Any user is broken by design. It cannot work.

So any interface that returns that ancestor/bastard is simply wrong.

                     Linus
--
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/