[git pull] apparmor fix for __d_path() misuse

[git pull] apparmor fix for __d_path() misuse

[Al Viro]

	Fix for use-after-free race in apparmor d_namespace_path() and
partially sanitizing the atrocious __d_path() API that has caused that
mess in the first place.  Please, pull from
git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Shortlog:
Al Viro (1):
      fix apparmor dereferencing potentially freed dentry, sanitize __d_path() API

Diffstat:
 fs/dcache.c                |   73 +++++++++++++++++++++++++++----------------
 fs/namespace.c             |   20 ++++++-----
 fs/seq_file.c              |    8 ++--
 include/linux/dcache.h     |    3 +-
 include/linux/fs.h         |    1 +
 security/apparmor/path.c   |   36 ++++++++-------------
 security/tomoyo/realpath.c |    2 +-
 7 files changed, 79 insertions(+), 64 deletions(-)

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

[Al Viro]

On Tue, Dec 06, 2011 at 03:48:54PM +0000, Al Viro wrote:
> 	Fix for use-after-free race in apparmor d_namespace_path() and
> partially sanitizing the atrocious __d_path() API that has caused that
> mess in the first place.  Please, pull from
> git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Argh...  Dumb typo when copying jj's fixlet; fixed almost immediately,
but not committed before push.  Noticed only when switching branches...

Is there any way to have git to complain if you are pushing the current
branch with uncommitted changes in your repository?

Anyway, fix commit --amend'ed and pushed into the same branch.  Neither
diffstats nor changelog changed on that, of course...

Al, hoping Linus still hasn't pulled that one...

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 8:41 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Is there any way to have git to complain if you are pushing the current
> branch with uncommitted changes in your repository?

Git has hooks you can use for many ops, but that isn't one of them.
The hooks tend to be at the other end (ie around doing things that
change *your* repository, rather than change some remote repository).
So there are hooks around commit-like operations and around fetching
(so you can check that certain rules apply), but not for pushing.

That said, depending on *how* you push, you can fake it. For example,
since I push to multiple repositories (kernel.org and github, and
before I used github I used to have a separate "backup location" at a
linux-foundation site), I actually don't use "git push" to update
remote repositories, I have a git alias called "push-all" that pushes
to multiple remotes in one go.

And if you can teach your fingers some new way of pushing, then inside
that alias you can obviously add any commands you would want.

                          Linus

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 7:48 AM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>        Fix for use-after-free race in apparmor d_namespace_path() and
> partially sanitizing the atrocious __d_path() API that has caused that
> mess in the first place.

Ugh, having looked at this, I really don't want to pull it.

First off, I do agree that the __d_path() api isn't pleasant, and that
we can/should change it. I just don't agree with the particular
changes. Feel free to try to convince me otherwise, but..

As far as I can tell, the *only* user of the new 'bastard' interface
is (apart from the pure amusement for the name) that silly special
check for AppArmour.

Is AA *worth* that special case? Does it even care that deeply?

So my argument is that I think your change to make 'root' const an
dnot be changed is good, and should stay. But 'bastard' should just go
away, amusingly named or not. All sane callers already call it with a
NULL pointer. The one case that doesn't isn't worth worrying about,
and should strive to solve its problems some saner way.

John explicitly cc'd, since he's the one that would have to figure out
that /sys special case. Is it possible that just calling __d_path()
with the global system root would be sufficient for apparmor in this
case to figure out the /sys part?

                        Linus

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

[Al Viro]

On Tue, Dec 06, 2011 at 11:54:16AM -0800, Linus Torvalds wrote:
> Is AA *worth* that special case? Does it even care that deeply?
> 
> So my argument is that I think your change to make 'root' const an
> dnot be changed is good, and should stay. But 'bastard' should just go
> away, amusingly named or not. All sane callers already call it with a
> NULL pointer. The one case that doesn't isn't worth worrying about,
> and should strive to solve its problems some saner way.
> 
> John explicitly cc'd, since he's the one that would have to figure out
> that /sys special case. Is it possible that just calling __d_path()
> with the global system root would be sufficient for apparmor in this
> case to figure out the /sys part?

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

That's why __d_path() modifying root had been asking for trouble (besides
being in bad taste); the really are situations when it's very tempting
to look at the place where it had stopped.

Hell knows...  We definitely want to be able to distinguish global roots from
detached vfsmounts.  We _might_ want to distinguish the root of our namespace
from those of others, but I'm less sure about that.

And we don't have a lot of channels for returning extra information, short
of passing a pointer to store it in.  In which case we might bloody well
return the entire vfsmount/dentry pair and let the caller sort it out.

Don't get me wrong, I'm not particulary enthusiastic about that variant;
it's just that I don't see an alternative that would be any better.  I mean,
__d_path(path, root, buf, buflen, &enum_what_kind_of_place_have_I_ended_up_in)?
Can be done, but that'll be just as ugly *and* we'll end up with a growing
zoo of those "kinds" over the next few years... ;-/

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

[Linus Torvalds]

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

                 Linus

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

[Al Viro]

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?

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

[John Johansen]

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.

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

[John Johansen]

On 12/06/2011 01:07 PM, 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.
> 
it is, and the plan is to not need the bastard even.  What apparmor should
be doing is lazy labeling the live objects, and anything that is disconnected
is evaluated based on the previous labeling.  This will also remove its
use of passing root = { NULL, NULL } to __d_path.

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

right it only cared about where it endup in the cases of reaching the fs->root
or own_mnt.  The sys case really is just broken, I started looking at it when
Al poked me and said wtf, and have been looking at ways to remove it.

Right now we could drop the bastard parameter and passing root = { NULL, NULL }
__d_path and only break in the case of some chroot situations, which is not the
standard use case anyways.  We are willing to drop support for this, and it can
be picked backup when the labeling patch is done.

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

[Al Viro]

On Tue, Dec 06, 2011 at 02:19:08PM -0800, John Johansen wrote:
> On 12/06/2011 01:07 PM, 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.
> > 
> it is, and the plan is to not need the bastard even.  What apparmor should
> be doing is lazy labeling the live objects, and anything that is disconnected
> is evaluated based on the previous labeling.

I still don't understand how you deal with objects seen by processes in
different namespaces.  Please, explain lazy labeling in details...

You do realize that there's no hope whatsoever for prohibiting access
to struct file opened in parent's namespace and already written into
there, right?

> This will also remove its
> use of passing root = { NULL, NULL } to __d_path.

One word: tomoyo...  You are not the only weird one.  They want an absolute
pathname and don't care about races with umount.  Whatever it's relative
to, the thing just goes ahead and plays with itse^Wthe string it got.  As
long as they are not dereferencing pointers to free objects, I Don't Want To
Know(tm).

> Right now we could drop the bastard parameter and passing root = { NULL, NULL }

OK, please tell me what do you need from __d_path().  Suppose it has missed
the supplied root; what do you want to know about the place it had stopped
in?  If we don't do extra arguments, we are limited by this:
	* some condition is checked by __d_path(); if it's satisfied,
pointer to relative pathname (based hell-knows-where) is returned as
usual; we *can't* tell that from normal "reached root, here's your
pathname" case.
	* if it isn't satisfied, we return NULL.  No relative pathname
to look at, etc.

Unless you are OK with what __d_path(path, root, NULL, buf, buflen) is doing
with this patch, we'd need to split it; seq_path_root()/show_mountinfo()
requirements leave no wiggling space for case root->mnt != NULL,
bastard == NULL.

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

[John Johansen]

On 12/06/2011 02:41 PM, Al Viro wrote:
> On Tue, Dec 06, 2011 at 02:19:08PM -0800, John Johansen wrote:
>> On 12/06/2011 01:07 PM, 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.
>>>
>> it is, and the plan is to not need the bastard even.  What apparmor should
>> be doing is lazy labeling the live objects, and anything that is disconnected
>> is evaluated based on the previous labeling.
> 
> I still don't understand how you deal with objects seen by processes in
> different namespaces.  Please, explain lazy labeling in details...
> 
Right now we leverage the file->cred, which is very crude, and fails in
a lot of situations forcing us, to do a revalidation (file and permission
lookup), and then return an error based on that.  At the moment it can
thought of more as caching based on the original cred that was used opening
the file, if the tasks cred matches it gets to avoid the path lookup.

What should be is when a task goes to access an object, it checks the label,
if it doesn't pass the check, it falls backs to doing a path lookup, if that
succeeds the label is updated, if it fails access is denied.  And yes this
does necessitate the labeling being on something that can be updated.

I have been working on a better, much longer writeup, but its still needs a
lot of work.

> You do realize that there's no hope whatsoever for prohibiting access
> to struct file opened in parent's namespace and already written into
> there, right?
> 
so with the caveat that I may have misunderstood what you wrote, yes.

>> This will also remove its
>> use of passing root = { NULL, NULL } to __d_path.
> 
> One word: tomoyo...  You are not the only weird one.  They want an absolute
> pathname and don't care about races with umount.  Whatever it's relative
> to, the thing just goes ahead and plays with itse^Wthe string it got.  As
> long as they are not dereferencing pointers to free objects, I Don't Want To
> Know(tm).
> 
>> Right now we could drop the bastard parameter and passing root = { NULL, NULL }
> 
> OK, please tell me what do you need from __d_path().  Suppose it has missed
> the supplied root; what do you want to know about the place it had stopped
> in?  If we don't do extra arguments, we are limited by this:
> 	* some condition is checked by __d_path(); if it's satisfied,
> pointer to relative pathname (based hell-knows-where) is returned as
> usual; we *can't* tell that from normal "reached root, here's your
> pathname" case.
> 	* if it isn't satisfied, we return NULL.  No relative pathname
> to look at, etc.
> 
> Unless you are OK with what __d_path(path, root, NULL, buf, buflen) is doing
> with this patch, we'd need to split it; seq_path_root()/show_mountinfo()
> requirements leave no wiggling space for case root->mnt != NULL,
> bastard == NULL.

What we want to know is if we missed the supplied root, so that we don't
mediate off of that path.  And it would be nice to get a partial path for
the purposes of logging, so that there is some guidance on how to update
policy.

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

[Linus Torvalds]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

On Tue, Dec 6, 2011 at 3:12 PM, John Johansen
<john.johansen@canonical.com> wrote:
>
> What we want to know is if we missed the supplied root, so that we don't
> mediate off of that path.  And it would be nice to get a partial path for
> the purposes of logging, so that there is some guidance on how to update
> policy.

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

