Fix refcounting problem in walk_symlink()
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>
diff --git a/kern/src/ns/chan.c b/kern/src/ns/chan.c
index 3929813..02f11fd 100644
--- a/kern/src/ns/chan.c
+++ b/kern/src/ns/chan.c
@@ -1684,6 +1684,7 @@
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 @@
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);
