| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <20111207003922.GO2203@ZenIV.linux.org.uk>
Date: Wed, 7 Dec 2011 00:39:22 +0000
From: Al Viro <viro@...IV.linux.org.uk>
To: Linus Torvalds <torvalds@...ux-foundation.org>
Cc: John Johansen <john.johansen@...onical.com>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] apparmor fix for __d_path() misuse
On 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;
}
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists