fs: Possible filp_open race experiment

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 <[email protected]>

Cc Pantelis Antoniou <[email protected]>

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

drivers/of/Makefile +1 -1

index d7efd9d458aa..a1698bf819b5 100644
@@ -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

drivers/of/configfs.c new file +149 -0

new file mode 100644
index 000000000000..407016e22209
@@ -0,0 +1,149 @@
+/*
+ * Possible race in filp_open
+ *
+ * Copyright (C) 2017 Marek Vasut <[email protected]>
+ *
+ * test based on Configfs entries for DTOs
+ *
+ * Copyright (C) 2013 - Pantelis Antoniou <[email protected]>
+ *
+ * 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);

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

Quoted 6 lines

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

Quoted 3 lines

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

Quoted 5 lines

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

Quoted 5 lines

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

Quoted 8 lines

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.

Quoted 2 lines

Note that when using DT overlays (whose configfs interface is not yet mainline),

Quoted 6 lines

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 ?

Quoted 1 line · Marek Vasut

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

You do know about linux-fsdevel, right?

Quoted 7 lines · Marek Vasut

Quoted 6 lines

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?

Quoted 3 lines

Quoted 3 lines

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?

Quoted 5 lines

Quoted 5 lines

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

Quoted 9 lines

Quoted 9 lines

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

Quoted 4 lines

Quoted 4 lines

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?

Quoted 2 lines

Quoted 2 lines

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

Quoted 6 lines

Quoted 6 lines

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

Quoted 4 lines · Greg Kroah-Hartman

Quoted 1 line · Marek Vasut

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

Quoted 8 lines

Quoted 7 lines · Marek Vasut

Quoted 6 lines

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

Quoted 3 lines

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.

Quoted 5 lines

Quoted 3 lines

Quoted 3 lines

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

Quoted 7 lines

Quoted 5 lines

Quoted 5 lines

$ 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().

Quoted 9 lines

Quoted 9 lines

Quoted 9 lines

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

Quoted 7 lines

Quoted 4 lines

Quoted 4 lines

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.

Quoted 5 lines

Quoted 2 lines

Quoted 2 lines

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

Quoted 6 lines · Greg Kroah-Hartman

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

Quoted 17 lines · Christoph Hellwig

Quoted 6 lines · Greg Kroah-Hartman

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

Quoted 1 line

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