Fix refcounting problem in walk_symlink()
authorBarret Rhoden <brho@cs.berkeley.edu>
Mon, 8 Apr 2019 13:19:16 +0000 (09:19 -0400)
committerBarret Rhoden <brho@cs.berkeley.edu>
Mon, 8 Apr 2019 13:48:36 +0000 (09:48 -0400)
The old code would double-close 'symlink' if it ever got into the "walk
succeeded but we were still on a symlink" case.

Recall that walk_symlink() either walks all the way, or not at all.  If
the old code (deleted) failed, we were supposed to close symlink.
However, there was a case where walk_symlink would fail, but it would
also close the chan passed in: when the sub-walk succeeded, but
walk_symlink() still failed.  How could that fail?  With a symlink loop.

If that sounds confusing, it's because it is.  There are actually two
spots of recursion, which might not even have been clear to me when I
wrote this.  The main recursive path is walk -> walk_symlink -> walk ->
walk_symlink.  We normally never got to the "walk succeeded, then call
walk_symlink" (deleted).  We usually only called walk_symlink() from
*within* walk itself.

You could trigger this situation with a no_follow symlink
(rename/remove, flags Aremove, flags no_follow).  Syzkaller basically
did this:
ln -s x x
rename x/x whatever
So we were walking the first x, had no_follow set, so walk ->
walk_symlink for the first x.  Since there were more names in the
main/outer namec, we'd get past the first no_follow.  Then that second
walk() would be for "../x", where no_follow would kick in, since there
were no longer any names left.  That walk_symlink() would return the
syml passed in, which appeared to walk to be a success (which it was).

Anyway, this was calling the buggy post-successful-walk() part of
walk_symlink().  The first fix I had was to only cclose(symlink) when that
interior walk_symlink() succeeded.  Although that fixed the bug
(walk_symlink either succeeds xor closes, so don't close on success),
the real problem was having that code at all.

If walk() lands on a symlink, walk itself should try to deal with it (by
calling walk_symlink()).  If walk returns (success or failure), we
should consider the walking of that symlink to be done, one way or
another.  If it is no_follow, we might be on a symlink (if last in the
path).  If it was a mount_point, we could be on a symlink too.

That whole "we're still on a symlink, let's walk it again" was busted,
and it would break when we had no_follow set globally and tried to
follow a legit intermediate symlink that had more than one link to
follow (but not 8 - the max).  And it was confusing.  Hopefully this
code makes a little more sense - esp when realizing that walk() sorts
out symlinks by calling walk_symlink().  walk_symlink() shouldn't call
itself, but it can call walk().  Up to 8 times.

Reported-by: syzbot+9eec51df84779065d6de@syzkaller.appspotmail.com
Signed-off-by: Barret Rhoden <brho@cs.berkeley.edu>
kern/src/ns/chan.c

index 3929813..02f11fd 100644 (file)
@@ -1684,6 +1684,7 @@ static struct chan *walk_symlink(struct chan *symlink, struct walk_helper *wh,
        struct dir *dir;
        char *link_name, *link_store;
        struct chan *from;
+       bool old_nofollow;
        Elemlist e = {0};
 
        /* mildly expensive: need to rlock the namespace */
@@ -1727,18 +1728,27 @@ static struct chan *walk_symlink(struct chan *symlink, struct walk_helper *wh,
        kfree(link_store);
 
        wh->nr_loops++;
+       /* no_follow applies to the outermost walk, i.e. the one that the
+        * original namec performs.  At this point, we've decided that we're
+        * going to try and follow a symlink: even if its no_follow, that only
+        * applies to the last link in the original path.  Our sub-walks are not
+        * no_follow.
+        *
+        * Note the other wh vars need to stay with the walk: nr_loops,
+        * since its our method of detecting symlink loops, and can_mount, which
+        * is a property of the overall namec() call. */
+       old_nofollow = wh->no_follow;
+       wh->no_follow = false;
        if (walk(&from, e.elems, e.ARRAY_SIZEs, wh, NULL) < 0) {
                cclose(from);
                from = NULL;
        } else {
+               /* We can still have a successful walk and have the new 'from'
+                * be a symlink.  We'd need walk_symlink to return a symlink
+                * chan, which happens if the symlink is a mount point. */
                cclose(symlink);
-               if (from->qid.type & QTSYMLINK) {
-                       symlink = from;
-                       from = walk_symlink(symlink, wh, nr_names_left);
-                       if (!from)
-                               cclose(symlink);
-               }
        }
+       wh->no_follow = old_nofollow;
        wh->nr_loops--;
 
        kfree(e.name);