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 for Android: free password hash cracker in your pocket

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

Message-ID: <20111207001643.GN2203@ZenIV.linux.org.uk>
Date:	Wed, 7 Dec 2011 00:16:43 +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 03:45:15PM -0800, Linus Torvalds wrote:
> How about this change:
>  - don't change 'root' (and mark it const)
>  - if we hit the expected root, we're all happy and do what we do now
>  - if we hit some *unexpected* root (the "global root") add a '?' or
> something at the head of the path.
> 
> End result: callers like getcwd() can trivially replace their current
> "path_equal(&tmp,&root)" (or whatever they do) with just checking the
> first character of the end result. A good path always starts with '/'.

You get broken /proc/self/mountinfo for chrooted processes with that patch.
You also get /proc/mounts contents change for the same.

Moreover, while we _probably_ can get away with that "prepend '?'",
we'll need to make sure that all checks are comparing with '?', _not_
with '/', or you'll get nasty surprises when __d_path() gets called
on e.g. pipe dentry (pipe:[...]).  And while we are at it, we'd better
document that "->d_dname() should never use '?' as the first character"
restriction we've got.

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
	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.

Let tomoyo use that one and path_put(ancestor) afterwards (or look at
it first, if it cares).  And let apparmor do the following:
	* first call __d_path(), unless asked not to.  If it returns
non-NULL, great we've got that path, game over.  Otherwise call
d_absolute_path() and log that partial pathname, check where we'd got,
etc.  Just remember to path_put(ancestor) after that.

We are trying to shove two different things in one function and result
is ugly; so let's just split it instead of trying to breed weird
hybrids.

Comments?
--
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/