Messages in this thread | | | Subject | Re: [PATCH] [RFC] fs: Possible filp_open race experiment | From | Marek Vasut <> | Date | Tue, 31 Jan 2017 06:29:36 +0100 |
| |
+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
| |