Message-ID: <20170131102102.GA5349@kroah.com>
Date:   Tue, 31 Jan 2017 11:21:02 +0100
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Marek Vasut <marex@...x.de>
Cc:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        hch@....de, Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Joel Becker <jlbec@...lplan.org>
Subject: Re: [PATCH] [RFC] fs: Possible filp_open race experiment

On Tue, Jan 31, 2017 at 11:08:05AM +0100, Marek Vasut wrote:
> On 01/31/2017 08:05 AM, Greg Kroah-Hartman wrote:
> > On Tue, Jan 31, 2017 at 06:29:36AM +0100, Marek Vasut wrote:
> >> +CC Greg, LKML as I don't quite know where this should go.
> > 
> > You do know about linux-fsdevel, right?
> 
> No, wasn't aware of it, sorry.
> 
> >> On 01/18/2017 12:16 AM, Marek Vasut wrote:
> >>> I believe there is a possible race condition when configfs attributes
> >>> trigger filp_open() from the kernel. I initially observed the problem
> >>> on Linux 4.4 when loading DT overlay , which in turn loads a driver
> >>> which loads firmware. After some further investigation, I came up with
> >>> the following minimal-ish example patch, which can trigger the same
> >>> behavior on Linux 4.10-rc4 (next 20170117).
> > 
> > What in-kernel code causes this problem?  I didn't think DT overlays
> > were a feature in 4.4, are you running with code that isn't in the
> > normal releases?
> 
> No, it happens in -next as well. I believe if write into configfs binary
> attribute triggers filp_open(), the kernel will crash.

Any specific configfs binary file in-tree that this happens to?

> >>> The core of the demo is in cfs_over_item_dtbo_write(), which just checks
> >>> for valid current->fs . This function is triggered by writing data into
> >>> configfs binary attribute, ie.:
> > 
> > Why are you caring about current->fs?
> 
> Because that is what's NULL and is referenced (in set_root_rcu()) when
> the configfs binary attribute is written and triggers filp_open() .
> 
> >>> $ mkdir /sys/kernel/config/test/overlays/1/dtbo
> >>> $ cat file_17201_bytes_long > /sys/kernel/config/test/overlays/1/dtbo
> >>>
> >>> I believe the 'cat' program exits quickly and thus calls fs_exit()
> >>> before the cfs_over_item_dtbo_write() is called.
> > 
> > How can exit be called before write?
> 
> I believe the exit happens after write, but this function
> cfs_over_item_dtbo_write() is entered only after the fs_exit().
> 
> >>> Any attempts to
> >>> access FS (like ie. loading firmware from FS) from that function will
> >>> therefore fail (by crashing the kernel, NULL pointer dereference in
> >>> set_root_rcu() in fs/namei.c).
> >>>
> >>> On the other hand, replacing 'cat' with 'dd' yields different result:
> >>>
> >>> $ dd if=file_17201_bytes_long of=/sys/kernel/config/test/overlays/1/dtbo
> >>>
> >>> The kernel does not crash. I believe this is because dd takes slightly
> >>> longer to complete, so the cfs_over_item_dtbo_write() can complete
> >>> before the dd process gets to calling fs_exit() and so the filesystem
> >>> access is still available, thus current->fs is valid.
> > 
> > cat and dd act differently, if you strace them, it should show the
> > differences, perhaps you can narrow it down there?
> 
> I can try.
> 
> >>> Note that when using DT overlays (whose configfs interface is not yet
> >>> mainline),
> > 
> > Ah, we can't do anything about code that is not merged, perhaps it is
> > just buggy? :)
> 
> The configfs stuff is in -next , how is it not merged ? The code below
> is an example that triggers the problem.

-next isn't Linus's tree, sometimes stuff sits in there for years :)

Anyway, if this is a configfs issue, Christoph and Joel can take a look
at it.  Any reason you didn't cc: Joel as well (the MAINTAINERS file is
your friend...)

thanks,

greg k-h

Powered by blists - more mailing lists