linux-kernel - fs/namei.c: Misuse of sequence counts?

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-ID: <20141011225808.GA20777@zzz>
Date:	Sat, 11 Oct 2014 17:58:08 -0500
From:	Eric Biggers <ebiggers3@...il.com>
To:	linux-fsdevel@...r.kernel.org
Cc:	viro@...iv.linux.org.uk, linux-kernel@...r.kernel.org
Subject: fs/namei.c: Misuse of sequence counts?

Hi, I've been reading through the path lookup code and I believe there may have
been bugs introduced by commit 4023bfc9 ("be careful with nd->inode in
path_init() and follow_dotdot_rcu()").  And I may have found a pre-existing bug
as well.

In follow_dotdot_rcu(), said commit moved loads of the inode to just before
read_seqcount_begin(), in several instances.  I don't think this is correct,
because (as I understand it) read_seqcount_begin() is opening a seq-read
critical section on the new dentry.  So the inode load should come *after* it,
as in the original, to ensure the inode pointer is correctly matched with the
sequence count.

In path_init(), said commit added a call to read_seqcount_retry() after loading
the inode.  I see two problems with this:

  - The read_seqcount_retry() isn't needed just to load the inode pointer, so
    the change doesn't seem to accomplish anything.
  - If the -ECHILD code path actually runs, the reference to the 'struct file'
    can be leaked.

Also: if there were actual problems that were "fixed" by this commit, I wonder
if they were/are actually caused by the fd-relative case in path_init() using:

	nd->seq = __read_seqcount_begin(&nd->path.dentry->d_seq);

instead of

	nd->seq = read_seqcount_begin(&nd->path.dentry->d_seq);

since the former is missing a memory barrier before the starting inode is
loaded.

Eric
--
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/