Something kind of like this (this does *not* change apparmor or tomoyo
- I didn't even look at those uses).

Comments?

               Linus

[-- Attachment #2: patch.diff --]
[-- Type: text/x-patch, Size: 4304 bytes --]

 fs/dcache.c              |   12 +++++-------
 fs/seq_file.c            |    6 ++----
 include/linux/dcache.h   |    2 +-
 include/linux/seq_file.h |    4 ++--
 4 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/fs/dcache.c b/fs/dcache.c
index 10ba92def3f6..93e063f713f5 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2448,7 +2448,7 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
  * If path is not reachable from the supplied root, then the value of
  * root is changed (without modifying refcounts).
  */
-static int prepend_path(const struct path *path, struct path *root,
+static int prepend_path(const struct path *path, const struct path *root,
 			char **buffer, int *buflen)
 {
 	struct dentry *dentry = path->dentry;
@@ -2500,8 +2500,7 @@ global_root:
 		WARN(1, "Root dentry has weird name <%.*s>\n",
 		     (int) dentry->d_name.len, dentry->d_name.name);
 	}
-	root->mnt = vfsmnt;
-	root->dentry = dentry;
+	error = prepend(buffer, buflen, "?", 1);
 	goto out;
 }
 
@@ -2522,7 +2521,7 @@ global_root:
  * If path is not reachable from the supplied root, then the value of
  * root is changed (without modifying refcounts).
  */
-char *__d_path(const struct path *path, struct path *root,
+char *__d_path(const struct path *path, const struct path *root,
 	       char *buf, int buflen)
 {
 	char *res = buf + buflen;
@@ -2758,19 +2757,18 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
 	write_seqlock(&rename_lock);
 	if (!d_unlinked(pwd.dentry)) {
 		unsigned long len;
-		struct path tmp = root;
 		char *cwd = page + PAGE_SIZE;
 		int buflen = PAGE_SIZE;
 
 		prepend(&cwd, &buflen, "\0", 1);
-		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
+		error = prepend_path(&pwd, &root, &cwd, &buflen);
 		write_sequnlock(&rename_lock);
 
 		if (error)
 			goto out;
 
 		/* Unreachable from current root */
-		if (!path_equal(&tmp, &root)) {
+		if (cwd[0] != '/') {
 			error = prepend_unreachable(&cwd, &buflen);
 			if (error)
 				goto out;
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 05d6b0e78c95..d5a128746974 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -427,7 +427,7 @@ EXPORT_SYMBOL(mangle_path);
  * return the absolute path of 'path', as represented by the
  * dentry / mnt pair in the path parameter.
  */
-int seq_path(struct seq_file *m, struct path *path, char *esc)
+int seq_path(struct seq_file *m, const struct path *path, char *esc)
 {
 	char *buf;
 	size_t size = seq_get_buf(m, &buf);
@@ -449,10 +449,8 @@ EXPORT_SYMBOL(seq_path);
 
 /*
  * Same as seq_path, but relative to supplied root.
- *
- * root may be changed, see __d_path().
  */
-int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
+int seq_path_root(struct seq_file *m, const struct path *path, const struct path *root,
 		  char *esc)
 {
 	char *buf;
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 4df926199369..581feaba8260 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -339,7 +339,7 @@ extern int d_validate(struct dentry *, struct dentry *);
  */
 extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
 
-extern char *__d_path(const struct path *path, struct path *root, char *, int);
+extern char *__d_path(const struct path *path, const struct path *root, char *, int);
 extern char *d_path(const struct path *, char *, int);
 extern char *d_path_with_unreachable(const struct path *, char *, int);
 extern char *dentry_path_raw(struct dentry *, char *, int);
diff --git a/include/linux/seq_file.h b/include/linux/seq_file.h
index 0b69a4684216..0b0a015c553a 100644
--- a/include/linux/seq_file.h
+++ b/include/linux/seq_file.h
@@ -86,9 +86,9 @@ int seq_write(struct seq_file *seq, const void *data, size_t len);
 
 __printf(2, 3) int seq_printf(struct seq_file *, const char *, ...);
 
-int seq_path(struct seq_file *, struct path *, char *);
+int seq_path(struct seq_file *, const struct path *, char *);
 int seq_dentry(struct seq_file *, struct dentry *, char *);
-int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
+int seq_path_root(struct seq_file *m, const struct path *path, const struct path *root,
 		  char *esc);
 int seq_bitmap(struct seq_file *m, const unsigned long *bits,
 				   unsigned int nr_bits);

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

[John Johansen]

On 12/06/2011 03:45 PM, Linus Torvalds wrote:
> On Tue, Dec 6, 2011 at 3:12 PM, John Johansen
> <john.johansen@canonical.com> wrote:
>>
>> What we want to know is if we missed the supplied root, so that we don't
>> mediate off of that path.  And it would be nice to get a partial path for
>> the purposes of logging, so that there is some guidance on how to update
>> policy.
> 
> 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 '/'.
> 
> Something kind of like this (this does *not* change apparmor or tomoyo
> - I didn't even look at those uses).
> 
yes this would give apparmor what it needs.  We would still have to break
one chroot case, until our labeling is properly updated.  But its insane
to plan the api around that case as it is hardly used and a proper solution
for that case is in the works.

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

[Al Viro]

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?

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

[Al Viro]

On Wed, Dec 07, 2011 at 12:16:43AM +0000, Al Viro wrote:
> 	__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?

IOW, I mean this:

diff --git a/fs/dcache.c b/fs/dcache.c
index 10ba92d..8ee05a0 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2439,7 +2439,8 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
 /**
  * prepend_path - Prepend path string to a buffer
  * @path: the dentry/vfsmount to report
- * @root: root vfsmnt/dentry (may be modified by this function)
+ * @root: root vfsmnt/dentry
+ * @ancestor: the fatherless thing we'd walked up to if we missed root
  * @buffer: pointer to the end of the buffer
  * @buflen: pointer to buffer length
  *
@@ -2448,7 +2449,9 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
  * If path is not reachable from the supplied root, then the value of
  * root is changed (without modifying refcounts).
  */
-static int prepend_path(const struct path *path, struct path *root,
+static int prepend_path(const struct path *path,
+			const struct path *root,
+			struct path *ancestor,
 			char **buffer, int *buflen)
 {
 	struct dentry *dentry = path->dentry;
@@ -2483,10 +2486,10 @@ static int prepend_path(const struct path *path, struct path *root,
 		dentry = parent;
 	}
 
-out:
 	if (!error && !slash)
 		error = prepend(buffer, buflen, "/", 1);
 
+out:
 	br_read_unlock(vfsmount_lock);
 	return error;
 
@@ -2500,15 +2503,21 @@ global_root:
 		WARN(1, "Root dentry has weird name <%.*s>\n",
 		     (int) dentry->d_name.len, dentry->d_name.name);
 	}
-	root->mnt = vfsmnt;
-	root->dentry = dentry;
+	if (ancestor) {
+		ancestor->mnt = mntget(vfsmnt);
+		ancestor->dentry = dget(dentry);
+	}
+	if (!slash)
+		error = prepend(buffer, buflen, "/", 1);
+	if (!error)
+		error = 1;
 	goto out;
 }
 
 /**
  * __d_path - return the path of a dentry
  * @path: the dentry/vfsmount to report
- * @root: root vfsmnt/dentry (may be modified by this function)
+ * @root: root vfsmnt/dentry
  * @buf: buffer to return value in
  * @buflen: buffer length
  *
@@ -2519,10 +2528,10 @@ global_root:
  *
  * "buflen" should be positive.
  *
- * If path is not reachable from the supplied root, then the value of
- * root is changed (without modifying refcounts).
+ * If the path is not reachable from the supplied root, return %NULL.
  */
-char *__d_path(const struct path *path, struct path *root,
+char *__d_path(const struct path *path,
+	       const struct path *root,
 	       char *buf, int buflen)
 {
 	char *res = buf + buflen;
@@ -2530,10 +2539,30 @@ char *__d_path(const struct path *path, struct path *root,
 
 	prepend(&res, &buflen, "\0", 1);
 	write_seqlock(&rename_lock);
-	error = prepend_path(path, root, &res, &buflen);
+	error = prepend_path(path, root, NULL, &res, &buflen);
 	write_sequnlock(&rename_lock);
 
-	if (error)
+	if (error < 0)
+		return ERR_PTR(error);
+	if (error > 0)
+		return NULL;
+	return res;
+}
+
+char *d_absolute_path(const struct path *path,
+	       struct path *ancestor,
+	       char *buf, int buflen)
+{
+	struct path root = {};
+	char *res = buf + buflen;
+	int error;
+
+	prepend(&res, &buflen, "\0", 1);
+	write_seqlock(&rename_lock);
+	error = prepend_path(path, &root, ancestor, &res, &buflen);
+	write_sequnlock(&rename_lock);
+
+	if (error < 0)
 		return ERR_PTR(error);
 	return res;
 }
@@ -2541,8 +2570,9 @@ char *__d_path(const struct path *path, struct path *root,
 /*
  * same as __d_path but appends "(deleted)" for unlinked files.
  */
-static int path_with_deleted(const struct path *path, struct path *root,
-				 char **buf, int *buflen)
+static int path_with_deleted(const struct path *path,
+			     const struct path *root,
+			     char **buf, int *buflen)
 {
 	prepend(buf, buflen, "\0", 1);
 	if (d_unlinked(path->dentry)) {
@@ -2551,7 +2581,7 @@ static int path_with_deleted(const struct path *path, struct path *root,
 			return error;
 	}
 
-	return prepend_path(path, root, buf, buflen);
+	return prepend_path(path, root, NULL, buf, buflen);
 }
 
 static int prepend_unreachable(char **buffer, int *buflen)
@@ -2579,7 +2609,6 @@ char *d_path(const struct path *path, char *buf, int buflen)
 {
 	char *res = buf + buflen;
 	struct path root;
-	struct path tmp;
 	int error;
 
 	/*
@@ -2594,9 +2623,8 @@ char *d_path(const struct path *path, char *buf, int buflen)
 
 	get_fs_root(current->fs, &root);
 	write_seqlock(&rename_lock);
-	tmp = root;
-	error = path_with_deleted(path, &tmp, &res, &buflen);
-	if (error)
+	error = path_with_deleted(path, &root, &res, &buflen);
+	if (error < 0)
 		res = ERR_PTR(error);
 	write_sequnlock(&rename_lock);
 	path_put(&root);
@@ -2617,7 +2645,6 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
 {
 	char *res = buf + buflen;
 	struct path root;
-	struct path tmp;
 	int error;
 
 	if (path->dentry->d_op && path->dentry->d_op->d_dname)
@@ -2625,9 +2652,8 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
 
 	get_fs_root(current->fs, &root);
 	write_seqlock(&rename_lock);
-	tmp = root;
-	error = path_with_deleted(path, &tmp, &res, &buflen);
-	if (!error && !path_equal(&tmp, &root))
+	error = path_with_deleted(path, &root, &res, &buflen);
+	if (error > 0)
 		error = prepend_unreachable(&res, &buflen);
 	write_sequnlock(&rename_lock);
 	path_put(&root);
@@ -2758,19 +2784,18 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
 	write_seqlock(&rename_lock);
 	if (!d_unlinked(pwd.dentry)) {
 		unsigned long len;
-		struct path tmp = root;
 		char *cwd = page + PAGE_SIZE;
 		int buflen = PAGE_SIZE;
 
 		prepend(&cwd, &buflen, "\0", 1);
-		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
+		error = prepend_path(&pwd, &root, NULL, &cwd, &buflen);
 		write_sequnlock(&rename_lock);
 
-		if (error)
+		if (error < 0)
 			goto out;
 
 		/* Unreachable from current root */
-		if (!path_equal(&tmp, &root)) {
+		if (error > 0) {
 			error = prepend_unreachable(&cwd, &buflen);
 			if (error)
 				goto out;
diff --git a/fs/namespace.c b/fs/namespace.c
index 6d3a196..cfc6d44 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1048,15 +1048,12 @@ static int show_mountinfo(struct seq_file *m, void *v)
 	if (err)
 		goto out;
 	seq_putc(m, ' ');
-	seq_path_root(m, &mnt_path, &root, " \t\n\\");
-	if (root.mnt != p->root.mnt || root.dentry != p->root.dentry) {
-		/*
-		 * Mountpoint is outside root, discard that one.  Ugly,
-		 * but less so than trying to do that in iterator in a
-		 * race-free way (due to renames).
-		 */
-		return SEQ_SKIP;
-	}
+
+	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
+	err = seq_path_root(m, &mnt_path, &root, " \t\n\\");
+	if (err)
+		goto out;
+
 	seq_puts(m, mnt->mnt_flags & MNT_READONLY ? " ro" : " rw");
 	show_mnt_opts(m, mnt);
 
@@ -2776,3 +2773,8 @@ void kern_unmount(struct vfsmount *mnt)
 	}
 }
 EXPORT_SYMBOL(kern_unmount);
+
+bool our_mnt(struct vfsmount *mnt)
+{
+	return check_mnt(mnt);
+}
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 05d6b0e..dba43c3 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -449,8 +449,6 @@ EXPORT_SYMBOL(seq_path);
 
 /*
  * Same as seq_path, but relative to supplied root.
- *
- * root may be changed, see __d_path().
  */
 int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 		  char *esc)
@@ -463,6 +461,8 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 		char *p;
 
 		p = __d_path(path, root, buf, size);
+		if (!p)
+			return SEQ_SKIP;
 		res = PTR_ERR(p);
 		if (!IS_ERR(p)) {
 			char *end = mangle_path(buf, p, esc);
@@ -474,7 +474,7 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 	}
 	seq_commit(m, res);
 
-	return res < 0 ? res : 0;
+	return res < 0 && res != -ENAMETOOLONG ? res : 0;
 }
 
 /*
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 4df9261..2c32199 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -339,7 +339,8 @@ extern int d_validate(struct dentry *, struct dentry *);
  */
 extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
 
-extern char *__d_path(const struct path *path, struct path *root, char *, int);
+extern char *__d_path(const struct path *, const struct path *, char *, int);
+extern char *d_absolute_path(const struct path *, struct path *, char *, int);
 extern char *d_path(const struct path *, char *, int);
 extern char *d_path_with_unreachable(const struct path *, char *, int);
 extern char *dentry_path_raw(struct dentry *, char *, int);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e313022..019dc55 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1942,6 +1942,7 @@ extern int fd_statfs(int, struct kstatfs *);
 extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
+extern bool our_mnt(struct vfsmount *mnt);
 
 extern int current_umask(void);
 
diff --git a/security/apparmor/path.c b/security/apparmor/path.c
index 36cc0cc..044d2a3 100644
--- a/security/apparmor/path.c
+++ b/security/apparmor/path.c
@@ -57,23 +57,27 @@ static int prepend(char **buffer, int buflen, const char *str, int namelen)
 static int d_namespace_path(struct path *path, char *buf, int buflen,
 			    char **name, int flags)
 {
-	struct path root, tmp;
+	struct path ancestor = {};
 	char *res;
-	int connected, error = 0;
+	int error = 0;
+	int connected = 1;
 
-	/* Get the root we want to resolve too, released below */
+	/* resolve paths relative to chroot?*/
 	if (flags & PATH_CHROOT_REL) {
-		/* resolve paths relative to chroot */
+		struct path root = {};
 		get_fs_root(current->fs, &root);
-	} else {
-		/* resolve paths relative to namespace */
-		root.mnt = current->nsproxy->mnt_ns->root;
-		root.dentry = root.mnt->mnt_root;
-		path_get(&root);
+		res = __d_path(path, &root, buf, buflen);
+		if (res && !IS_ERR(res)) {
+			/* everything's fine */
+			*name = res;
+			path_put(&root);
+			goto ok;
+		}
+		path_put(&root);
+		connected = 0;
 	}
 
-	tmp = root;
-	res = __d_path(path, &tmp, buf, buflen);
+	res = d_absolute_path(path, &ancestor, buf, buflen);
 
 	*name = res;
 	/* handle error conditions - and still allow a partial path to
@@ -84,7 +88,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 		*name = buf;
 		goto out;
 	}
+	if (!our_mnt(ancestor.mnt))
+		connected = 0;
 
+ok:
 	/* Handle two cases:
 	 * 1. A deleted dentry && profile is not allowing mediation of deleted
 	 * 2. On some filesystems, newly allocated dentries appear to the
@@ -97,10 +104,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 			goto out;
 	}
 
-	/* Determine if the path is connected to the expected root */
-	connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;
-
-	/* If the path is not connected,
+	/* If the path is not connected to the expected root,
 	 * check if it is a sysctl and handle specially else remove any
 	 * leading / that __d_path may have returned.
 	 * Unless
@@ -113,7 +117,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 	 */
 	if (!connected) {
 		/* is the disconnect path a sysctl? */
-		if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
+		if (ancestor.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
 		    strncmp(*name, "/sys/", 5) == 0) {
 			/* TODO: convert over to using a per namespace
 			 * control instead of hard coded /proc
@@ -121,8 +125,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 			error = prepend(name, *name - buf, "/proc", 5);
 		} else if (!(flags & PATH_CONNECT_PATH) &&
 			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
-			     (tmp.mnt == current->nsproxy->mnt_ns->root &&
-			      tmp.dentry == tmp.mnt->mnt_root))) {
+			     our_mnt(ancestor.mnt))) {
 			/* disconnected path, don't return pathname starting
 			 * with '/'
 			 */
@@ -133,7 +136,8 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 	}
 
 out:
-	path_put(&root);
+	if (ancestor.mnt)
+		path_put(&ancestor);
 
 	return error;
 }
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 738bbdf..19a6c23 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -101,16 +101,19 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
 {
 	char *pos = ERR_PTR(-ENOMEM);
 	if (buflen >= 256) {
-		struct path ns_root = { };
+		struct path ancestor = { };
 		/* go to whatever namespace root we are under */
-		pos = __d_path(path, &ns_root, buffer, buflen - 1);
-		if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
+		pos = d_absolute_path(path, &ancestor, buffer, buflen - 1);
+		if (IS_ERR(pos))
+			return pos;
+		if (*pos == '/' && pos[1]) {
 			struct inode *inode = path->dentry->d_inode;
 			if (inode && S_ISDIR(inode->i_mode)) {
 				buffer[buflen - 2] = '/';
 				buffer[buflen - 1] = '\0';
 			}
 		}
+		path_put(&ancestor);
 	}
 	return pos;
 }

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 4:39 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> IOW, I mean this:

Yeah, no.

> +       if (ancestor) {
> +               ancestor->mnt = mntget(vfsmnt);
> +               ancestor->dentry = dget(dentry);
> +       }

This part is still just pure and utter sh*t.

You have not explained why that information is *ever* valid. And I
claim it isn't.

We have a bug in our current __d_path(). And I claim that the
underlying cause of the bug is the crazy "let's return this
nonsensical and idiotic information that cannot possibly make sense to
anybody".

We shouldn't have done that in the first place. And we certainly
shouldn't *continue* doing that.

              Linus

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

[Al Viro]

On Tue, Dec 06, 2011 at 04:42:00PM -0800, Linus Torvalds wrote:

> This part is still just pure and utter sh*t.
> 
> You have not explained why that information is *ever* valid. And I
> claim it isn't.
> 
> We have a bug in our current __d_path(). And I claim that the
> underlying cause of the bug is the crazy "let's return this
> nonsensical and idiotic information that cannot possibly make sense to
> anybody".
> 
> We shouldn't have done that in the first place. And we certainly
> shouldn't *continue* doing that.

Sigh...  This is what it boils down to: there are 3 very different cases -
we'd walked to a global root, we'd raced with umount and we are someplace
never mounted at all.  Case 1 is fine; if apparmor cares whose namespace
it is, it can bloody well check path->mnt itself.  Case 2 is one where
I think that returning pathname does more damage than good; it's really
random in that case and returning NULL is the best thing we can do.
So far, so good, and we don't need to return *any* references to vfsmounts.

Unfortunately, there's also case 3.  Internal vfsmounts.  And that's where
it hits the fan.  Oh, wait...

Guys, I think I know how to deal with that crap.  We *CAN* recognize
internal vfsmounts just fine.  It's right there in ->mnt_flags.  And
in that case bothering with __d_path() and correcting it post-factum is
just plain wrong.

So let's add d_absolute_path(path, buf, buflen).  Having it check that
we'd walked to something mounted.  And returning NULL otherwise.  _Never_
mangle the pathname; replace that procfs weirdness in apparmor with
"Is our path on internal vfsmount?  If so, use dentry_path() on dentry
part and slap /proc/ in front if it was procfs" and that's it.

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

[Al Viro]

On Wed, Dec 07, 2011 at 01:10:47AM +0000, Al Viro wrote:

> So let's add d_absolute_path(path, buf, buflen).  Having it check that
> we'd walked to something mounted.  And returning NULL otherwise.  _Never_

Turns out that returning ERR_PTR(-EINVAL) is more convenient.
All right, here's a variant that does *NOT* return any vfsmount/dentry
pointers at all.  And it does best-sane-effort wrt returning the pathname;
i.e. if it *is* something outside of chroot, that absolute pathname is
what we'll get.

diff --git a/fs/dcache.c b/fs/dcache.c
index 10ba92d..89509b5 100644
--- a/fs/dcache.c
+++ b/fs/dcache.c
@@ -2439,16 +2439,14 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
 /**
  * prepend_path - Prepend path string to a buffer
  * @path: the dentry/vfsmount to report
- * @root: root vfsmnt/dentry (may be modified by this function)
+ * @root: root vfsmnt/dentry
  * @buffer: pointer to the end of the buffer
  * @buflen: pointer to buffer length
  *
  * Caller holds the rename_lock.
- *
- * If path is not reachable from the supplied root, then the value of
- * root is changed (without modifying refcounts).
  */
-static int prepend_path(const struct path *path, struct path *root,
+static int prepend_path(const struct path *path,
+			const struct path *root,
 			char **buffer, int *buflen)
 {
 	struct dentry *dentry = path->dentry;
@@ -2483,10 +2481,10 @@ static int prepend_path(const struct path *path, struct path *root,
 		dentry = parent;
 	}
 
-out:
 	if (!error && !slash)
 		error = prepend(buffer, buflen, "/", 1);
 
+out:
 	br_read_unlock(vfsmount_lock);
 	return error;
 
@@ -2500,15 +2498,17 @@ global_root:
 		WARN(1, "Root dentry has weird name <%.*s>\n",
 		     (int) dentry->d_name.len, dentry->d_name.name);
 	}
-	root->mnt = vfsmnt;
-	root->dentry = dentry;
+	if (!slash)
+		error = prepend(buffer, buflen, "/", 1);
+	if (!error)
+		error = vfsmnt->mnt_ns ? 1 : 2;
 	goto out;
 }
 
 /**
  * __d_path - return the path of a dentry
  * @path: the dentry/vfsmount to report
- * @root: root vfsmnt/dentry (may be modified by this function)
+ * @root: root vfsmnt/dentry
  * @buf: buffer to return value in
  * @buflen: buffer length
  *
@@ -2519,10 +2519,10 @@ global_root:
  *
  * "buflen" should be positive.
  *
- * If path is not reachable from the supplied root, then the value of
- * root is changed (without modifying refcounts).
+ * If the path is not reachable from the supplied root, return %NULL.
  */
-char *__d_path(const struct path *path, struct path *root,
+char *__d_path(const struct path *path,
+	       const struct path *root,
 	       char *buf, int buflen)
 {
 	char *res = buf + buflen;
@@ -2533,7 +2533,28 @@ char *__d_path(const struct path *path, struct path *root,
 	error = prepend_path(path, root, &res, &buflen);
 	write_sequnlock(&rename_lock);
 
-	if (error)
+	if (error < 0)
+		return ERR_PTR(error);
+	if (error > 0)
+		return NULL;
+	return res;
+}
+
+char *d_absolute_path(const struct path *path,
+	       char *buf, int buflen)
+{
+	struct path root = {};
+	char *res = buf + buflen;
+	int error;
+
+	prepend(&res, &buflen, "\0", 1);
+	write_seqlock(&rename_lock);
+	error = prepend_path(path, &root, &res, &buflen);
+	write_sequnlock(&rename_lock);
+
+	if (error > 1)
+		error = -EINVAL;
+	if (error < 0)
 		return ERR_PTR(error);
 	return res;
 }
@@ -2541,8 +2562,9 @@ char *__d_path(const struct path *path, struct path *root,
 /*
  * same as __d_path but appends "(deleted)" for unlinked files.
  */
-static int path_with_deleted(const struct path *path, struct path *root,
-				 char **buf, int *buflen)
+static int path_with_deleted(const struct path *path,
+			     const struct path *root,
+			     char **buf, int *buflen)
 {
 	prepend(buf, buflen, "\0", 1);
 	if (d_unlinked(path->dentry)) {
@@ -2579,7 +2601,6 @@ char *d_path(const struct path *path, char *buf, int buflen)
 {
 	char *res = buf + buflen;
 	struct path root;
-	struct path tmp;
 	int error;
 
 	/*
@@ -2594,9 +2615,8 @@ char *d_path(const struct path *path, char *buf, int buflen)
 
 	get_fs_root(current->fs, &root);
 	write_seqlock(&rename_lock);
-	tmp = root;
-	error = path_with_deleted(path, &tmp, &res, &buflen);
-	if (error)
+	error = path_with_deleted(path, &root, &res, &buflen);
+	if (error < 0)
 		res = ERR_PTR(error);
 	write_sequnlock(&rename_lock);
 	path_put(&root);
@@ -2617,7 +2637,6 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
 {
 	char *res = buf + buflen;
 	struct path root;
-	struct path tmp;
 	int error;
 
 	if (path->dentry->d_op && path->dentry->d_op->d_dname)
@@ -2625,9 +2644,8 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
 
 	get_fs_root(current->fs, &root);
 	write_seqlock(&rename_lock);
-	tmp = root;
-	error = path_with_deleted(path, &tmp, &res, &buflen);
-	if (!error && !path_equal(&tmp, &root))
+	error = path_with_deleted(path, &root, &res, &buflen);
+	if (error > 0)
 		error = prepend_unreachable(&res, &buflen);
 	write_sequnlock(&rename_lock);
 	path_put(&root);
@@ -2758,19 +2776,18 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
 	write_seqlock(&rename_lock);
 	if (!d_unlinked(pwd.dentry)) {
 		unsigned long len;
-		struct path tmp = root;
 		char *cwd = page + PAGE_SIZE;
 		int buflen = PAGE_SIZE;
 
 		prepend(&cwd, &buflen, "\0", 1);
-		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
+		error = prepend_path(&pwd, &root, &cwd, &buflen);
 		write_sequnlock(&rename_lock);
 
-		if (error)
+		if (error < 0)
 			goto out;
 
 		/* Unreachable from current root */
-		if (!path_equal(&tmp, &root)) {
+		if (error > 0) {
 			error = prepend_unreachable(&cwd, &buflen);
 			if (error)
 				goto out;
diff --git a/fs/namespace.c b/fs/namespace.c
index 6d3a196..cfc6d44 100644
--- a/fs/namespace.c
+++ b/fs/namespace.c
@@ -1048,15 +1048,12 @@ static int show_mountinfo(struct seq_file *m, void *v)
 	if (err)
 		goto out;
 	seq_putc(m, ' ');
-	seq_path_root(m, &mnt_path, &root, " \t\n\\");
-	if (root.mnt != p->root.mnt || root.dentry != p->root.dentry) {
-		/*
-		 * Mountpoint is outside root, discard that one.  Ugly,
-		 * but less so than trying to do that in iterator in a
-		 * race-free way (due to renames).
-		 */
-		return SEQ_SKIP;
-	}
+
+	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
+	err = seq_path_root(m, &mnt_path, &root, " \t\n\\");
+	if (err)
+		goto out;
+
 	seq_puts(m, mnt->mnt_flags & MNT_READONLY ? " ro" : " rw");
 	show_mnt_opts(m, mnt);
 
@@ -2776,3 +2773,8 @@ void kern_unmount(struct vfsmount *mnt)
 	}
 }
 EXPORT_SYMBOL(kern_unmount);
+
+bool our_mnt(struct vfsmount *mnt)
+{
+	return check_mnt(mnt);
+}
diff --git a/fs/seq_file.c b/fs/seq_file.c
index 05d6b0e..dba43c3 100644
--- a/fs/seq_file.c
+++ b/fs/seq_file.c
@@ -449,8 +449,6 @@ EXPORT_SYMBOL(seq_path);
 
 /*
  * Same as seq_path, but relative to supplied root.
- *
- * root may be changed, see __d_path().
  */
 int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 		  char *esc)
@@ -463,6 +461,8 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 		char *p;
 
 		p = __d_path(path, root, buf, size);
+		if (!p)
+			return SEQ_SKIP;
 		res = PTR_ERR(p);
 		if (!IS_ERR(p)) {
 			char *end = mangle_path(buf, p, esc);
@@ -474,7 +474,7 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
 	}
 	seq_commit(m, res);
 
-	return res < 0 ? res : 0;
+	return res < 0 && res != -ENAMETOOLONG ? res : 0;
 }
 
 /*
diff --git a/include/linux/dcache.h b/include/linux/dcache.h
index 4df9261..ed9f74f 100644
--- a/include/linux/dcache.h
+++ b/include/linux/dcache.h
@@ -339,7 +339,8 @@ extern int d_validate(struct dentry *, struct dentry *);
  */
 extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
 
-extern char *__d_path(const struct path *path, struct path *root, char *, int);
+extern char *__d_path(const struct path *, const struct path *, char *, int);
+extern char *d_absolute_path(const struct path *, char *, int);
 extern char *d_path(const struct path *, char *, int);
 extern char *d_path_with_unreachable(const struct path *, char *, int);
 extern char *dentry_path_raw(struct dentry *, char *, int);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index e313022..019dc55 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -1942,6 +1942,7 @@ extern int fd_statfs(int, struct kstatfs *);
 extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
 extern int freeze_super(struct super_block *super);
 extern int thaw_super(struct super_block *super);
+extern bool our_mnt(struct vfsmount *mnt);
 
 extern int current_umask(void);
 
diff --git a/security/apparmor/path.c b/security/apparmor/path.c
index 36cc0cc..b566eba 100644
--- a/security/apparmor/path.c
+++ b/security/apparmor/path.c
@@ -57,23 +57,44 @@ static int prepend(char **buffer, int buflen, const char *str, int namelen)
 static int d_namespace_path(struct path *path, char *buf, int buflen,
 			    char **name, int flags)
 {
-	struct path root, tmp;
 	char *res;
-	int connected, error = 0;
+	int error = 0;
+	int connected = 1;
+
+	if (path->mnt->mnt_flags & MNT_INTERNAL) {
+		/* it's not mounted anywhere */
+		res = dentry_path(path->dentry, buf, buflen);
+		*name = res;
+		if (IS_ERR(res)) {
+			*name = buf;
+			return PTR_ERR(res);
+		}
+		if (path->dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
+		    strncmp(*name, "/sys/", 5) == 0) {
+			/* TODO: convert over to using a per namespace
+			 * control instead of hard coded /proc
+			 */
+			return prepend(name, *name - buf, "/proc", 5);
+		}
+		return 0;
+	}
 
-	/* Get the root we want to resolve too, released below */
+	/* resolve paths relative to chroot?*/
 	if (flags & PATH_CHROOT_REL) {
-		/* resolve paths relative to chroot */
+		struct path root;
 		get_fs_root(current->fs, &root);
-	} else {
-		/* resolve paths relative to namespace */
-		root.mnt = current->nsproxy->mnt_ns->root;
-		root.dentry = root.mnt->mnt_root;
-		path_get(&root);
+		res = __d_path(path, &root, buf, buflen);
+		if (res && !IS_ERR(res)) {
+			/* everything's fine */
+			*name = res;
+			path_put(&root);
+			goto ok;
+		}
+		path_put(&root);
+		connected = 0;
 	}
 
-	tmp = root;
-	res = __d_path(path, &tmp, buf, buflen);
+	res = d_absolute_path(path, buf, buflen);
 
 	*name = res;
 	/* handle error conditions - and still allow a partial path to
@@ -84,7 +105,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 		*name = buf;
 		goto out;
 	}
+	if (!our_mnt(path->mnt))
+		connected = 0;
 
+ok:
 	/* Handle two cases:
 	 * 1. A deleted dentry && profile is not allowing mediation of deleted
 	 * 2. On some filesystems, newly allocated dentries appear to the
@@ -97,10 +121,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 			goto out;
 	}
 
-	/* Determine if the path is connected to the expected root */
-	connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;
-
-	/* If the path is not connected,
+	/* If the path is not connected to the expected root,
 	 * check if it is a sysctl and handle specially else remove any
 	 * leading / that __d_path may have returned.
 	 * Unless
@@ -112,17 +133,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 	 *     namespace root.
 	 */
 	if (!connected) {
-		/* is the disconnect path a sysctl? */
-		if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
-		    strncmp(*name, "/sys/", 5) == 0) {
-			/* TODO: convert over to using a per namespace
-			 * control instead of hard coded /proc
-			 */
-			error = prepend(name, *name - buf, "/proc", 5);
-		} else if (!(flags & PATH_CONNECT_PATH) &&
+		if (!(flags & PATH_CONNECT_PATH) &&
 			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
-			     (tmp.mnt == current->nsproxy->mnt_ns->root &&
-			      tmp.dentry == tmp.mnt->mnt_root))) {
+			     our_mnt(path->mnt))) {
 			/* disconnected path, don't return pathname starting
 			 * with '/'
 			 */
@@ -133,8 +146,6 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
 	}
 
 out:
-	path_put(&root);
-
 	return error;
 }
 
diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
index 738bbdf..36fa7c9 100644
--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -101,9 +101,8 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
 {
 	char *pos = ERR_PTR(-ENOMEM);
 	if (buflen >= 256) {
-		struct path ns_root = { };
 		/* go to whatever namespace root we are under */
-		pos = __d_path(path, &ns_root, buffer, buflen - 1);
+		pos = d_absolute_path(path, buffer, buflen - 1);
 		if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
 			struct inode *inode = path->dentry->d_inode;
 			if (inode && S_ISDIR(inode->i_mode)) {

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

[Al Viro]

On Wed, Dec 07, 2011 at 01:37:20AM +0000, Al Viro wrote:
> +	if (path->mnt->mnt_flags & MNT_INTERNAL) {
> +		/* it's not mounted anywhere */
> +		res = dentry_path(path->dentry, buf, buflen);
> +		*name = res;
> +		if (IS_ERR(res)) {
> +			*name = buf;
> +			return PTR_ERR(res);
> +		}
> +		if (path->dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
> +		    strncmp(*name, "/sys/", 5) == 0) {
> +			/* TODO: convert over to using a per namespace
> +			 * control instead of hard coded /proc
> +			 */
> +			return prepend(name, *name - buf, "/proc", 5);


Incidentally, why only /proc/sys?  For one thing, we won't be accessing
anything else on internal vfsmount of proc, so strncmp() part looks
wrong; for another, if some code starts doing that, why would acting
as if it was on normally mounted /proc be the wrong thing?  John?

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 5:37 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Turns out that returning ERR_PTR(-EINVAL) is more convenient.
> All right, here's a variant that does *NOT* return any vfsmount/dentry
> pointers at all.  And it does best-sane-effort wrt returning the pathname;
> i.e. if it *is* something outside of chroot, that absolute pathname is
> what we'll get.

So I don't mind this patch, but I think the d_absolute_path() thing is
actually being a bit too nice to the two users of that new function.

Both users of that are just broken. And I think they should be fixed,
rather than pandered to. Both of them would be better off just
considering the "out of my mountspace" to be an error, rather than
using some random 'absolute' pathname (where "error" might obviously
mean "I will use another strategy entirely for this case").

Afaik, you really are trying to make crazy code continue to work in
ways that I'm not convinced is really required. Now, maybe it's a good
idea for this stage in the -rc process, but my gut feel is that we
could simply have broken it too. JJ seems willing to fix things up for
AppArmor, and I don't know of any big distro that uses Tomoyo, so...

But I don't actually have any real hatred of this patch. It definitely
falls into my acceptable range.

                      Linus

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

[Al Viro]

On Tue, Dec 06, 2011 at 06:21:55PM -0800, Linus Torvalds wrote:

> Afaik, you really are trying to make crazy code continue to work in
> ways that I'm not convinced is really required. Now, maybe it's a good
> idea for this stage in the -rc process, but my gut feel is that we
> could simply have broken it too. JJ seems willing to fix things up for
> AppArmor, and I don't know of any big distro that uses Tomoyo, so...

*snort*

As far as I'm concerned, adding

# misnomer/marketing/lie - that's LSM, actually
config SECURITY
	depends on INSANITY || CRETINOUS_MANAGEMENT || SHITTY_GOV_REQS

would be a good thing; git rm on the whole bunch would be even better
(yes, both the pathname-based horrors and selinux).  Not feasible,
unfortunately - Linux S&M is there to stay.  More's the pity...

Anyway, we are at -rc4 and we have an oopsable race that needs to be fixed.
So let's fix it and then let apparmor and tomoyo folks deal with their
ugly stuff.  It's also 3.0.x / 3.1.x fodder; the same goes for 2.6.3[6-9].x
but there tomoyo part will need backporting (they had changes circa 3.0-rc3
in that area).

I don't think that d_absolute_path() is a bad thing per se - it's *definitely*
bad for making security decisions, but you've mentioned debugging yourself.
Better that than kludges like struct path root = {} and passing &root around,
especially if random debugging code starts using it...

I'm waiting an ACK/NAK for apparmor bits from John; I think the summary I've
put in the previous mail would do as commit message, so I'll put it into
commit and push in #for-linus; if John ACKs that sucker, pull request time...

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

[John Johansen]

On 12/06/2011 05:37 PM, Al Viro wrote:
> On Wed, Dec 07, 2011 at 01:10:47AM +0000, Al Viro wrote:
> 
>> So let's add d_absolute_path(path, buf, buflen).  Having it check that
>> we'd walked to something mounted.  And returning NULL otherwise.  _Never_
> 
> Turns out that returning ERR_PTR(-EINVAL) is more convenient.
> All right, here's a variant that does *NOT* return any vfsmount/dentry
> pointers at all.  And it does best-sane-effort wrt returning the pathname;
> i.e. if it *is* something outside of chroot, that absolute pathname is
> what we'll get.
> 
I haven't had a chance to build and test yet but this looks good to me.  I do
think splitting the two uses cases makes sense.  I am building a kernel now
and will update when I am done with it.



> diff --git a/fs/dcache.c b/fs/dcache.c
> index 10ba92d..89509b5 100644
> --- a/fs/dcache.c
> +++ b/fs/dcache.c
> @@ -2439,16 +2439,14 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
>  /**
>   * prepend_path - Prepend path string to a buffer
>   * @path: the dentry/vfsmount to report
> - * @root: root vfsmnt/dentry (may be modified by this function)
> + * @root: root vfsmnt/dentry
>   * @buffer: pointer to the end of the buffer
>   * @buflen: pointer to buffer length
>   *
>   * Caller holds the rename_lock.
> - *
> - * If path is not reachable from the supplied root, then the value of
> - * root is changed (without modifying refcounts).
>   */
> -static int prepend_path(const struct path *path, struct path *root,
> +static int prepend_path(const struct path *path,
> +			const struct path *root,
>  			char **buffer, int *buflen)
>  {
>  	struct dentry *dentry = path->dentry;
> @@ -2483,10 +2481,10 @@ static int prepend_path(const struct path *path, struct path *root,
>  		dentry = parent;
>  	}
>  
> -out:
>  	if (!error && !slash)
>  		error = prepend(buffer, buflen, "/", 1);
>  
> +out:
>  	br_read_unlock(vfsmount_lock);
>  	return error;
>  
> @@ -2500,15 +2498,17 @@ global_root:
>  		WARN(1, "Root dentry has weird name <%.*s>\n",
>  		     (int) dentry->d_name.len, dentry->d_name.name);
>  	}
> -	root->mnt = vfsmnt;
> -	root->dentry = dentry;
> +	if (!slash)
> +		error = prepend(buffer, buflen, "/", 1);
> +	if (!error)
> +		error = vfsmnt->mnt_ns ? 1 : 2;
>  	goto out;
>  }
>  
>  /**
>   * __d_path - return the path of a dentry
>   * @path: the dentry/vfsmount to report
> - * @root: root vfsmnt/dentry (may be modified by this function)
> + * @root: root vfsmnt/dentry
>   * @buf: buffer to return value in
>   * @buflen: buffer length
>   *
> @@ -2519,10 +2519,10 @@ global_root:
>   *
>   * "buflen" should be positive.
>   *
> - * If path is not reachable from the supplied root, then the value of
> - * root is changed (without modifying refcounts).
> + * If the path is not reachable from the supplied root, return %NULL.
>   */
> -char *__d_path(const struct path *path, struct path *root,
> +char *__d_path(const struct path *path,
> +	       const struct path *root,
>  	       char *buf, int buflen)
>  {
>  	char *res = buf + buflen;
> @@ -2533,7 +2533,28 @@ char *__d_path(const struct path *path, struct path *root,
>  	error = prepend_path(path, root, &res, &buflen);
>  	write_sequnlock(&rename_lock);
>  
> -	if (error)
> +	if (error < 0)
> +		return ERR_PTR(error);
> +	if (error > 0)
> +		return NULL;
> +	return res;
> +}
> +
> +char *d_absolute_path(const struct path *path,
> +	       char *buf, int buflen)
> +{
> +	struct path root = {};
> +	char *res = buf + buflen;
> +	int error;
> +
> +	prepend(&res, &buflen, "\0", 1);
> +	write_seqlock(&rename_lock);
> +	error = prepend_path(path, &root, &res, &buflen);
> +	write_sequnlock(&rename_lock);
> +
> +	if (error > 1)
> +		error = -EINVAL;
> +	if (error < 0)
>  		return ERR_PTR(error);
>  	return res;
>  }
> @@ -2541,8 +2562,9 @@ char *__d_path(const struct path *path, struct path *root,
>  /*
>   * same as __d_path but appends "(deleted)" for unlinked files.
>   */
> -static int path_with_deleted(const struct path *path, struct path *root,
> -				 char **buf, int *buflen)
> +static int path_with_deleted(const struct path *path,
> +			     const struct path *root,
> +			     char **buf, int *buflen)
>  {
>  	prepend(buf, buflen, "\0", 1);
>  	if (d_unlinked(path->dentry)) {
> @@ -2579,7 +2601,6 @@ char *d_path(const struct path *path, char *buf, int buflen)
>  {
>  	char *res = buf + buflen;
>  	struct path root;
> -	struct path tmp;
>  	int error;
>  
>  	/*
> @@ -2594,9 +2615,8 @@ char *d_path(const struct path *path, char *buf, int buflen)
>  
>  	get_fs_root(current->fs, &root);
>  	write_seqlock(&rename_lock);
> -	tmp = root;
> -	error = path_with_deleted(path, &tmp, &res, &buflen);
> -	if (error)
> +	error = path_with_deleted(path, &root, &res, &buflen);
> +	if (error < 0)
>  		res = ERR_PTR(error);
>  	write_sequnlock(&rename_lock);
>  	path_put(&root);
> @@ -2617,7 +2637,6 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
>  {
>  	char *res = buf + buflen;
>  	struct path root;
> -	struct path tmp;
>  	int error;
>  
>  	if (path->dentry->d_op && path->dentry->d_op->d_dname)
> @@ -2625,9 +2644,8 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
>  
>  	get_fs_root(current->fs, &root);
>  	write_seqlock(&rename_lock);
> -	tmp = root;
> -	error = path_with_deleted(path, &tmp, &res, &buflen);
> -	if (!error && !path_equal(&tmp, &root))
> +	error = path_with_deleted(path, &root, &res, &buflen);
> +	if (error > 0)
>  		error = prepend_unreachable(&res, &buflen);
>  	write_sequnlock(&rename_lock);
>  	path_put(&root);
> @@ -2758,19 +2776,18 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
>  	write_seqlock(&rename_lock);
>  	if (!d_unlinked(pwd.dentry)) {
>  		unsigned long len;
> -		struct path tmp = root;
>  		char *cwd = page + PAGE_SIZE;
>  		int buflen = PAGE_SIZE;
>  
>  		prepend(&cwd, &buflen, "\0", 1);
> -		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
> +		error = prepend_path(&pwd, &root, &cwd, &buflen);
>  		write_sequnlock(&rename_lock);
>  
> -		if (error)
> +		if (error < 0)
>  			goto out;
>  
>  		/* Unreachable from current root */
> -		if (!path_equal(&tmp, &root)) {
> +		if (error > 0) {
>  			error = prepend_unreachable(&cwd, &buflen);
>  			if (error)
>  				goto out;
> diff --git a/fs/namespace.c b/fs/namespace.c
> index 6d3a196..cfc6d44 100644
> --- a/fs/namespace.c
> +++ b/fs/namespace.c
> @@ -1048,15 +1048,12 @@ static int show_mountinfo(struct seq_file *m, void *v)
>  	if (err)
>  		goto out;
>  	seq_putc(m, ' ');
> -	seq_path_root(m, &mnt_path, &root, " \t\n\\");
> -	if (root.mnt != p->root.mnt || root.dentry != p->root.dentry) {
> -		/*
> -		 * Mountpoint is outside root, discard that one.  Ugly,
> -		 * but less so than trying to do that in iterator in a
> -		 * race-free way (due to renames).
> -		 */
> -		return SEQ_SKIP;
> -	}
> +
> +	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
> +	err = seq_path_root(m, &mnt_path, &root, " \t\n\\");
> +	if (err)
> +		goto out;
> +
>  	seq_puts(m, mnt->mnt_flags & MNT_READONLY ? " ro" : " rw");
>  	show_mnt_opts(m, mnt);
>  
> @@ -2776,3 +2773,8 @@ void kern_unmount(struct vfsmount *mnt)
>  	}
>  }
>  EXPORT_SYMBOL(kern_unmount);
> +
> +bool our_mnt(struct vfsmount *mnt)
> +{
> +	return check_mnt(mnt);
> +}
> diff --git a/fs/seq_file.c b/fs/seq_file.c
> index 05d6b0e..dba43c3 100644
> --- a/fs/seq_file.c
> +++ b/fs/seq_file.c
> @@ -449,8 +449,6 @@ EXPORT_SYMBOL(seq_path);
>  
>  /*
>   * Same as seq_path, but relative to supplied root.
> - *
> - * root may be changed, see __d_path().
>   */
>  int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>  		  char *esc)
> @@ -463,6 +461,8 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>  		char *p;
>  
>  		p = __d_path(path, root, buf, size);
> +		if (!p)
> +			return SEQ_SKIP;
>  		res = PTR_ERR(p);
>  		if (!IS_ERR(p)) {
>  			char *end = mangle_path(buf, p, esc);
> @@ -474,7 +474,7 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>  	}
>  	seq_commit(m, res);
>  
> -	return res < 0 ? res : 0;
> +	return res < 0 && res != -ENAMETOOLONG ? res : 0;
>  }
>  
>  /*
> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
> index 4df9261..ed9f74f 100644
> --- a/include/linux/dcache.h
> +++ b/include/linux/dcache.h
> @@ -339,7 +339,8 @@ extern int d_validate(struct dentry *, struct dentry *);
>   */
>  extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
>  
> -extern char *__d_path(const struct path *path, struct path *root, char *, int);
> +extern char *__d_path(const struct path *, const struct path *, char *, int);
> +extern char *d_absolute_path(const struct path *, char *, int);
>  extern char *d_path(const struct path *, char *, int);
>  extern char *d_path_with_unreachable(const struct path *, char *, int);
>  extern char *dentry_path_raw(struct dentry *, char *, int);
> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index e313022..019dc55 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1942,6 +1942,7 @@ extern int fd_statfs(int, struct kstatfs *);
>  extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
>  extern int freeze_super(struct super_block *super);
>  extern int thaw_super(struct super_block *super);
> +extern bool our_mnt(struct vfsmount *mnt);
>  
>  extern int current_umask(void);
>  
> diff --git a/security/apparmor/path.c b/security/apparmor/path.c
> index 36cc0cc..b566eba 100644
> --- a/security/apparmor/path.c
> +++ b/security/apparmor/path.c
> @@ -57,23 +57,44 @@ static int prepend(char **buffer, int buflen, const char *str, int namelen)
>  static int d_namespace_path(struct path *path, char *buf, int buflen,
>  			    char **name, int flags)
>  {
> -	struct path root, tmp;
>  	char *res;
> -	int connected, error = 0;
> +	int error = 0;
> +	int connected = 1;
> +
> +	if (path->mnt->mnt_flags & MNT_INTERNAL) {
> +		/* it's not mounted anywhere */
> +		res = dentry_path(path->dentry, buf, buflen);
> +		*name = res;
> +		if (IS_ERR(res)) {
> +			*name = buf;
> +			return PTR_ERR(res);
> +		}
> +		if (path->dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
> +		    strncmp(*name, "/sys/", 5) == 0) {
> +			/* TODO: convert over to using a per namespace
> +			 * control instead of hard coded /proc
> +			 */
> +			return prepend(name, *name - buf, "/proc", 5);
> +		}
> +		return 0;
> +	}
>  
> -	/* Get the root we want to resolve too, released below */
> +	/* resolve paths relative to chroot?*/
>  	if (flags & PATH_CHROOT_REL) {
> -		/* resolve paths relative to chroot */
> +		struct path root;
>  		get_fs_root(current->fs, &root);
> -	} else {
> -		/* resolve paths relative to namespace */
> -		root.mnt = current->nsproxy->mnt_ns->root;
> -		root.dentry = root.mnt->mnt_root;
> -		path_get(&root);
> +		res = __d_path(path, &root, buf, buflen);
> +		if (res && !IS_ERR(res)) {
> +			/* everything's fine */
> +			*name = res;
> +			path_put(&root);
> +			goto ok;
> +		}
> +		path_put(&root);
> +		connected = 0;
>  	}
>  
> -	tmp = root;
> -	res = __d_path(path, &tmp, buf, buflen);
> +	res = d_absolute_path(path, buf, buflen);
>  
>  	*name = res;
>  	/* handle error conditions - and still allow a partial path to
> @@ -84,7 +105,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  		*name = buf;
>  		goto out;
>  	}
> +	if (!our_mnt(path->mnt))
> +		connected = 0;
>  
> +ok:
>  	/* Handle two cases:
>  	 * 1. A deleted dentry && profile is not allowing mediation of deleted
>  	 * 2. On some filesystems, newly allocated dentries appear to the
> @@ -97,10 +121,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  			goto out;
>  	}
>  
> -	/* Determine if the path is connected to the expected root */
> -	connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;
> -
> -	/* If the path is not connected,
> +	/* If the path is not connected to the expected root,
>  	 * check if it is a sysctl and handle specially else remove any
>  	 * leading / that __d_path may have returned.
>  	 * Unless
> @@ -112,17 +133,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  	 *     namespace root.
>  	 */
>  	if (!connected) {
> -		/* is the disconnect path a sysctl? */
> -		if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
> -		    strncmp(*name, "/sys/", 5) == 0) {
> -			/* TODO: convert over to using a per namespace
> -			 * control instead of hard coded /proc
> -			 */
> -			error = prepend(name, *name - buf, "/proc", 5);
> -		} else if (!(flags & PATH_CONNECT_PATH) &&
> +		if (!(flags & PATH_CONNECT_PATH) &&
>  			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
> -			     (tmp.mnt == current->nsproxy->mnt_ns->root &&
> -			      tmp.dentry == tmp.mnt->mnt_root))) {
> +			     our_mnt(path->mnt))) {
>  			/* disconnected path, don't return pathname starting
>  			 * with '/'
>  			 */
> @@ -133,8 +146,6 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>  	}
>  
>  out:
> -	path_put(&root);
> -
>  	return error;
>  }
>  
> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
> index 738bbdf..36fa7c9 100644
> --- a/security/tomoyo/realpath.c
> +++ b/security/tomoyo/realpath.c
> @@ -101,9 +101,8 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
>  {
>  	char *pos = ERR_PTR(-ENOMEM);
>  	if (buflen >= 256) {
> -		struct path ns_root = { };
>  		/* go to whatever namespace root we are under */
> -		pos = __d_path(path, &ns_root, buffer, buflen - 1);
> +		pos = d_absolute_path(path, buffer, buflen - 1);
>  		if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
>  			struct inode *inode = path->dentry->d_inode;
>  			if (inode && S_ISDIR(inode->i_mode)) {

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

[John Johansen]

On 12/06/2011 07:11 PM, John Johansen wrote:
> On 12/06/2011 05:37 PM, Al Viro wrote:
>> On Wed, Dec 07, 2011 at 01:10:47AM +0000, Al Viro wrote:
>>
>>> So let's add d_absolute_path(path, buf, buflen).  Having it check that
>>> we'd walked to something mounted.  And returning NULL otherwise.  _Never_
>>
>> Turns out that returning ERR_PTR(-EINVAL) is more convenient.
>> All right, here's a variant that does *NOT* return any vfsmount/dentry
>> pointers at all.  And it does best-sane-effort wrt returning the pathname;
>> i.e. if it *is* something outside of chroot, that absolute pathname is
>> what we'll get.
>>
> I haven't had a chance to build and test yet but this looks good to me.  I do
> think splitting the two uses cases makes sense.  I am building a kernel now
> and will update when I am done with it.
> 
Okay its good, certainly better than what was there, I will work on cleaning up
the apparmor bits, especially the sysctl mess.

Reviewed-by: John Johansen <john.johansen@canonical.com>

> 
> 
>> diff --git a/fs/dcache.c b/fs/dcache.c
>> index 10ba92d..89509b5 100644
>> --- a/fs/dcache.c
>> +++ b/fs/dcache.c
>> @@ -2439,16 +2439,14 @@ static int prepend_name(char **buffer, int *buflen, struct qstr *name)
>>  /**
>>   * prepend_path - Prepend path string to a buffer
>>   * @path: the dentry/vfsmount to report
>> - * @root: root vfsmnt/dentry (may be modified by this function)
>> + * @root: root vfsmnt/dentry
>>   * @buffer: pointer to the end of the buffer
>>   * @buflen: pointer to buffer length
>>   *
>>   * Caller holds the rename_lock.
>> - *
>> - * If path is not reachable from the supplied root, then the value of
>> - * root is changed (without modifying refcounts).
>>   */
>> -static int prepend_path(const struct path *path, struct path *root,
>> +static int prepend_path(const struct path *path,
>> +			const struct path *root,
>>  			char **buffer, int *buflen)
>>  {
>>  	struct dentry *dentry = path->dentry;
>> @@ -2483,10 +2481,10 @@ static int prepend_path(const struct path *path, struct path *root,
>>  		dentry = parent;
>>  	}
>>  
>> -out:
>>  	if (!error && !slash)
>>  		error = prepend(buffer, buflen, "/", 1);
>>  
>> +out:
>>  	br_read_unlock(vfsmount_lock);
>>  	return error;
>>  
>> @@ -2500,15 +2498,17 @@ global_root:
>>  		WARN(1, "Root dentry has weird name <%.*s>\n",
>>  		     (int) dentry->d_name.len, dentry->d_name.name);
>>  	}
>> -	root->mnt = vfsmnt;
>> -	root->dentry = dentry;
>> +	if (!slash)
>> +		error = prepend(buffer, buflen, "/", 1);
>> +	if (!error)
>> +		error = vfsmnt->mnt_ns ? 1 : 2;
>>  	goto out;
>>  }
>>  
>>  /**
>>   * __d_path - return the path of a dentry
>>   * @path: the dentry/vfsmount to report
>> - * @root: root vfsmnt/dentry (may be modified by this function)
>> + * @root: root vfsmnt/dentry
>>   * @buf: buffer to return value in
>>   * @buflen: buffer length
>>   *
>> @@ -2519,10 +2519,10 @@ global_root:
>>   *
>>   * "buflen" should be positive.
>>   *
>> - * If path is not reachable from the supplied root, then the value of
>> - * root is changed (without modifying refcounts).
>> + * If the path is not reachable from the supplied root, return %NULL.
>>   */
>> -char *__d_path(const struct path *path, struct path *root,
>> +char *__d_path(const struct path *path,
>> +	       const struct path *root,
>>  	       char *buf, int buflen)
>>  {
>>  	char *res = buf + buflen;
>> @@ -2533,7 +2533,28 @@ char *__d_path(const struct path *path, struct path *root,
>>  	error = prepend_path(path, root, &res, &buflen);
>>  	write_sequnlock(&rename_lock);
>>  
>> -	if (error)
>> +	if (error < 0)
>> +		return ERR_PTR(error);
>> +	if (error > 0)
>> +		return NULL;
>> +	return res;
>> +}
>> +
>> +char *d_absolute_path(const struct path *path,
>> +	       char *buf, int buflen)
>> +{
>> +	struct path root = {};
>> +	char *res = buf + buflen;
>> +	int error;
>> +
>> +	prepend(&res, &buflen, "\0", 1);
>> +	write_seqlock(&rename_lock);
>> +	error = prepend_path(path, &root, &res, &buflen);
>> +	write_sequnlock(&rename_lock);
>> +
>> +	if (error > 1)
>> +		error = -EINVAL;
>> +	if (error < 0)
>>  		return ERR_PTR(error);
>>  	return res;
>>  }
>> @@ -2541,8 +2562,9 @@ char *__d_path(const struct path *path, struct path *root,
>>  /*
>>   * same as __d_path but appends "(deleted)" for unlinked files.
>>   */
>> -static int path_with_deleted(const struct path *path, struct path *root,
>> -				 char **buf, int *buflen)
>> +static int path_with_deleted(const struct path *path,
>> +			     const struct path *root,
>> +			     char **buf, int *buflen)
>>  {
>>  	prepend(buf, buflen, "\0", 1);
>>  	if (d_unlinked(path->dentry)) {
>> @@ -2579,7 +2601,6 @@ char *d_path(const struct path *path, char *buf, int buflen)
>>  {
>>  	char *res = buf + buflen;
>>  	struct path root;
>> -	struct path tmp;
>>  	int error;
>>  
>>  	/*
>> @@ -2594,9 +2615,8 @@ char *d_path(const struct path *path, char *buf, int buflen)
>>  
>>  	get_fs_root(current->fs, &root);
>>  	write_seqlock(&rename_lock);
>> -	tmp = root;
>> -	error = path_with_deleted(path, &tmp, &res, &buflen);
>> -	if (error)
>> +	error = path_with_deleted(path, &root, &res, &buflen);
>> +	if (error < 0)
>>  		res = ERR_PTR(error);
>>  	write_sequnlock(&rename_lock);
>>  	path_put(&root);
>> @@ -2617,7 +2637,6 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
>>  {
>>  	char *res = buf + buflen;
>>  	struct path root;
>> -	struct path tmp;
>>  	int error;
>>  
>>  	if (path->dentry->d_op && path->dentry->d_op->d_dname)
>> @@ -2625,9 +2644,8 @@ char *d_path_with_unreachable(const struct path *path, char *buf, int buflen)
>>  
>>  	get_fs_root(current->fs, &root);
>>  	write_seqlock(&rename_lock);
>> -	tmp = root;
>> -	error = path_with_deleted(path, &tmp, &res, &buflen);
>> -	if (!error && !path_equal(&tmp, &root))
>> +	error = path_with_deleted(path, &root, &res, &buflen);
>> +	if (error > 0)
>>  		error = prepend_unreachable(&res, &buflen);
>>  	write_sequnlock(&rename_lock);
>>  	path_put(&root);
>> @@ -2758,19 +2776,18 @@ SYSCALL_DEFINE2(getcwd, char __user *, buf, unsigned long, size)
>>  	write_seqlock(&rename_lock);
>>  	if (!d_unlinked(pwd.dentry)) {
>>  		unsigned long len;
>> -		struct path tmp = root;
>>  		char *cwd = page + PAGE_SIZE;
>>  		int buflen = PAGE_SIZE;
>>  
>>  		prepend(&cwd, &buflen, "\0", 1);
>> -		error = prepend_path(&pwd, &tmp, &cwd, &buflen);
>> +		error = prepend_path(&pwd, &root, &cwd, &buflen);
>>  		write_sequnlock(&rename_lock);
>>  
>> -		if (error)
>> +		if (error < 0)
>>  			goto out;
>>  
>>  		/* Unreachable from current root */
>> -		if (!path_equal(&tmp, &root)) {
>> +		if (error > 0) {
>>  			error = prepend_unreachable(&cwd, &buflen);
>>  			if (error)
>>  				goto out;
>> diff --git a/fs/namespace.c b/fs/namespace.c
>> index 6d3a196..cfc6d44 100644
>> --- a/fs/namespace.c
>> +++ b/fs/namespace.c
>> @@ -1048,15 +1048,12 @@ static int show_mountinfo(struct seq_file *m, void *v)
>>  	if (err)
>>  		goto out;
>>  	seq_putc(m, ' ');
>> -	seq_path_root(m, &mnt_path, &root, " \t\n\\");
>> -	if (root.mnt != p->root.mnt || root.dentry != p->root.dentry) {
>> -		/*
>> -		 * Mountpoint is outside root, discard that one.  Ugly,
>> -		 * but less so than trying to do that in iterator in a
>> -		 * race-free way (due to renames).
>> -		 */
>> -		return SEQ_SKIP;
>> -	}
>> +
>> +	/* mountpoints outside of chroot jail will give SEQ_SKIP on this */
>> +	err = seq_path_root(m, &mnt_path, &root, " \t\n\\");
>> +	if (err)
>> +		goto out;
>> +
>>  	seq_puts(m, mnt->mnt_flags & MNT_READONLY ? " ro" : " rw");
>>  	show_mnt_opts(m, mnt);
>>  
>> @@ -2776,3 +2773,8 @@ void kern_unmount(struct vfsmount *mnt)
>>  	}
>>  }
>>  EXPORT_SYMBOL(kern_unmount);
>> +
>> +bool our_mnt(struct vfsmount *mnt)
>> +{
>> +	return check_mnt(mnt);
>> +}
>> diff --git a/fs/seq_file.c b/fs/seq_file.c
>> index 05d6b0e..dba43c3 100644
>> --- a/fs/seq_file.c
>> +++ b/fs/seq_file.c
>> @@ -449,8 +449,6 @@ EXPORT_SYMBOL(seq_path);
>>  
>>  /*
>>   * Same as seq_path, but relative to supplied root.
>> - *
>> - * root may be changed, see __d_path().
>>   */
>>  int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>>  		  char *esc)
>> @@ -463,6 +461,8 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>>  		char *p;
>>  
>>  		p = __d_path(path, root, buf, size);
>> +		if (!p)
>> +			return SEQ_SKIP;
>>  		res = PTR_ERR(p);
>>  		if (!IS_ERR(p)) {
>>  			char *end = mangle_path(buf, p, esc);
>> @@ -474,7 +474,7 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root,
>>  	}
>>  	seq_commit(m, res);
>>  
>> -	return res < 0 ? res : 0;
>> +	return res < 0 && res != -ENAMETOOLONG ? res : 0;
>>  }
>>  
>>  /*
>> diff --git a/include/linux/dcache.h b/include/linux/dcache.h
>> index 4df9261..ed9f74f 100644
>> --- a/include/linux/dcache.h
>> +++ b/include/linux/dcache.h
>> @@ -339,7 +339,8 @@ extern int d_validate(struct dentry *, struct dentry *);
>>   */
>>  extern char *dynamic_dname(struct dentry *, char *, int, const char *, ...);
>>  
>> -extern char *__d_path(const struct path *path, struct path *root, char *, int);
>> +extern char *__d_path(const struct path *, const struct path *, char *, int);
>> +extern char *d_absolute_path(const struct path *, char *, int);
>>  extern char *d_path(const struct path *, char *, int);
>>  extern char *d_path_with_unreachable(const struct path *, char *, int);
>>  extern char *dentry_path_raw(struct dentry *, char *, int);
>> diff --git a/include/linux/fs.h b/include/linux/fs.h
>> index e313022..019dc55 100644
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ -1942,6 +1942,7 @@ extern int fd_statfs(int, struct kstatfs *);
>>  extern int statfs_by_dentry(struct dentry *, struct kstatfs *);
>>  extern int freeze_super(struct super_block *super);
>>  extern int thaw_super(struct super_block *super);
>> +extern bool our_mnt(struct vfsmount *mnt);
>>  
>>  extern int current_umask(void);
>>  
>> diff --git a/security/apparmor/path.c b/security/apparmor/path.c
>> index 36cc0cc..b566eba 100644
>> --- a/security/apparmor/path.c
>> +++ b/security/apparmor/path.c
>> @@ -57,23 +57,44 @@ static int prepend(char **buffer, int buflen, const char *str, int namelen)
>>  static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  			    char **name, int flags)
>>  {
>> -	struct path root, tmp;
>>  	char *res;
>> -	int connected, error = 0;
>> +	int error = 0;
>> +	int connected = 1;
>> +
>> +	if (path->mnt->mnt_flags & MNT_INTERNAL) {
>> +		/* it's not mounted anywhere */
>> +		res = dentry_path(path->dentry, buf, buflen);
>> +		*name = res;
>> +		if (IS_ERR(res)) {
>> +			*name = buf;
>> +			return PTR_ERR(res);
>> +		}
>> +		if (path->dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
>> +		    strncmp(*name, "/sys/", 5) == 0) {
>> +			/* TODO: convert over to using a per namespace
>> +			 * control instead of hard coded /proc
>> +			 */
>> +			return prepend(name, *name - buf, "/proc", 5);
>> +		}
>> +		return 0;
>> +	}
>>  
>> -	/* Get the root we want to resolve too, released below */
>> +	/* resolve paths relative to chroot?*/
>>  	if (flags & PATH_CHROOT_REL) {
>> -		/* resolve paths relative to chroot */
>> +		struct path root;
>>  		get_fs_root(current->fs, &root);
>> -	} else {
>> -		/* resolve paths relative to namespace */
>> -		root.mnt = current->nsproxy->mnt_ns->root;
>> -		root.dentry = root.mnt->mnt_root;
>> -		path_get(&root);
>> +		res = __d_path(path, &root, buf, buflen);
>> +		if (res && !IS_ERR(res)) {
>> +			/* everything's fine */
>> +			*name = res;
>> +			path_put(&root);
>> +			goto ok;
>> +		}
>> +		path_put(&root);
>> +		connected = 0;
>>  	}
>>  
>> -	tmp = root;
>> -	res = __d_path(path, &tmp, buf, buflen);
>> +	res = d_absolute_path(path, buf, buflen);
>>  
>>  	*name = res;
>>  	/* handle error conditions - and still allow a partial path to
>> @@ -84,7 +105,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  		*name = buf;
>>  		goto out;
>>  	}
>> +	if (!our_mnt(path->mnt))
>> +		connected = 0;
>>  
>> +ok:
>>  	/* Handle two cases:
>>  	 * 1. A deleted dentry && profile is not allowing mediation of deleted
>>  	 * 2. On some filesystems, newly allocated dentries appear to the
>> @@ -97,10 +121,7 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  			goto out;
>>  	}
>>  
>> -	/* Determine if the path is connected to the expected root */
>> -	connected = tmp.dentry == root.dentry && tmp.mnt == root.mnt;
>> -
>> -	/* If the path is not connected,
>> +	/* If the path is not connected to the expected root,
>>  	 * check if it is a sysctl and handle specially else remove any
>>  	 * leading / that __d_path may have returned.
>>  	 * Unless
>> @@ -112,17 +133,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  	 *     namespace root.
>>  	 */
>>  	if (!connected) {
>> -		/* is the disconnect path a sysctl? */
>> -		if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC &&
>> -		    strncmp(*name, "/sys/", 5) == 0) {
>> -			/* TODO: convert over to using a per namespace
>> -			 * control instead of hard coded /proc
>> -			 */
>> -			error = prepend(name, *name - buf, "/proc", 5);
>> -		} else if (!(flags & PATH_CONNECT_PATH) &&
>> +		if (!(flags & PATH_CONNECT_PATH) &&
>>  			   !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) &&
>> -			     (tmp.mnt == current->nsproxy->mnt_ns->root &&
>> -			      tmp.dentry == tmp.mnt->mnt_root))) {
>> +			     our_mnt(path->mnt))) {
>>  			/* disconnected path, don't return pathname starting
>>  			 * with '/'
>>  			 */
>> @@ -133,8 +146,6 @@ static int d_namespace_path(struct path *path, char *buf, int buflen,
>>  	}
>>  
>>  out:
>> -	path_put(&root);
>> -
>>  	return error;
>>  }
>>  
>> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
>> index 738bbdf..36fa7c9 100644
>> --- a/security/tomoyo/realpath.c
>> +++ b/security/tomoyo/realpath.c
>> @@ -101,9 +101,8 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
>>  {
>>  	char *pos = ERR_PTR(-ENOMEM);
>>  	if (buflen >= 256) {
>> -		struct path ns_root = { };
>>  		/* go to whatever namespace root we are under */
>> -		pos = __d_path(path, &ns_root, buffer, buflen - 1);
>> +		pos = d_absolute_path(path, buffer, buflen - 1);
>>  		if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
>>  			struct inode *inode = path->dentry->d_inode;
>>  			if (inode && S_ISDIR(inode->i_mode)) {
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
> 

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

[Al Viro]

On Tue, Dec 06, 2011 at 08:26:09PM -0800, John Johansen wrote:

> Reviewed-by: John Johansen <john.johansen@canonical.com>

Umm... assuming that's an ACK, the damn thing is in
git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git for-linus

Linus, could you pull it?

Shortlog:
Al Viro (1):
      fix apparmor dereferencing potentially freed dentry, sanitize __d_path() API

Diffstat:
 fs/dcache.c                |   71 +++++++++++++++++++++++++++----------------
 fs/namespace.c             |   20 +++++++-----
 fs/seq_file.c              |    6 ++--
 include/linux/dcache.h     |    3 +-
 include/linux/fs.h         |    1 +
 security/apparmor/path.c   |   65 +++++++++++++++++++++++----------------
 security/tomoyo/realpath.c |    3 +-
 7 files changed, 100 insertions(+), 69 deletions(-)

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

[Al Viro]

On Wed, Dec 07, 2011 at 04:45:10AM +0000, Al Viro wrote:
> On Tue, Dec 06, 2011 at 08:26:09PM -0800, John Johansen wrote:
> 
> > Reviewed-by: John Johansen <john.johansen@canonical.com>
> 
> Umm... assuming that's an ACK, the damn thing is in
> git://git.kernel.org:/pub/scm/linux/kernel/git/viro/vfs.git for-linus
> 
> Linus, could you pull it?

OK, explicit ACKed-by confirmed, added to commit and pushed.

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

[Tetsuo Handa]

Al Viro wrote:
> diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c
> index 738bbdf..36fa7c9 100644
> --- a/security/tomoyo/realpath.c
> +++ b/security/tomoyo/realpath.c
> @@ -101,9 +101,8 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer,
>  {
>         char *pos = ERR_PTR(-ENOMEM);
>         if (buflen >= 256) {
> -               struct path ns_root = { };
>                 /* go to whatever namespace root we are under */
> -               pos = __d_path(path, &ns_root, buffer, buflen - 1);
> +               pos = d_absolute_path(path, buffer, buflen - 1);
>                 if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
>                         struct inode *inode = path->dentry->d_inode;
>                         if (inode && S_ISDIR(inode->i_mode)) {

Currently, TOMOYO assumes that -ENAMETOOLONG is the only error which __d_path()
might return (and retries with larger buffer size unless kmalloc() fails).
If d_absolute_path() starts returning -EINVAL, TOMOYO will deny requests even
if "partial (I mean the result would have been different if reachable)"
pathname is granted by the policy.

How commonly can conditions that make d_absolute_path() return -EINVAL happen?



Linus Torvalds wrote:
> I don't know of any big distro that uses Tomoyo, so...
Ubuntu, Debian, OpenSUSE, ArchLinux, Mandriva...
Hope you know one of these distro. ;-)

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

[Al Viro]

On Wed, Dec 07, 2011 at 12:26:25PM +0900, Tetsuo Handa wrote:
> >         char *pos = ERR_PTR(-ENOMEM);
> >         if (buflen >= 256) {
> > -               struct path ns_root = { };
> >                 /* go to whatever namespace root we are under */
> > -               pos = __d_path(path, &ns_root, buffer, buflen - 1);
> > +               pos = d_absolute_path(path, buffer, buflen - 1);
> >                 if (!IS_ERR(pos) && *pos == '/' && pos[1]) {
> >                         struct inode *inode = path->dentry->d_inode;
> >                         if (inode && S_ISDIR(inode->i_mode)) {
> 
> Currently, TOMOYO assumes that -ENAMETOOLONG is the only error which __d_path()
> might return (and retries with larger buffer size unless kmalloc() fails).
> If d_absolute_path() starts returning -EINVAL, TOMOYO will deny requests even
> if "partial (I mean the result would have been different if reachable)"
> pathname is granted by the policy.
> 
> How commonly can conditions that make d_absolute_path() return -EINVAL happen?

Race with umount -l, basically.  In that case the pathname is completely
unreliable - if I do umount -l /mnt, pathnames that would be under mnt
may get truncated on *ANY* mountpoint.  Not "always cut on /mnt"; not "always
cut on the last mountpoint"; it's "everything from root to arbitrary mountpoint
on that path is not noticed".  If your policy really has to deal with such
situations (unexpected umount -l racing with operations in the subtree that
gets dissolved), you do have a problem.  Right now, in the mainline kernel.
Because the pathname you are currently getting is unreliable as hell and
making any decisions basing on it is, er, not particulary wise.

If you want to add handling of that -EINVAL in some form, feel free.  But
keep in mind that the *ONLY* part of pathname you can really recover at
that point is what dentry_path() would give you - i.e. from fs root to
object in question.  Everything prior to that is absolutely unreliable
and is bound to go away in a very short while - ->mnt_parent on the entire
chain is in process of being dissolved when we hit that race.

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

[Tetsuo Handa]

Al Viro wrote:
> > How commonly can conditions that make d_absolute_path() return -EINVAL happen?
> 
> Race with umount -l, basically.

d_absolute_path() will return -EINVAL if lazy unmount happens. I see.

Then, I prefer not denying the request with -EINVAL no matter how unreliable
the returned pathname is. I don't want to deny the request unless -ENOMEM
happens or rejected by the policy.

> In that case the pathname is completely
> unreliable - if I do umount -l /mnt, pathnames that would be under mnt
> may get truncated on *ANY* mountpoint.  Not "always cut on /mnt"; not "always
> cut on the last mountpoint"; it's "everything from root to arbitrary mountpoint
> on that path is not noticed".

Unfortunate specification for pathname based access control.
But since I assume that multiple LSM modules can run in parallel
( http://sourceforge.jp/projects/tomoyo/docs/lca2009-kumaneko.pdf),
I leave more stricter decisions to inode based access control.

So, can we keep behavior of tomoyo_get_absolute_path() unchanged?

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

[Al Viro]

On Wed, Dec 07, 2011 at 02:01:21PM +0900, Tetsuo Handa wrote:
> Al Viro wrote:
> > > How commonly can conditions that make d_absolute_path() return -EINVAL happen?
> > 
> > Race with umount -l, basically.
> 
> d_absolute_path() will return -EINVAL if lazy unmount happens. I see.
> 
> Then, I prefer not denying the request with -EINVAL no matter how unreliable
> the returned pathname is. I don't want to deny the request unless -ENOMEM
> happens or rejected by the policy.
> 
> > In that case the pathname is completely
> > unreliable - if I do umount -l /mnt, pathnames that would be under mnt
> > may get truncated on *ANY* mountpoint.  Not "always cut on /mnt"; not "always
> > cut on the last mountpoint"; it's "everything from root to arbitrary mountpoint
> > on that path is not noticed".
> 
> Unfortunate specification for pathname based access control.
> But since I assume that multiple LSM modules can run in parallel
> ( http://sourceforge.jp/projects/tomoyo/docs/lca2009-kumaneko.pdf),
> I leave more stricter decisions to inode based access control.
> 
> So, can we keep behavior of tomoyo_get_absolute_path() unchanged?

Sure, you are always free to add
	if (pos == ERR_PTR(-EINVAL)) {
		pos = dentry_path(path->dentry, ...)
		/* do whatever you want to buffer to indicate that
		 * beginning had been lost
		 */
	}
since that's the _only_ reliable part of pathname information there is in
such situation.  What should be done to buffer contents is *really* up
to you - what you have there is the path from the root of filesystem path
points to and to path->dentry.  Beginning *is* lost; the thing had been
unmounted and this is all you have.

Or you might want to do __d_path() from (path->mnt,path->mnt->mnt_root) to
path - that's the path from the last mountpoint to your object; i.e. it may
be shorter if that vfsmount had been a binding into the guts of filesystem,
but that is what __d_path() as you used it would stabilize to once the race
window is over.

Again, that's what happens if you are hit with umount and there is *no*
absolute path anymore.  What should be done in such situation is really
up to you - as far as I'm concerned, those races are among the reasons why
pathname-based MAC is a fundamentally wrong idea.

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

[Tetsuo Handa]

Al Viro wrote:
> > So, can we keep behavior of tomoyo_get_absolute_path() unchanged?
> 
> Sure, you are always free to add
> 	if (pos == ERR_PTR(-EINVAL)) {
> 		pos = dentry_path(path->dentry, ...)
> 		/* do whatever you want to buffer to indicate that
> 		 * beginning had been lost
> 		 */
> 	}

Maybe I can call tomoyo_get_local_path() if tomoyo_get_absolute_path() failed
with -EINVAL.

> Or you might want to do __d_path() from (path->mnt,path->mnt->mnt_root) to
> path - that's the path from the last mountpoint to your object; i.e. it may
> be shorter if that vfsmount had been a binding into the guts of filesystem,
> but that is what __d_path() as you used it would stabilize to once the race
> window is over.

Excuse me, what "once the race window is over" means?
Does

  do {
    pos = d_absolute_path(path, buffer, buflen - 1);
  } while (pos == ERR_PTR(-EINVAL));

work (i.e. racing with "umount -l" is a temporary failure)?

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

[Al Viro]

On Wed, Dec 07, 2011 at 02:44:33PM +0900, Tetsuo Handa wrote:

> Excuse me, what "once the race window is over" means?
> Does
> 
>   do {
>     pos = d_absolute_path(path, buffer, buflen - 1);
>   } while (pos == ERR_PTR(-EINVAL));
> 
> work (i.e. racing with "umount -l" is a temporary failure)?

I said "what your original use of __d_path() would stabilize to".  IOW,
that's what you'd get after all ->mnt_parent in the chain are killed
by release_mounts().  And no, since the moment that release_mounts()
started there was *no* *absolute* *path* *at* *all*.

>From that moment on, the point you are looking at is not connected to any
global root.  Or to anything still mounted, of course.  What changes is
how little of what used to be the path to root remains; very shortly
it's down to just path->mnt not connected to *anything*.  __d_path() call
as you have it in the current tree will report the remaining (shrinking)
part of path, eventually settling to just the part from path->mnt->root
to path->dentry.  d_absolute_path() will be giving you ERR_PTR(-EINVAL)
all along; the thing it is supposed to give you does not exist anymore.

Racing with umount -l is temporary in a sense that as soon as a vfsmount
detached by umount -l ceases being busy, it gets killed.  If you stand
there, holding a reference and looking for a path connecting it to something
mounted, well... (a) such path won't appear and (b) vfsmount will remain
busy for as long as you are holding that reference...

The real question is what pathname do you _want_ in this situation.  Define
that and we'll be able to do something about it; if you really are asking
for "whatever this code used to do, modulo races", then what you want is
	if (pos == ERR_PTR(-EINVAL)) {
		/* it got unmounted; just report what's left and be quiet */
		struct path root = {path->mnt, path->mnt->root};
		pos = __d_path(path, &root, buf, buflen - 1);
		if (!pos) /* it's really, *REALLY* screwed up somehow */
			return ERR_PTR(-EINVAL);
	}
and that's it.  But at that point I would start seriously thinking about the
usefulness of checks done on that tail of pathname, false negatives, etc.

We could, I guess, make d_absolute_path() do just that on such paths as an
automatic fallback [1].  apparmor's use of our_mnt(path->mnt) would've caught
those, so we are not introducing false negatives there.

However, I *really* wonder if that's the right thing to do in any sense.
BTW, what your current code does if you have a file bound on another
file, open it, umount -l it, let the dust settle and then do some operation
that triggers tomoyo_get_absolute_path() on it?  Because you'll be getting
a vfsmount/dentry pair that has
	* dentry == vfsmount->mnt_root
	* vfsmount->mnt_parent == vfsmount
	* dentry->d_inode being a non-directory
and there is nothing whatsoever in what remains of the pathname.  Not a single
component.  IOW, you'll get "/" in buf.  Might be good in a testsuite - is
there any code in security/tomoyo that would be relying on assumption that
only directory might have a name entirely without components?

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

[Tetsuo Handa]

Al Viro wrote:
> BTW, what your current code does if you have a file bound on another
> file, open it, umount -l it, let the dust settle and then do some operation
> that triggers tomoyo_get_absolute_path() on it?  Because you'll be getting
> a vfsmount/dentry pair that has
> 	* dentry == vfsmount->mnt_root
> 	* vfsmount->mnt_parent == vfsmount
> 	* dentry->d_inode being a non-directory
> and there is nothing whatsoever in what remains of the pathname.  Not a single
> component.  IOW, you'll get "/" in buf.  Might be good in a testsuite - is
> there any code in security/tomoyo that would be relying on assumption that
> only directory might have a name entirely without components?

TOMOYO assumes that only directory ends with '/'.

I wrote a test program and compared among below cases.

   current code
   current code + your patch
   current code + your patch + my patch (attached at the bottom)

---------- test1.c ----------
#include <stdio.h>
#include <stdlib.h>
#include <unistd.h>
#include <sys/ioctl.h>
#include <sys/mount.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>
#define MNT_DETACH 2

static void die(const char *msg)
{
        fprintf(stderr, "%s\n", msg);
        exit(1);
}

int main(int argc, char *argv[])
{
        int fd;
        if (mount("/tmp/", "/tmp", "tmpfs", 0, NULL))
                die("Can't mount");
        fd = open("/tmp/file1", O_WRONLY | O_CREAT, 0600);
        if (fd == EOF)
                die("Can't create /tmp/file1");
        close(fd);
        fd = open("/tmp/file2", O_WRONLY | O_CREAT, 0600);
        if (fd == EOF)
                die("Can't create /tmp/file2");
        close(fd);
        if (mount("/tmp/file1", "/tmp/file2", NULL, MS_BIND, 0))
                die("Can't bind mount");
        fd = open("/tmp/file2", O_RDONLY | O_CREAT, 0600);
        if (fd == EOF)
                die("Can't open /tmp/file2");
        if (umount2("/tmp/file2", MNT_DETACH))
                die("Can't unmount 2");
        ioctl(fd, 0);
        close(fd);
        return 0;
}
---------- test1.c ----------

Below are the policy checked by TOMOYO.

---- linux b835c0f4 ----
    0: file create  /tmp/file1 0600
    1: file create  /tmp/file2 0600
    2: file ioctl   / 0x0
    3: file mount   /tmp/ /tmp/ tmpfs 0x0
    4: file mount   /tmp/file1 /tmp/file2 --bind 0x0
    5: file read    /tmp/file2
    6: file unmount /tmp/file2
    7: file write   /tmp/file1
    8: file write   /tmp/file2

ioctl() handled but its pathname is wrong.

---- linux b835c0f4 with your patch ----
    0: file create  /tmp/file1 0600
    1: file create  /tmp/file2 0600
    2: file mount   /tmp/ /tmp/ tmpfs 0x0
    3: file mount   /tmp/file1 /tmp/file2 --bind 0x0
    4: file read    /tmp/file2
    5: file unmount /tmp/file2
    6: file write   /tmp/file1
    7: file write   /tmp/file2

ioctl() failed (i.e. regression) because tomoyo_get_absolute_path() failed.
The error message was

  ERROR: Out of memory at tomoyo_realpath_from_path.

---- linux b835c0f4 with your patch and my patch ----
    0: file create  /tmp/file1 0600
    1: file create  /tmp/file2 0600
    2: file ioctl   tmpfs:/file1 0x0
    3: file mount   /tmp/ /tmp/ tmpfs 0x0
    4: file mount   /tmp/file1 /tmp/file2 --bind 0x0
    5: file read    /tmp/file2
    6: file unmount /tmp/file2
    7: file write   /tmp/file1
    8: file write   /tmp/file2

ioctl() handled using pathname returned by tomoyo_get_local_path().

> The real question is what pathname do you _want_ in this situation.  Define
> that and we'll be able to do something about it;

Among above three results, the last one will be the best.

[PATCH] TOMOYO: Fix pathname handling of disconnected paths.

Current tomoyo_realpath_from_path() implementation returns strange pathname
when calculating pathname of a file which belongs to lazy unmounted tree.
Use local pathname rather than strange absolute pathname in that case.

Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

--- a/security/tomoyo/realpath.c
+++ b/security/tomoyo/realpath.c
@@ -293,8 +293,16 @@ char *tomoyo_realpath_from_path(struct path *path)
 			pos = tomoyo_get_local_path(path->dentry, buf,
 						    buf_len - 1);
 		/* Get absolute name for the rest. */
-		else
+		else {
 			pos = tomoyo_get_absolute_path(path, buf, buf_len - 1);
+			/*
+			 * Fall back to local name if absolute name is not
+			 * available.
+			 */
+			if (pos == ERR_PTR(-EINVAL))
+				pos = tomoyo_get_local_path(path->dentry, buf,
+							    buf_len - 1);
+		}
 encode:
 		if (IS_ERR(pos))
 			continue;

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

[Al Viro]

On Wed, Dec 07, 2011 at 05:59:49PM +0900, Tetsuo Handa wrote:
> Al Viro wrote:
> > BTW, what your current code does if you have a file bound on another
> > file, open it, umount -l it, let the dust settle and then do some operation
> > that triggers tomoyo_get_absolute_path() on it?  Because you'll be getting
> > a vfsmount/dentry pair that has
> > 	* dentry == vfsmount->mnt_root
> > 	* vfsmount->mnt_parent == vfsmount
> > 	* dentry->d_inode being a non-directory
> > and there is nothing whatsoever in what remains of the pathname.  Not a single
> > component.  IOW, you'll get "/" in buf.  Might be good in a testsuite - is
> > there any code in security/tomoyo that would be relying on assumption that
> > only directory might have a name entirely without components?
> 
> TOMOYO assumes that only directory ends with '/'.

Then it's broken in the current mainline (and had been for as long as it
had been using __d_path()).  Because that's all you'll get from it
for such vfsmount/dentry pair...

> Among above three results, the last one will be the best.

OK, I'm fine with your patch; for bisectability sake it ought to go before
mine, with mine on top of it.

How will we do that?  Should I put it into vfs.git#for-linus before __d_path()
patch and ask Linus to pull that?

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

[Al Viro]

On Wed, Dec 07, 2011 at 05:59:49PM +0900, Tetsuo Handa wrote:

> [PATCH] TOMOYO: Fix pathname handling of disconnected paths.
> 
> Current tomoyo_realpath_from_path() implementation returns strange pathname
> when calculating pathname of a file which belongs to lazy unmounted tree.
> Use local pathname rather than strange absolute pathname in that case.
> 
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

seeing that Linus has already pulled mine, I guess the best I can do is
this:

Acked-by: Al Viro <viro@zeniv.linux.org.uk>

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 4:16 PM, Al Viro <viro@zeniv.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

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

[Al Viro]

On Tue, Dec 06, 2011 at 04:39:40PM -0800, Linus Torvalds wrote:
> 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?

Because the path is useless without some idea where it starts?
"It was /foo/bar/baz counting from some point; might have
been global root, might have been any random mountpoint if we'd
raced with umount and you've got no way to tell one from another"
is not particulary useful in logs.

The thing is, on the next boot exact same line might grow or lose
a prefix.  Randomness of that kind is OK if you know that the
pathname is just the best-effort thing here and you *always*
end up guessing which one it was.  But here most of the real-world
cases will have them relative to absolute root and stable.  That's
a really ugly combination - something that works most of the time
until you hit a race.  Then, without any notice, you need to
guess that _this_ time have to go looking for what that pathname
might have been appended to...  Not fun.

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 4:52 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>>
>> But why do you want that ancestor thing?
>
> Because the path is useless without some idea where it starts?

No.

You're thinking about this wrong, and you have gotten stuck thinking that way.

The "where it starts" doesn't help you in any way. It has absolutely
zero new information, apart from the "it's unreachable from your
current root" part.

> "It was /foo/bar/baz counting from some point; might have
> been global root, might have been any random mountpoint if we'd
> raced with umount and you've got no way to tell one from another"
> is not particulary useful in logs.

Umm. Think it through. NOBODY WILL EVER SEE ANYTHING ELSE *ANYWAY*.

There is no valid information in that "where it starts". There's
nothing more you can tell about it. So why return it?

The "it was /foo/bar/baz" part is undeniabty useful. People do
actually want that part, but they want it for printouts to the
sysadmin or the user, who will then go "Ahh, I see what's up, they're
in a chroot environment and they still have access to /foo/bar/baz
from outside".

But no possible user would ever actually care about that "ancestor"
thing. There is absolutely nothing useful you can say about it. It's
*some* kind of root, but that's just about all the relevant
information you'd ever want to know.

Equally importantly, no existing users actually use that information.
Sure, AppArmor looked at it, but it looked at it *WRONG*. What
AppArmor does doesn't actually make any sense to do.

So why make excuses for it? It's useless and unused information that
only complicates the interface. The only thing anybody *ever* actually
wants to know is that one single bit of information: "was it reachable
or not?"

                           Linus

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

[Al Viro]

On Tue, Dec 06, 2011 at 05:11:59PM -0800, Linus Torvalds wrote:

> It's
> *some* kind of root, but that's just about all the relevant
> information you'd ever want to know.

Damnit, it may very well be NOT ANY KIND OF ROOT AT ALL.  Which is what
I'd been trying to tell you all along.  You can race with umount -l.
In that case that thing might have been *INSIDE* your chroot jail.
Anywhere in it.

We raced with umount -l /mnt.  And what would be /mnt/1/2/foo/bar/baz
had shown up as /foo/bar/baz.  A few cycles earlier or later it could
be /1/2/foo/bar/baz or /bar/baz.  And you are not in chroot jail at
all.  _That_ is what I have trouble with.  Because that sysadmin will
misinterpret it exactly the way you have...

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.

And yes, we can avoid returning that struct vfsmount *.  Fortunately.
See the posting in another branch of that thread for details.

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 5:23 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> Damnit, it may very well be NOT ANY KIND OF ROOT AT ALL.  Which is what
> I'd been trying to tell you all along.  You can race with umount -l.
> In that case that thing might have been *INSIDE* your chroot jail.
> Anywhere in it.

AND NOBODY CARES.

That's the deeper point here. IT DOES NOT MATTER.

Why do you try to convince people that it does. Nobody uses the
information that you claim is so magically important.

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.

Why do you keep on harping on this totally useless issue? Seriously?

                 Linus

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

[Al Viro]

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?

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

[Linus Torvalds]

On Tue, Dec 6, 2011 at 6:17 PM, Al Viro <viro@zeniv.linux.org.uk> wrote:
>
> 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.

So I like your prepend_path() changes. Returning the status of the
thing as a positive number looks good, and you never lose any
information.

In some ways, I'd prefer to make "prepend_path()" the "real"
interface, but whatever.

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

Again, my only real complaint about this is that you are trying too
hard to pander to crazy users, but I have no complaints about the code
itself any more. In this form, the patch looks fine, now it's just the
*callers* that are insane.

>        * __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().

Looks fine.

>        * __d_path() root argument becomes const.  Everyone agrees, I hope.

Absolutely.

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

So I think this crazy code should just be deleted, but whatever. At
least now it's fairly well insulated, and now it's "Apparmor crazy
code" rather than "VFS layer crazy code", which makes it a lot more
palatable to me from a maintenance standpoint.

Crazy security policies are almost par for the course. Crazy VFS layer
functions I get upset about.

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

Again, I'm not convinced that is actually a sane security model, and
I'd really hope that code just goes away or gets handled totally
differently. But again, at least now it's "internal to apparmor"
craziness, and as such not as horrible at all.

> Are you OK with the above?

Yes.

                     Linus