lkml.org 
[lkml]   [2011]   [Dec]   [5]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patch in this message
    /
    Date
    From
    SubjectRe: [RFC] __d_path() API change (was Re: [PATCH] Remove use of mnt_ns->root and fix a couple of bugs in d_namespace_path)
    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;
    > }


    \
     
     \ /
      Last update: 2011-12-06 03:37    [W:0.039 / U:0.016 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site