linux-kernel - Re: [PATCH 1/1] fix d_revalidate oopsen on NFS exports

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-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20111201072947.GA10329@onthe.net.au>
Date:	Thu, 1 Dec 2011 18:29:47 +1100
From:	Chris Dunlop <chris@...he.net.au>
To:	Tyler Hicks <tyhicks@...onical.com>
Cc:	David Howells <dhowells@...hat.com>,
	Al Viro <viro@...iv.linux.org.uk>,
	"Myklebust, Trond" <Trond.Myklebust@...app.com>,
	linux-fsdevel@...r.kernel.org, linux-kernel@...r.kernel.org,
	Eric Van Hensbergen <ericvh@...il.com>,
	Ron Minnich <rminnich@...dia.gov>,
	Latchesar Ionkov <lucho@...kov.net>,
	Jan Harkes <jaharkes@...cmu.edu>,
	"maintainer:CODA FILE SYSTEM" <coda@...cmu.edu>,
	Dave Kleikamp <shaggy@...nel.org>,
	Petr Vandrovec <petr@...drovec.name>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	v9fs-developer@...ts.sourceforge.net,
	linux-afs@...ts.infradead.org, codalist@...EMANN.coda.cs.cmu.edu,
	jfs-discussion@...ts.sourceforge.net, linux-nfs@...r.kernel.org
Subject: Re: [PATCH 1/1] fix d_revalidate oopsen on NFS exports

On Thu, Dec 01, 2011 at 12:31:58AM -0600, Tyler Hicks wrote:
> On 2011-12-01 11:47:09, Chris Dunlop wrote:
>> On Wed, Nov 30, 2011 at 08:54:43AM +0000, David Howells wrote:
>>> Chris Dunlop <chris@...he.net.au> wrote:
>>> 
>>>> To avoid other people further wasting their and your time on
>>>> exactly the same thing future, how something like the following
>>>> patch, based on your comment in:
>>>> 
>>>> http://article.gmane.org/gmane.linux.nfs/40370
>>>> 
>>>> ...and, if that's acceptable, is it worthwhile doing for the
>>>> other file systems which are likewise currently vulnerable when
>>>> abused by broken layered file systems?
>>> 
>>> Also, this may get fixed by Al's atomic open patches - but obviously it hasn't
>>> been yet...
>>> 
>>>> Don't oops when abused by broken layered file systems
>>>> 
>>>> Signed-off-by: Chris Dunlop <chris@...he.net.au>
>>> 
>>> Acked-by: David Howells <dhowells@...hat.com>
>>> 
>>> It's also worth printing a message - this *is* a kernel bug of some description
>>> if it happens.
>> 
>> Like the below?  This covers the d_revalidate for 9p, afs, coda,
>> hfs, ncpfs, proc, sysfs.
> 
> I don't like the looks of this patch. It makes sense for NFS to error
> out of d_revalidate() when passed a NULL nameidata pointer because NFS
> actually uses the nameidata to do something useful. That can't be said
> about the other filesystems in this patch.

I can see nd is used in nfs_open_revalidate(), but is it
necessarily used in nfs_lookup_revalidate()?  I'm way out of my
depth here, but everywhere it's used in nfs_lookup_revalidate()
(nfs_neg_need_reval(), nfs_is_exclusive_create(),
nfs_lookup_verify_inode()) there are also checks for nd != NULL.

> Why not handle the other filesystems like the previous fixes you
> referenced in your original email by checking for a non-NULL nd like
> this:
> 
> 	if (nd && nd->flags & LOOKUP_RCU)
> 		return -ECHILD;

'Cos Trond scared me into it!  ;-)

But mostly because I don't really know what I'm doing. The
original patch came about because I was tracking down the Oops
in the NFS code and it seemed such an obvious fix that
lookup_one_len() passes down a hard-coded NULL and that NULL
isn't checked in all the d_revalidate routines. I thought I'd do
the right thing and make sure it was checked everywhere. Little
did I know there's "history" behind it! I'm afraid I don't know
anywhere near enough to argue about the right way to deal with it.

> I'm also not sure about the printk in the NFS case. Instead of littering
> the logs, we should probably just disallow the stacked filesystem (are
> we talking about eCryptfs here?) from mounting on top of NFS in the
> first place.

See other reply: it wasn't a stacked file system.

But it seems useful to have the d_revalidate routines indicate
via the log that they're being abused.

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