+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 <ma...@denx.de>
> Cc: Pantelis Antoniou <pantelis.anton...@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 <ma...@denx.de>
> + *
> + * test based on Configfs entries for DTOs
> + *
> + * Copyright (C) 2013 - Pantelis Antoniou <pa...@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
