Message-ID: <16b3fb38-0dfc-2d3c-7cfd-681b15432fb7@denx.de>
Date:   Tue, 31 Jan 2017 06:29:36 +0100
From:   Marek Vasut <marex@...x.de>
To:     "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Cc:     hch@....de, Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Subject: Re: [PATCH] [RFC] fs: Possible filp_open race experiment

+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@...x.de>
> Cc: Pantelis Antoniou <pantelis.antoniou@...sulko.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@...x.de>
> + *
> + * test based on Configfs entries for DTOs
> + *
> + * Copyright (C) 2013 - Pantelis Antoniou <panto@...oniou-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

Powered by blists - more mailing lists