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:	Wed, 7 Dec 2011 02:17:21 +0000
From:	Al Viro <viro@...IV.linux.org.uk>
To:	Linus Torvalds <torvalds@...ux-foundation.org>
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 06, 2011 at 06:02:22PM -0800, Linus Torvalds wrote:
> You are the only one who seems to think that it matters. No code
> agrees with you except for the clearly broken AppArmor code that
> everybody agrees should just go the f*ck away.
> 
> > See what I'm talking about? ?I'm fine with giving the pathname to global
> > root. ?It's doing that to *random* just-unmounted vfsmount that is not
> > a good thing.
> 
> It *never* matters. The pathname should never be used at all.
> 
> We want to *see* what the pathname is, but no code should ever use it.
> 
> The *only* valid use for the broken pathname is for a "show user debug
> information". That's all I've ever claimed. The "where it was mounted
> - or *if* it was mounted" part is pointless.

Sigh...  OK, let's leave that.  I'm not saying that any code uses that
information - in fact, if you look at the last patch I've posted, nothing
(including apparmor) does.  I have a problem with wildly racy debug info
randomly intermixed with reliable one, but in the end it doesn't matter.

Could you look at that patch and comment on it, on its merits alone?
What's done there:
	* prepend_path() root argument becomes const.  I think everyone
agrees with that.
	* __d_path() is never called with NULL/NULL root.  It was a kludge
to start with.  Instead, we have an explicit function - d_absolute_root().
Same as __d_path(), except that it doesn't get root passed and stops where
it stops.  apparmor and tomoyo are using it.
	* __d_path() returns NULL on path outside of root.  The main
caller is show_mountinfo() and that's precisely what we pass root for - to
skip those outside chroot jail.  Those who don't want that can (and do)
use d_path().
	* __d_path() root argument becomes const.  Everyone agrees, I hope.
	* apparmor does *NOT* try to use __d_path() or any of its variants
when it sees that path->mnt is an internal vfsmount.  In that case it's
definitely not mounted anywhere and dentry_path() is exactly what we want
there.  Handling of sysctl()-triggered weirdness is moved to that place.
	* if apparmor is asked to do pathname relative to chroot jail
and __d_path() tells it we it's not in that jail, the sucker just calls
d_absolute_path() instead.  That's the other remaining caller of __d_path(),
BTW.

Are you OK with the above?
--
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/