Re: [PATCH] [RFC] fs: Possible filp_open race experiment

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Marek Vasut]

+CC Greg, LKML as I don't quite know where this should go.

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).
> 
> 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.:
> 
> $ 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. 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.
> 
> Note that when using DT overlays (whose configfs interface is not yet
> mainline), there can easily be a device which requires a firmware in
> the DT overlay. Such device will invoke firmware load, which uses the
> filp_open() and will thus trigger the behavior above. Depending on
> whether one uses dd or cat, the kernel will either crash or not.
> 
> Any ideas ?
> 
> Signed-off-by: Marek Vasut <marex@denx.de>
> Cc: Pantelis Antoniou <pantelis.antoniou@konsulko.com>
> ---
> NOTE: I don't usually dig in these areas of the kernel, do we have a
>       generic FS list for this sort of discussion ? Thus far, sending
>       it off-list. Maybe the description could also use some fleshing
>       out. Thanks
> ---
>  drivers/of/Makefile   |   2 +-
>  drivers/of/configfs.c | 149 ++++++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 150 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/of/configfs.c
> 
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index d7efd9d458aa..a1698bf819b5 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,4 +1,4 @@
> -obj-y = base.o device.o platform.o
> +obj-y = base.o device.o platform.o configfs.o
>  obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
>  obj-$(CONFIG_OF_FLATTREE) += fdt.o
>  obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
> diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
> new file mode 100644
> index 000000000000..407016e22209
> --- /dev/null
> +++ b/drivers/of/configfs.c
> @@ -0,0 +1,149 @@
> +/*
> + * Possible race in filp_open
> + *
> + * Copyright (C) 2017 Marek Vasut <marex@denx.de>
> + *
> + * test based on Configfs entries for DTOs
> + *
> + * Copyright (C) 2013 - Pantelis Antoniou <panto@antoniou-consulting.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * as published by the Free Software Foundation; either version
> + * 2 of the License, or (at your option) any later version.
> + */
> +#include <linux/module.h>
> +#include <linux/configfs.h>
> +#include <linux/types.h>
> +#include <linux/file.h>
> +#include <linux/slab.h>
> +#include <linux/sched.h>
> +
> +static struct configfs_attribute *cfs_overlay_attrs[] = {
> +	NULL,
> +};
> +
> +ssize_t cfs_overlay_item_dtbo_read(struct config_item *item, void *buf,
> +		size_t max_count)
> +{
> +	return 0;
> +}
> +
> +ssize_t cfs_overlay_item_dtbo_write(struct config_item *item, const void *buf,
> +		size_t count)
> +{
> +	WARN_ON(!current->fs);
> +	/*
> +	 * If anything here triggers filp_open(), kernel may crash.
> +	 * The filp_open() is triggered ie. by firmware loading.
> +	 *
> +	 * 1) The following example will crash because cat exits too quickly
> +	 *    and fs_exit() is called before this code is executed, so no FS
> +	 *    access is available anymore.
> +	 * $ mkdir /sys/kernel/config/test/overlays/1/dtbo
> +	 * $ cat file_17201_bytes_long > /sys/kernel/config/test/overlays/1/dtbo
> +	 *
> +	 *    Using "dmesg" instead of cat file_17201_bytes_long works too.
> +	 *    Any file length over 16 kiB will work, 17201 Bytes long file is
> +	 *    what I used:
> +	 *
> +	 * $ dmesg > /sys/kernel/config/test/overlays/1/dtbo
> +	 *
> +	 * 2) The follow example will not crash because dd exits slightly slower
> +	 *    and thus the fs_exit() is called after this code is executed and
> +	 *    the FS access is still available.
> +	 * $ mkdir /sys/kernel/config/test/overlays/1/dtbo
> +	 * $ dd if=file_17201_bytes_long of=/sys/kernel/config/test/overlays/1/dtbo
> +	 *
> +	 *   Same example with dmesg, which does not crash:
> +	 *
> +	 * $ dmesg | dd of=/sys/kernel/config/test/overlays/1/dtbo
> +	 */
> +	return -EINVAL;
> +}
> +
> +CONFIGFS_BIN_ATTR(cfs_overlay_item_, dtbo, NULL, SZ_1M);
> +
> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
> +	&cfs_overlay_item_attr_dtbo,
> +	NULL,
> +};
> +
> +static struct config_item_type cfs_overlay_type = {
> +	.ct_attrs	= cfs_overlay_attrs,
> +	.ct_bin_attrs	= cfs_overlay_bin_attrs,
> +	.ct_owner	= THIS_MODULE,
> +};
> +
> +static struct config_item *cfs_overlay_group_make_item(
> +		struct config_group *group, const char *name)
> +{
> +	struct config_item *item;
> +
> +	item = kzalloc(sizeof(*item), GFP_KERNEL);
> +	if (!item)
> +		return ERR_PTR(-ENOMEM);
> +
> +	config_item_init_type_name(item, name, &cfs_overlay_type);
> +	return item;
> +}
> +
> +static void cfs_overlay_group_drop_item(struct config_group *group,
> +		struct config_item *item)
> +{
> +	config_item_put(item);
> +}
> +
> +static struct configfs_group_operations overlays_ops = {
> +	.make_item	= cfs_overlay_group_make_item,
> +	.drop_item	= cfs_overlay_group_drop_item,
> +};
> +
> +static struct config_item_type overlays_type = {
> +	.ct_group_ops   = &overlays_ops,
> +	.ct_owner       = THIS_MODULE,
> +};
> +
> +static struct configfs_group_operations of_cfs_ops = {
> +	/* empty - we don't allow anything to be created */
> +};
> +
> +static struct config_item_type of_cfs_type = {
> +	.ct_group_ops   = &of_cfs_ops,
> +	.ct_owner       = THIS_MODULE,
> +};
> +
> +struct config_group of_cfs_overlay_group;
> +
> +static struct configfs_subsystem of_cfs_subsys = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_namebuf = "test",
> +			.ci_type = &of_cfs_type,
> +		},
> +	},
> +	.su_mutex = __MUTEX_INITIALIZER(of_cfs_subsys.su_mutex),
> +};
> +
> +static int __init of_cfs_init(void)
> +{
> +	int ret;
> +
> +	pr_info("%s\n", __func__);
> +
> +	config_group_init(&of_cfs_subsys.su_group);
> +	config_group_init_type_name(&of_cfs_overlay_group, "overlays",
> +			&overlays_type);
> +	configfs_add_default_group(&of_cfs_overlay_group,
> +			&of_cfs_subsys.su_group);
> +
> +	ret = configfs_register_subsystem(&of_cfs_subsys);
> +	if (ret != 0) {
> +		pr_err("%s: failed to register subsys\n", __func__);
> +		goto out;
> +	}
> +	pr_info("%s: OK\n", __func__);
> +out:
> +	return ret;
> +}
> +late_initcall(of_cfs_init);
> 


