* [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) [not found] ` <20111204221020.GB2203@ZenIV.linux.org.uk> @ 2011-12-05 3:27 ` Al Viro 2011-12-06 2:34 ` John Johansen 0 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2011-12-05 3:27 UTC (permalink / raw) To: John Johansen; +Cc: linux-kernel, linux-security-module, linux-fsdevel I think I have a saner solution; the root of the problem is that __d_path() API is asking for trouble. What I'm proposing is: * Give prepend_path() an extra argument for reporting which fatherless thing has it reached if it has missed the root - struct path *bastard. struct path *root becomes const on that *and* what we put into *bastard is properly grabbed. We do that only if that pointer is not NULL, of course. In any case we return 1 if we go out through global_root: * modify the callers accordingly; everything except __d_path() will simply pass NULL as that extra argument and instead of checking if root has been changed, just check if return value was positive. * __d_path() gets the similar extra argument itself; if it's non-NULL and we miss the root, the caller can expect to get (referenced) point where the walk has stopped stored there. _Maybe_ it ought to fill it with NULL/NULL otherwise; I've just done that in the only such caller before calling __d_path(). If it is NULL *and* root has not been NULL/NULL (i.e. we asked for subtree explicitly and have nowhere to put the stopping point), we return NULL instead of pointer to relative pathname. * seq_path_root() does _NOT_ return -ENAMETOOLONG (it's stupid anyway - the normal seq_file logics will take care of growing the buffer and redoing the call of ->show() just fine). However, if it gets path not reachable from root, it returns SEQ_SKIP. The only caller adjusted (i.e. stopped ignoring the return value as it used to do). * unlike seq_path_root(), the caller in tomoyo passes NULL/NULL in *root (it wants absolute path). It still calls with bastard == NULL, so no references are grabbed. * apparmor d_namespace_path() calls __d_path() passing it a pointer to local struct path to fill. It's been earlier initialized to NULL/NULL (see above in part about __d_path()), so the check for "has it been outside of chroot jail" is simply bastard.mnt != NULL after the call. Moreover, in this case it's safe to access vfsmount and dentry; we can do the checks just fine, they won't be freed under us. One more thing - I've exported uninlined variant of check_mnt() (called our_mnt(), for obvious reasons) and used it here. Completely untested patch follows: Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> --- diff --git a/fs/dcache.c b/fs/dcache.c index 2ac8e7e..d8a1783 100644 --- a/fs/dcache.c +++ b/fs/dcache.c @@ -2440,7 +2440,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 + * @bastard: 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 * @@ -2449,7 +2450,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 *bastard, 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,8 +2503,14 @@ 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 (bastard) { + bastard->mnt = mntget(vfsmnt); + bastard->dentry = dget(dentry); + } + if (!slash) + error = prepend(buffer, buflen, "/", 1); + if (!error) + error = 1; goto out; } @@ -2521,8 +2530,12 @@ global_root: * * If path is not reachable from the supplied root, then the value of * root is changed (without modifying refcounts). + * + * XXX: rewrite description */ -char *__d_path(const struct path *path, struct path *root, +char *__d_path(const struct path *path, + const struct path *root, + struct path *bastard, char *buf, int buflen) { char *res = buf + buflen; @@ -2530,19 +2543,26 @@ 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, bastard, &res, &buflen); write_sequnlock(&rename_lock); - if (error) + if (error < 0) return ERR_PTR(error); + /* + * we'd been asked to stop at specific point, missed it and + * had not been asked to tell where we'd ended up. + */ + if (error > 0 && !bastard && root->mnt) + return NULL; return res; } /* * 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 +2571,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 +2599,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 +2613,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 +2635,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 +2642,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 +2774,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 ea1cf5a..58c64c3 100644 --- a/fs/namespace.c +++ b/fs/namespace.c @@ -1023,15 +1023,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); @@ -2761,3 +2758,9 @@ void kern_unmount(struct vfsmount *mnt) } } EXPORT_SYMBOL(kern_unmount); + +bool our_mnt(struct vfsmount *mnt) +{ + return check_mnt(mnt); +} +EXPORT_SYMBOL(our_mnt); diff --git a/fs/seq_file.c b/fs/seq_file.c index 05d6b0e..688642d 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) @@ -462,7 +460,9 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root, if (size) { char *p; - p = __d_path(path, root, buf, size); + p = __d_path(path, root, NULL, 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..2b6cf1e 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 *, + 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..ed300cc 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -57,23 +57,16 @@ 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 root = {}; + struct path bastard = {}; char *res; - int connected, error = 0; + int error = 0; - /* Get the root we want to resolve too, released below */ - if (flags & PATH_CHROOT_REL) { - /* resolve paths relative to chroot */ + /* resolve paths relative to chroot?*/ + if (flags & PATH_CHROOT_REL) 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); - } - tmp = root; - res = __d_path(path, &tmp, buf, buflen); + res = __d_path(path, &root, &bastard, buf, buflen); *name = res; /* handle error conditions - and still allow a partial path to @@ -97,10 +90,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 @@ -111,9 +101,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, * of chroot) and specifically directed to connect paths to * namespace root. */ - if (!connected) { + if (bastard.mnt) { /* is the disconnect path a sysctl? */ - if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC && + if (bastard.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 +111,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(bastard.mnt))) { /* disconnected path, don't return pathname starting * with '/' */ @@ -133,7 +122,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, } out: - path_put(&root); + if (bastard.mnt) + path_put(&bastard); + if (flags & PATH_CHROOT_REL) + path_put(&root); return error; } diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c index e1eb982..d5f469a 100644 --- a/security/tomoyo/realpath.c +++ b/security/tomoyo/realpath.c @@ -95,7 +95,7 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer, 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_path(path, &ns_root, NULL, buffer, buflen - 1); if (!IS_ERR(pos) && *pos == '/' && pos[1]) { struct inode *inode = path->dentry->d_inode; if (inode && S_ISDIR(inode->i_mode)) { ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-05 3:27 ` [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) Al Viro @ 2011-12-06 2:34 ` John Johansen 2011-12-06 3:58 ` Al Viro 0 siblings, 1 reply; 8+ messages in thread From: John Johansen @ 2011-12-06 2:34 UTC (permalink / raw) To: Al Viro; +Cc: linux-kernel, linux-security-module, linux-fsdevel On 12/04/2011 07:27 PM, Al Viro wrote: > I think I have a saner solution; the root of the problem is that > __d_path() API is asking for trouble. What I'm proposing is: > > * Give prepend_path() an extra argument for reporting which > fatherless thing has it reached if it has missed the root - struct > path *bastard. struct path *root becomes const on that *and* what > we put into *bastard is properly grabbed. We do that only if that > pointer is not NULL, of course. In any case we return 1 if we go > out through global_root: > > * modify the callers accordingly; everything except __d_path() > will simply pass NULL as that extra argument and instead of checking > if root has been changed, just check if return value was positive. > > * __d_path() gets the similar extra argument itself; if it's non-NULL > and we miss the root, the caller can expect to get (referenced) point where > the walk has stopped stored there. _Maybe_ it ought to fill it with > NULL/NULL otherwise; I've just done that in the only such caller before > calling __d_path(). If it is NULL *and* root has not been NULL/NULL (i.e. > we asked for subtree explicitly and have nowhere to put the stopping point), > we return NULL instead of pointer to relative pathname. > > * seq_path_root() does _NOT_ return -ENAMETOOLONG (it's stupid anyway - > the normal seq_file logics will take care of growing the buffer and redoing > the call of ->show() just fine). However, if it gets path not reachable > from root, it returns SEQ_SKIP. The only caller adjusted (i.e. stopped > ignoring the return value as it used to do). > > * unlike seq_path_root(), the caller in tomoyo passes NULL/NULL in > *root (it wants absolute path). It still calls with bastard == NULL, so > no references are grabbed. > > * apparmor d_namespace_path() calls __d_path() passing it a pointer > to local struct path to fill. It's been earlier initialized to NULL/NULL > (see above in part about __d_path()), so the check for "has it been outside > of chroot jail" is simply bastard.mnt != NULL after the call. Moreover, > in this case it's safe to access vfsmount and dentry; we can do the checks > just fine, they won't be freed under us. One more thing - I've exported > uninlined variant of check_mnt() (called our_mnt(), for obvious reasons) > and used it here. > > Completely untested patch follows: > Thanks Al, this looks good, and I have been running this one a test box doing regression testing successfully for hours with one small change in the apparmor portion of the code which I have included below. With the apparmor change you can add Reviewed-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > --- << snip >> > --- a/security/apparmor/path.c > +++ b/security/apparmor/path.c > @@ -57,23 +57,16 @@ 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 root = {}; > + struct path bastard = {}; > char *res; > - int connected, error = 0; > + int error = 0; > > - /* Get the root we want to resolve too, released below */ > - if (flags & PATH_CHROOT_REL) { > - /* resolve paths relative to chroot */ > + /* resolve paths relative to chroot?*/ > + if (flags & PATH_CHROOT_REL) > 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); > - } > > - tmp = root; > - res = __d_path(path, &tmp, buf, buflen); > + res = __d_path(path, &root, &bastard, buf, buflen); > > *name = res; > /* handle error conditions - and still allow a partial path to > @@ -97,10 +90,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 > @@ -111,9 +101,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, > * of chroot) and specifically directed to connect paths to > * namespace root. > */ > - if (!connected) { > + if (bastard.mnt) { This doesn't quite match the original !connected check. It is the same for the case of flags & PATH_CHROOT_REL (ie. root = current->fs), but for the case where we pass root = { } bastard.mnt is always true, while in the old !connected case, it would only be true if !our_mnt(bastard). This isn't entirely covered by the subsequent check !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) && our_mnt(bastard.mnt))) { which means apparmor ends up treating the path as disconnected when it shouldn't. This can be fixed by changing the check to - if (!connected) { + if (bastard.mnt && (root.mnt || !our_mnt(bastard.mnt)) { I will send a separate patch to kill !(((flags & CHROOT_NSCONNECT) == CHROOT_NSCONNECT) && our_mnt(bastard.mnt)) as its an unused vestige which isn't even possible, and was missed being removed during code cleanup. > /* is the disconnect path a sysctl? */ > - if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC && > + if (bastard.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 +111,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(bastard.mnt))) { > /* disconnected path, don't return pathname starting > * with '/' > */ > @@ -133,7 +122,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, > } > > out: > - path_put(&root); > + if (bastard.mnt) > + path_put(&bastard); > + if (flags & PATH_CHROOT_REL) > + path_put(&root); > > return error; > } ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-06 2:34 ` John Johansen @ 2011-12-06 3:58 ` Al Viro 2011-12-06 4:41 ` Tetsuo Handa 2011-12-06 7:07 ` James Morris 0 siblings, 2 replies; 8+ messages in thread From: Al Viro @ 2011-12-06 3:58 UTC (permalink / raw) To: John Johansen; +Cc: linux-kernel, linux-security-module, linux-fsdevel On Mon, Dec 05, 2011 at 06:34:46PM -0800, John Johansen wrote: > Thanks Al, this looks good, and I have been running this one a test box doing > regression testing successfully for hours with one small change in the apparmor > portion of the code which I have included below. > > With the apparmor change you can add > > Reviewed-by: John Johansen <john.johansen@canonical.com> OK, change folded, description written, commit pushed to #for-linus. I have *NOT* sent pull request yet; folks, if you have objections, it's time to yell. The commit in question is: commit 8096378b2d15ccd9f185fd16c1255f8f2619e0d6 Author: Al Viro <viro@zeniv.linux.org.uk> Date: Mon Dec 5 08:43:34 2011 -0500 fix apparmor dereferencing potentially freed dentry, sanitize __d_path() API __d_path() API is asking for trouble and in case of apparmor d_namespace_path() getting just that. The root cause is that when __d_path() misses the root it had been told to look for, it stores the location of the most remote ancestor in *root. Without grabbing references. Sure, at the moment of call it had been pinned down by what we have in *path. And if we raced with umount -l, we could have very well stopped at vfsmount/dentry that got freed as soon as prepend_path() dropped vfsmount_lock. It is safe to compare these pointers with pre-existing (and known to be still alive) vfsmount and dentry, as long as all we are asking is "is it the same address?". Dereferencing is not safe and apparmor ended up stepping into that. d_namespace_path() really wants to examine the place where we stopped, even if it's not connected to our namespace. As the result, it looked at ->d_sb->s_magic of a dentry that might've been already freed by that point. All other callers had been careful enough to avoid that, but it's really a bad interface - it invites that kind of trouble. On the other hand, we do *not* want show_mountinfo() to have to do mntput()/dput() - doing that (or any IO, for that matter) under namespace_sem is a bad idea for reasons of robustness, if nothing else. The fix is fairly straightforward, even though it's bigger than I'd like: * Give prepend_path() an extra argument for reporting which fatherless thing has it reached if it has missed the root - struct path *bastard. struct path *root becomes const on that *and* what we put into *bastard is properly grabbed. We do that only if that pointer is not NULL, of course. In any case we return 1 if we go out through global_root: * modify the callers accordingly; everything except __d_path() will simply pass NULL as that extra argument and instead of checking if root has been changed, just check if return value was positive. * __d_path() gets the similar extra argument itself; if it's non-NULL and we miss the root, the caller can expect to get (referenced) point where the walk has stopped stored there. _Maybe_ it ought to fill it with NULL/NULL otherwise; I've just done that in the only such caller before calling __d_path(). If it is NULL *and* root has not been NULL/NULL (i.e. we asked for subtree explicitly and have nowhere to put the stopping point), we return NULL instead of pointer to relative pathname. * seq_path_root() does _NOT_ return -ENAMETOOLONG (it's stupid anyway - the normal seq_file logics will take care of growing the buffer and redoing the call of ->show() just fine). However, if it gets path not reachable from root, it returns SEQ_SKIP. The only caller adjusted (i.e. stopped ignoring the return value as it used to do). * unlike seq_path_root(), the caller in tomoyo passes NULL/NULL in *root (it wants absolute path). It still calls with bastard == NULL, so no references are grabbed. * apparmor d_namespace_path() calls __d_path() passing it a pointer to local struct path to fill. It's been earlier initialized to NULL/NULL (see above in part about __d_path()), so the check for "has it been outside of chroot jail" is simply bastard.mnt != NULL after the call. Moreover, in this case it's safe to access vfsmount and dentry; we can do the checks just fine, they won't be freed under us. One more thing - I've exported uninlined variant of check_mnt() (called our_mnt(), for obvious reasons) and used it here. [AV: folded fix for braino in apparmor part spotted by jj; with that done the patch survives his testing] Reviewed-by: John Johansen <john.johansen@canonical.com> Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> Cc: stable@vger.kernel.org diff --git a/fs/dcache.c b/fs/dcache.c index 10ba92d..d9854ee 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 + * @bastard: 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 *bastard, 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,22 @@ 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 (bastard) { + bastard->mnt = mntget(vfsmnt); + bastard->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 + * @bastard: the fatherless thing we'd walked to if we missed root * @buf: buffer to return value in * @buflen: buffer length * @@ -2519,10 +2529,17 @@ 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). - */ -char *__d_path(const struct path *path, struct path *root, + * If the path is not reachable from the supplied root, results depend on + * whether "bastard" is %NULL or not. If it's non-%NULL, the most remote + * ancestor we'd been able to find will be stored in *bastard, references + * properly grabbed. Otherwise, if *root had not been {%NULL, %NULL} (i.e. + * we'd been asked for a path relative to root of some specific subtree), + * we return %NULL since the pathname we'd formed is _not_ what the caller + * has asked for. + */ +char *__d_path(const struct path *path, + const struct path *root, + struct path *bastard, char *buf, int buflen) { char *res = buf + buflen; @@ -2530,19 +2547,26 @@ 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, bastard, &res, &buflen); write_sequnlock(&rename_lock); - if (error) + if (error < 0) return ERR_PTR(error); + /* + * we'd been asked to stop at specific point, missed it and + * had not been asked to tell where we'd ended up. + */ + if (error > 0 && !bastard && root->mnt) + return NULL; return res; } /* * 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 +2575,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 +2603,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 +2617,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 +2639,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 +2646,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 +2778,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..2c784ea 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,9 @@ void kern_unmount(struct vfsmount *mnt) } } EXPORT_SYMBOL(kern_unmount); + +bool our_mnt(struct vfsmount *mnt) +{ + return check_mnt(mnt); +} +EXPORT_SYMBOL(our_mnt); diff --git a/fs/seq_file.c b/fs/seq_file.c index 05d6b0e..688642d 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) @@ -462,7 +460,9 @@ int seq_path_root(struct seq_file *m, struct path *path, struct path *root, if (size) { char *p; - p = __d_path(path, root, buf, size); + p = __d_path(path, root, NULL, 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..2b6cf1e 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 *, + 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..350cb50 100644 --- a/security/apparmor/path.c +++ b/security/apparmor/path.c @@ -57,23 +57,16 @@ 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 root = {}; + struct path bastard = {}; char *res; - int connected, error = 0; + int error = 0; - /* Get the root we want to resolve too, released below */ - if (flags & PATH_CHROOT_REL) { - /* resolve paths relative to chroot */ + /* resolve paths relative to chroot?*/ + if (flags & PATH_CHROOT_REL) 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); - } - tmp = root; - res = __d_path(path, &tmp, buf, buflen); + res = __d_path(path, &root, &bastard, buf, buflen); *name = res; /* handle error conditions - and still allow a partial path to @@ -97,10 +90,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 @@ -111,9 +101,9 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, * of chroot) and specifically directed to connect paths to * namespace root. */ - if (!connected) { + if (bastard.mnt && (root.mnt || !our_mnt(bastard.mnt)) { /* is the disconnect path a sysctl? */ - if (tmp.dentry->d_sb->s_magic == PROC_SUPER_MAGIC && + if (bastard.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 +111,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(bastard.mnt))) { /* disconnected path, don't return pathname starting * with '/' */ @@ -133,7 +122,10 @@ static int d_namespace_path(struct path *path, char *buf, int buflen, } out: - path_put(&root); + if (bastard.mnt) + path_put(&bastard); + if (flags & PATH_CHROOT_REL) + path_put(&root); return error; } diff --git a/security/tomoyo/realpath.c b/security/tomoyo/realpath.c index 738bbdf..99ee841 100644 --- a/security/tomoyo/realpath.c +++ b/security/tomoyo/realpath.c @@ -103,7 +103,7 @@ static char *tomoyo_get_absolute_path(struct path *path, char * const buffer, 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_path(path, &ns_root, NULL, buffer, buflen - 1); if (!IS_ERR(pos) && *pos == '/' && pos[1]) { struct inode *inode = path->dentry->d_inode; if (inode && S_ISDIR(inode->i_mode)) { ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-06 3:58 ` Al Viro @ 2011-12-06 4:41 ` Tetsuo Handa 2011-12-06 5:20 ` Al Viro 2011-12-06 7:07 ` James Morris 1 sibling, 1 reply; 8+ messages in thread From: Tetsuo Handa @ 2011-12-06 4:41 UTC (permalink / raw) To: viro; +Cc: linux-fsdevel > Cc: stable@vger.kernel.org Please add version you meant to apply. By the way, why do we need to call like write_seqlock(&rename_lock); error = prepend_path(path, root, &res, &buflen); write_sequnlock(&rename_lock); ? I think it would look something like do { seq = read_seqbegin(&rename_lock); error = prepend_path(path, root, &res, &buflen); } while (read_seqretry(&rename_lock, seq)); because prepend_path() is for reading dentry tree rather than updating ... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-06 4:41 ` Tetsuo Handa @ 2011-12-06 5:20 ` Al Viro 2011-12-06 6:45 ` Tetsuo Handa 0 siblings, 1 reply; 8+ messages in thread From: Al Viro @ 2011-12-06 5:20 UTC (permalink / raw) To: Tetsuo Handa; +Cc: linux-fsdevel On Tue, Dec 06, 2011 at 01:41:21PM +0900, Tetsuo Handa wrote: > > Cc: stable@vger.kernel.org > Please add version you meant to apply. Everything since mid 2010, I'm afraid - that's when apparmor went in and it had that bug back then. IOW, 2.6.36 and everything later... Tomoyo bits will need a backport for stuff older that 3.0 - there had been changes around 3.0-rc2 or so. > By the way, why do we need to call like > > write_seqlock(&rename_lock); > error = prepend_path(path, root, &res, &buflen); > write_sequnlock(&rename_lock); > > ? I think it would look something like > > do { > seq = read_seqbegin(&rename_lock); > error = prepend_path(path, root, &res, &buflen); > } while (read_seqretry(&rename_lock, seq)); > > because prepend_path() is for reading dentry tree rather than updating ... I really don't like the idea. Look what's inside that function; it'll grab/release ->d_lock on a bunch of dentries, in addition to grabbing and releasing vfsmount_lock. We _might_ be able to get away with that if we just use rcu_read_lock() instead of ->d_lock, but I would very much like to see the data before doing that kind of change. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-06 5:20 ` Al Viro @ 2011-12-06 6:45 ` Tetsuo Handa 2011-12-06 15:49 ` Al Viro 0 siblings, 1 reply; 8+ messages in thread From: Tetsuo Handa @ 2011-12-06 6:45 UTC (permalink / raw) To: viro; +Cc: john.johansen, linux-fsdevel Al Viro wrote: > On Tue, Dec 06, 2011 at 01:41:21PM +0900, Tetsuo Handa wrote: > > > Cc: stable@vger.kernel.org > > Please add version you meant to apply. > > Everything since mid 2010, I'm afraid - that's when apparmor went in and > it had that bug back then. IOW, 2.6.36 and everything later... > > Tomoyo bits will need a backport for stuff older that 3.0 - there had > been changes around 3.0-rc2 or so. I see. | +bool our_mnt(struct vfsmount *mnt) | +{ | + return check_mnt(mnt); | +} | +EXPORT_SYMBOL(our_mnt); Seems that our_mnt() need not to be exported because only AppArmor calls it and LSM modules cannot be built as a loadable kernel module. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-06 6:45 ` Tetsuo Handa @ 2011-12-06 15:49 ` Al Viro 0 siblings, 0 replies; 8+ messages in thread From: Al Viro @ 2011-12-06 15:49 UTC (permalink / raw) To: Tetsuo Handa; +Cc: john.johansen, linux-fsdevel On Tue, Dec 06, 2011 at 03:45:04PM +0900, Tetsuo Handa wrote: > | +bool our_mnt(struct vfsmount *mnt) > | +{ > | + return check_mnt(mnt); > | +} > | +EXPORT_SYMBOL(our_mnt); > > Seems that our_mnt() need not to be exported because only AppArmor calls it and > LSM modules cannot be built as a loadable kernel module. Point. OK, that export killed, pull request sent to Linus... ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) 2011-12-06 3:58 ` Al Viro 2011-12-06 4:41 ` Tetsuo Handa @ 2011-12-06 7:07 ` James Morris 1 sibling, 0 replies; 8+ messages in thread From: James Morris @ 2011-12-06 7:07 UTC (permalink / raw) To: Al Viro; +Cc: John Johansen, linux-kernel, linux-security-module, linux-fsdevel On Tue, 6 Dec 2011, Al Viro wrote: > the patch survives his testing] > > Reviewed-by: John Johansen <john.johansen@canonical.com> > Signed-off-by: Al Viro <viro@zeniv.linux.org.uk> > Cc: stable@vger.kernel.org Reviewed-by: James Morris <jmorris@namei.org> -- James Morris <jmorris@namei.org> ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2011-12-06 15:49 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <1323034020-28780-1-git-send-email-john.johansen@canonical.com>
[not found] ` <20111204221020.GB2203@ZenIV.linux.org.uk>
2011-12-05 3:27 ` [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path) Al Viro
2011-12-06 2:34 ` John Johansen
2011-12-06 3:58 ` Al Viro
2011-12-06 4:41 ` Tetsuo Handa
2011-12-06 5:20 ` Al Viro
2011-12-06 6:45 ` Tetsuo Handa
2011-12-06 15:49 ` Al Viro
2011-12-06 7:07 ` James Morris
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).