lkml.org 
[lkml]   [2011]   [Dec]   [6]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [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;
    }

    \
     
     \ /
      Last update: 2011-12-07 01:41    [W:0.054 / U:0.124 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site