Re: [git pull] apparmor fix for __d_path() misuse

[John Johansen <john.johansen@canonical.com>]

On 12/06/2011 01:41 PM, Al Viro wrote:
> On Tue, Dec 06, 2011 at 01:07:56PM -0800, Linus Torvalds wrote:
>> On Tue, Dec 6, 2011 at 12:53 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>>
>>> The trouble is, might very well stop *NOT* at the global root. ?Consider
>>> a race with umount -l; we have no way to tell "it had been outside of
>>> chroot jail" from "it had walked up to the place where ->mnt_parent had
>>> been already reset, sorry, no idea what it was".
>>
>> Sure, but you made that case return NULL already as part of the "no
>> bastard" case, didn't you?
>>
>> That part of the patch looked fine.
>>
>> It was just the extra convolutions around 'bastard' that seemed to be
>> over-designed, and made for just a single use that seems very
>> peripheral anyway.
>>
>> Apart from AppArmor, afaik nobody even really cared where they ended
>> up, and even AppArmor really didn't want to know - it just had this
>> totally crazy special case about "/sys".
> 
> It's not /sys, actually, it's implicit /proc/sys/something from sysctl() ;-/
> 
> Hell knows.  Frankly, the whole "let's build a pathname and decide basing
> on it" thing had been insane from the very beginning.  For many reasons,
> starting with "pathname in which namespace?" and continuing through "who said
> that it still means what it used to at the moment of operation" to "what
> if it's not a part of any namespace anymore/had never been in one".
> 
> Hey, it wasn't me who insisted that pathname-based stuff made any sense
> whatsoever...  But "path to the nearest thing that has no ancestor now"
> is _meaningless_ if we know nothing about that ancestor.  Cases include
> 	* that ancestor is the root of our namespace.
> 	* that ancestor is a solitary vfsmount we'd started in, never had
> been mounted in any namespace (e.g. procfs internal vfsmount, etc.)
> 	* that ancestor is a random vfsmount in a subtree that had been
> hit by umount -l, just as we'd been looking at it.  Might be its root,
> might be equal to path->mnt, might be something in between.
> 	* cases 1 and 3 in another process' namespace
> 
> In case 3 and analogs the path we'd obtained is pure junk, so I can agree
> with "let's just return NULL on those".  The trouble is, how do you tell
> (3) from (2)?  And do we want (1)-not-our-namespace to be distinguished
> from (1)-our-namespace?

You don't, and the chroot case of walking back to the namespace while sort
of possible isn't something I like either.  This whole mess with bastard
comes about because apparmor is supposed to deny access to the disconnected
paths, so it needs to know where it stopped so it can disconnect the path
that __d_path so helpfully attaches to /.  As I said early its insane to
mediate off of disconnected paths, and it can only be done via a previous
labeling.

The path value we get in these disconnected path cases is used for logging
to help the user figuring out why/where the access was denied.  Having
this information when building policy his helpful, but isn't critical as
it can only be used as a guide.

The current labeling in apparmor is crude and needs to be updated/replaced,
It is the only way to correctly handle fs namespaces.  As I offered before
we can break the one case that is currently problematic, until we can get
our labeling in order.

It would be nice to still be able to get a path back, in the cases where
the path isn't connected to root so we can log it but if we have to we can
live without it.