-- 
Best regards,
Marek Vasut

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Greg Kroah-Hartman]

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?

> 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?

> > 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?

> > $ 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?

> > 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?

> > 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? :)

> > there can easily be a device which requires a firmware in
> > the DT overlay. Such device will invoke firmware load, which uses the
> > filp_open() and will thus trigger the behavior above. Depending on
> > whether one uses dd or cat, the kernel will either crash or not.
> > 
> > Any ideas ?

I think you need to fix your device tree overlay code...

thanks,

greg k-h

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Marek Vasut]

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.

>>> 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.

>>> there can easily be a device which requires a firmware in
>>> the DT overlay. Such device will invoke firmware load, which uses the
>>> filp_open() and will thus trigger the behavior above. Depending on
>>> whether one uses dd or cat, the kernel will either crash or not.
>>>
>>> Any ideas ?
> 
> I think you need to fix your device tree overlay code...

This is not related to DTO, I only use that to trigger the problem.

-- 
Best regards,
Marek Vasut

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Greg Kroah-Hartman]

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

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Christoph Hellwig]

On Tue, Jan 31, 2017 at 11:21:02AM +0100, Greg Kroah-Hartman wrote:
> 
> -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...)

It's really a mismatched assumption.  The configfs binary file
code just chunks updates up into a buffer, which only gets flushed
at ->release time.  If we'd move that to ->flush the issue Marek reports
would be fixed.

But I don't think we want that - triggering a filp_open from the update
of a _binary_ attribute for a start is wrong.  And second doing this
using ->fs of a random calling process is bound to cause problems.

I think he is using the wrong kind of interface for the job.

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Greg Kroah-Hartman]

On Tue, Jan 31, 2017 at 01:58:14PM +0100, Christoph Hellwig wrote:
> On Tue, Jan 31, 2017 at 11:21:02AM +0100, Greg Kroah-Hartman wrote:
> > 
> > -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...)
> 
> It's really a mismatched assumption.  The configfs binary file
> code just chunks updates up into a buffer, which only gets flushed
> at ->release time.  If we'd move that to ->flush the issue Marek reports
> would be fixed.
> 
> But I don't think we want that - triggering a filp_open from the update
> of a _binary_ attribute for a start is wrong.  And second doing this
> using ->fs of a random calling process is bound to cause problems.
> 
> I think he is using the wrong kind of interface for the job.

Ah, that's why no one has seen this before :)

So, the DT overlay code needs to be fixed...

thanks,

greg k-h

Re: [PATCH] [RFC] fs: Possible filp_open race experiment

[Marek Vasut]

On 01/31/2017 02:17 PM, Greg Kroah-Hartman wrote:
> On Tue, Jan 31, 2017 at 01:58:14PM +0100, Christoph Hellwig wrote:
>> On Tue, Jan 31, 2017 at 11:21:02AM +0100, Greg Kroah-Hartman wrote:
>>>
>>> -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...)
>>
>> It's really a mismatched assumption.  The configfs binary file
>> code just chunks updates up into a buffer, which only gets flushed
>> at ->release time.  If we'd move that to ->flush the issue Marek reports
>> would be fixed.
>>
>> But I don't think we want that - triggering a filp_open from the update
>> of a _binary_ attribute for a start is wrong.  And second doing this
>> using ->fs of a random calling process is bound to cause problems.
>>
>> I think he is using the wrong kind of interface for the job.
> 
> Ah, that's why no one has seen this before :)
> 
> So, the DT overlay code needs to be fixed...

Well I ran into the issue when I loaded DTO using 'cat' which bound a
driver which required firmware and the firmware loader uses the
filp_open() to load the firmware file from the FS. This crashed my
kernel because the current->fs was NULL. The example I provided is a
stripped down version which checks the current->fs directly to make
things simpler.

I think the issue is in the firmware loader (or filp_open() itself?) .
Shouldn't the filp_open() somehow assure that the current->fs is valid
if it is used within instead of triggering a NULL ptr dereference when
called from ie. the configfs binary attribute write callback ?

-- 
Best regards,
Marek Vasut