| 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
| ||
|
Date: Tue, 06 Dec 2011 20:26:09 -0800
From: John Johansen <john.johansen@...onical.com>
To: John Johansen <john.johansen@...onical.com>
CC: Al Viro <viro@...IV.linux.org.uk>,
Linus Torvalds <torvalds@...ux-foundation.org>,
linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org
Subject: Re: [git pull] apparmor fix for __d_path() misuse
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@...onical.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@...r.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at http://www.tux.org/lkml/
>
--
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