linux-kernel - Re: fs: NULL deref in atime_needs

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 for Android: free password hash cracker in your pocket

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

Message-ID: <14804.1456753436@warthog.procyon.org.uk>
Date:	Mon, 29 Feb 2016 13:43:56 +0000
From:	David Howells <dhowells@...hat.com>
To:	Al Viro <viro@...IV.linux.org.uk>
Cc:	dhowells@...hat.com, Dmitry Vyukov <dvyukov@...gle.com>,
	Ian Kent <raven@...maw.net>,
	Mickaël Salaün <mic@...ikod.net>,
	"linux-fsdevel@...r.kernel.org" <linux-fsdevel@...r.kernel.org>,
	LKML <linux-kernel@...r.kernel.org>,
	syzkaller <syzkaller@...glegroups.com>,
	Kostya Serebryany <kcc@...gle.com>,
	Alexander Potapenko <glider@...gle.com>,
	Sasha Levin <sasha.levin@...cle.com>,
	Linus Torvalds <torvalds@...ux-foundation.org>
Subject: Re: fs: NULL deref in atime_needs_update

Al Viro <viro@...IV.linux.org.uk> wrote:

> David, Linus, do you see any problems with that?  To me it looks saner
> that way and as cheap as the current code, but I might be missing something 
> here...

You're effectively converting to this:

	read d_seq.sequence
	smp_rmb()
	read d_inode, d_flags
	smp_rmb()
	check d_seq.sequence

in the read path and this:

	write d_seq.sequence
	smp_wmb()
	write d_inode, d_flags
	smp_wmb()
	write d_seq.sequence

This should work - especially if we're wangling these sequence points anyway,
and so have to pay the barrier penalties whatever.  In fact, you actually take
a barrier out, I think.

I have had a problem with getting the ordering of d_inode and d_flags right
because of __d_clear_type_and_inode() where we're required to unset a dentry
so that it can be repurposed[*] as a negative dentry rather than replacing it.

This is something we for performance sake - and it's something we can only do
if the dentry isn't referenced, whereas if we properly followed the RCU model,
we would have to wait a grace period after delisting an unlinked dentry before
we could repurpose it - but that significantly slows down rename, unlink and
rmdir.

[*] Al and I disagree on whether this is a reuse or merely a change of state.

David