All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] OF: DT-Overlay configfs interface (v3)
@ 2014-12-03 11:23 Pantelis Antoniou
       [not found] ` <1417605808-23327-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2014-12-03 11:23 UTC (permalink / raw)
  To: Grant Likely
  Cc: Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou,
	Pantelis Antoniou

Add a runtime interface to using configfs for generic device tree overlay
usage. With it its possible to use device tree overlays without having
to use a per-platform overlay manager.

Please see Documentation/devicetree/configfs-overlays.txt for more info.

Changes since v2:
- Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
- Created a documentation entry
- Slight rewording in Kconfig

Changes since v1:
- of_resolve() -> of_resolve_phandles().

Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
---
 Documentation/devicetree/configfs-overlays.txt |  31 +++
 drivers/of/Kconfig                             |   7 +
 drivers/of/Makefile                            |   1 +
 drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
 4 files changed, 371 insertions(+)
 create mode 100644 Documentation/devicetree/configfs-overlays.txt
 create mode 100644 drivers/of/configfs.c

diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
new file mode 100644
index 0000000..5fa43e0
--- /dev/null
+++ b/Documentation/devicetree/configfs-overlays.txt
@@ -0,0 +1,31 @@
+Howto use the configfs overlay interface.
+
+A device-tree configfs entry is created in /config/device-tree/overlays
+and and it is manipulated using standard file system I/O.
+Note that this is a debug level interface, for use by developers and
+not necessarily something accessed by normal users due to the
+security implications of having direct access to the kernel's device tree.
+
+* To create an overlay you mkdir the directory:
+
+	# mkdir /config/device-tree/overlays/foo
+
+* Either you echo the overlay firmware file to the path property file.
+
+	# echo foo.dtbo >/config/device-tree/overlays/foo/path
+
+* Or you cat the contents of the overlay to the dtbo file
+
+	# cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
+
+The overlay file will be applied, and devices will be created/destroyed
+as required.
+
+To remove it simply rmdir the directory.
+
+	# rmdir /config/device-tree/overlays/foo
+
+The rationalle of the dual interface (firmware & direct copy) is that each is
+better suited to different use patterns. The firmware interface is what's
+intended to be used by hardware managers in the kernel, while the copy interface
+make sense for developers (since it avoids problems with namespaces).
diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 18b2e25..5d36306 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -91,4 +91,11 @@ config OF_OVERLAY
 	select OF_DEVICE
 	select OF_RESOLVE
 
+config OF_CONFIGFS
+	bool "Device Tree Overlay ConfigFS interface"
+	select CONFIGFS_FS
+	select OF_OVERLAY
+	help
+	  Enable a simple user-space driven DT overlay interface.
+
 endmenu # OF
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index 7563f36..6f7f4f8 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -1,4 +1,5 @@
 obj-y = base.o device.o platform.o
+obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
--- /dev/null
+++ b/drivers/of/configfs.c
@@ -0,0 +1,332 @@
+/*
+ * Configfs entries for device-tree
+ *
+ * Copyright (C) 2013 - Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
+ *
+ * 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/ctype.h>
+#include <linux/cpu.h>
+#include <linux/module.h>
+#include <linux/of.h>
+#include <linux/of_fdt.h>
+#include <linux/spinlock.h>
+#include <linux/slab.h>
+#include <linux/proc_fs.h>
+#include <linux/configfs.h>
+#include <linux/types.h>
+#include <linux/stat.h>
+#include <linux/limits.h>
+#include <linux/file.h>
+#include <linux/vmalloc.h>
+#include <linux/firmware.h>
+
+#include "of_private.h"
+
+struct cfs_overlay_item {
+	struct config_item	item;
+
+	char			path[PATH_MAX];
+
+	const struct firmware	*fw;
+	struct device_node	*overlay;
+	int			ov_id;
+
+	void			*dtbo;
+	int			dtbo_size;
+};
+
+static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
+{
+	int err;
+
+	/* unflatten the tree */
+	of_fdt_unflatten_tree(blob, &overlay->overlay);
+	if (overlay->overlay == NULL) {
+		pr_err("%s: failed to unflatten tree\n", __func__);
+		err = -EINVAL;
+		goto out_err;
+	}
+	pr_debug("%s: unflattened OK\n", __func__);
+
+	/* mark it as detached */
+	of_node_set_flag(overlay->overlay, OF_DETACHED);
+
+	/* perform resolution */
+	err = of_resolve_phandles(overlay->overlay);
+	if (err != 0) {
+		pr_err("%s: Failed to resolve tree\n", __func__);
+		goto out_err;
+	}
+	pr_debug("%s: resolved OK\n", __func__);
+
+	err = of_overlay_create(overlay->overlay);
+	if (err < 0) {
+		pr_err("%s: Failed to create overlay (err=%d)\n",
+				__func__, err);
+		goto out_err;
+	}
+	overlay->ov_id = err;
+
+out_err:
+	return err;
+}
+
+static inline struct cfs_overlay_item *to_cfs_overlay_item(
+		struct config_item *item)
+{
+	return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
+}
+
+CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
+#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)	\
+struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
+	__CONFIGFS_ATTR(_name, _mode, _show, _store)
+#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)	\
+struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
+	__CONFIGFS_ATTR_RO(_name, _show)
+
+CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
+#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
+struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
+	__CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
+#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)	\
+struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
+	__CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
+
+static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
+		char *page)
+{
+	return sprintf(page, "%s\n", overlay->path);
+}
+
+static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
+		const char *page, size_t count)
+{
+	const char *p = page;
+	char *s;
+	int err;
+
+	/* if it's set do not allow changes */
+	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
+		return -EPERM;
+
+	/* copy to path buffer (and make sure it's always zero terminated */
+	count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
+	overlay->path[sizeof(overlay->path) - 1] = '\0';
+
+	/* strip trailing newlines */
+	s = overlay->path + strlen(overlay->path);
+	while (s > overlay->path && *--s == '\n')
+		*s = '\0';
+
+	pr_debug("%s: path is '%s'\n", __func__, overlay->path);
+
+	err = request_firmware(&overlay->fw, overlay->path, NULL);
+	if (err != 0)
+		goto out_err;
+
+	err = create_overlay(overlay, (void *)overlay->fw->data);
+	if (err != 0)
+		goto out_err;
+
+	return count;
+
+out_err:
+
+	release_firmware(overlay->fw);
+	overlay->fw = NULL;
+
+	overlay->path[0] = '\0';
+	return err;
+}
+
+static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
+		char *page)
+{
+	return sprintf(page, "%s\n",
+			overlay->ov_id >= 0 ? "applied" : "unapplied");
+}
+
+CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
+		cfs_overlay_item_path_show, cfs_overlay_item_path_store);
+CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
+
+static struct configfs_attribute *cfs_overlay_attrs[] = {
+	&cfs_overlay_item_attr_path.attr,
+	&cfs_overlay_item_attr_status.attr,
+	NULL,
+};
+
+ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
+		void *buf, size_t max_count)
+{
+	pr_debug("%s: buf=%p max_count=%u\n", __func__,
+			buf, max_count);
+
+	if (overlay->dtbo == NULL)
+		return 0;
+
+	/* copy if buffer provided */
+	if (buf != NULL) {
+		/* the buffer must be large enough */
+		if (overlay->dtbo_size > max_count)
+			return -ENOSPC;
+
+		memcpy(buf, overlay->dtbo, overlay->dtbo_size);
+	}
+
+	return overlay->dtbo_size;
+}
+
+ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
+		const void *buf, size_t count)
+{
+	int err;
+
+	/* if it's set do not allow changes */
+	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
+		return -EPERM;
+
+	/* copy the contents */
+	overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
+	if (overlay->dtbo == NULL)
+		return -ENOMEM;
+
+	overlay->dtbo_size = count;
+
+	err = create_overlay(overlay, overlay->dtbo);
+	if (err != 0)
+		goto out_err;
+
+	return count;
+
+out_err:
+	kfree(overlay->dtbo);
+	overlay->dtbo = NULL;
+	overlay->dtbo_size = 0;
+
+	return err;
+}
+
+CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
+		cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
+		NULL, SZ_1M);
+
+static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
+	&cfs_overlay_item_bin_attr_dtbo.bin_attr,
+	NULL,
+};
+
+static void cfs_overlay_release(struct config_item *item)
+{
+	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
+
+	if (overlay->ov_id >= 0)
+		of_overlay_destroy(overlay->ov_id);
+	if (overlay->fw)
+		release_firmware(overlay->fw);
+	/* kfree with NULL is safe */
+	kfree(overlay->dtbo);
+	kfree(overlay);
+}
+
+CONFIGFS_ATTR_OPS(cfs_overlay_item);
+CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
+static struct configfs_item_operations cfs_overlay_item_ops = {
+	.release		= cfs_overlay_release,
+	.show_attribute		= cfs_overlay_item_attr_show,
+	.store_attribute	= cfs_overlay_item_attr_store,
+	.read_bin_attribute	= cfs_overlay_item_bin_attr_read,
+	.write_bin_attribute	= cfs_overlay_item_bin_attr_write,
+};
+
+static struct config_item_type cfs_overlay_type = {
+	.ct_item_ops	= &cfs_overlay_item_ops,
+	.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 cfs_overlay_item *overlay;
+
+	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
+	if (!overlay)
+		return ERR_PTR(-ENOMEM);
+	overlay->ov_id = -1;
+
+	config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
+	return &overlay->item;
+}
+
+static void cfs_overlay_group_drop_item(struct config_group *group,
+		struct config_item *item)
+{
+	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
+
+	config_item_put(&overlay->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;
+
+struct config_group *of_cfs_def_groups[] = {
+	&of_cfs_overlay_group,
+	NULL
+};
+
+static struct configfs_subsystem of_cfs_subsys = {
+	.su_group = {
+		.cg_item = {
+			.ci_namebuf = "device-tree",
+			.ci_type = &of_cfs_type,
+		},
+		.default_groups = of_cfs_def_groups,
+	},
+	.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);
+
+	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);
-- 
1.7.12

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply related	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found] ` <1417605808-23327-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
@ 2014-12-04 10:31   ` Grant Likely
       [not found]     ` <20141204103104.7D8E0C40992-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
  2015-05-13 14:40   ` Geert Uytterhoeven
  2015-05-13 15:31   ` Geert Uytterhoeven
  2 siblings, 1 reply; 16+ messages in thread
From: Grant Likely @ 2014-12-04 10:31 UTC (permalink / raw)
  To: Kees Cook
  Cc: Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou,
	Pantelis Antoniou

Kees,

I'm copying you on this email. This is a new interface being proposed
for making live changes to the device tree at runtime, which gives
pretty close to direct control to userspace of the Linux device model.
Userspace can supply a blob that causes devices to get added or removed
from /sys/devices, and can change how a device is accessed. This is
definitely a security concern and I would love to get your opinion on
the interface.

Naturally, the interface has been made root-only, but I don't think that
is enough. I think we need a method which parts of the devicetree can be
modified by a devicetree overlay. I've been thinking in terms of adding
allow/deny properties that control whether or not changes can be made
below a particular node, with the default for the whole tree to be
'deny'. That way the devicetree for the platform can restrict dynamic
change to only the node that need to be modifed.

What do you think?

Pantelis, I've made comments below on the patch...

On Wed,  3 Dec 2014 13:23:28 +0200
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> Add a runtime interface to using configfs for generic device tree overlay
> usage. With it its possible to use device tree overlays without having
> to use a per-platform overlay manager.
> 
> Please see Documentation/devicetree/configfs-overlays.txt for more info.
> 
> Changes since v2:
> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
> - Created a documentation entry
> - Slight rewording in Kconfig
> 
> Changes since v1:
> - of_resolve() -> of_resolve_phandles().
> 
> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> ---
>  Documentation/devicetree/configfs-overlays.txt |  31 +++
>  drivers/of/Kconfig                             |   7 +
>  drivers/of/Makefile                            |   1 +
>  drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
>  4 files changed, 371 insertions(+)
>  create mode 100644 Documentation/devicetree/configfs-overlays.txt
>  create mode 100644 drivers/of/configfs.c
> 
> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
> new file mode 100644
> index 0000000..5fa43e0
> --- /dev/null
> +++ b/Documentation/devicetree/configfs-overlays.txt
> @@ -0,0 +1,31 @@
> +Howto use the configfs overlay interface.
> +
> +A device-tree configfs entry is created in /config/device-tree/overlays
> +and and it is manipulated using standard file system I/O.
> +Note that this is a debug level interface, for use by developers and
> +not necessarily something accessed by normal users due to the
> +security implications of having direct access to the kernel's device tree.
> +
> +* To create an overlay you mkdir the directory:
> +
> +	# mkdir /config/device-tree/overlays/foo
> +
> +* Either you echo the overlay firmware file to the path property file.
> +
> +	# echo foo.dtbo >/config/device-tree/overlays/foo/path
> +
> +* Or you cat the contents of the overlay to the dtbo file
> +
> +	# cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
> +
> +The overlay file will be applied, and devices will be created/destroyed
> +as required.
> +
> +To remove it simply rmdir the directory.
> +
> +	# rmdir /config/device-tree/overlays/foo
> +
> +The rationalle of the dual interface (firmware & direct copy) is that each is

sp. rationale

> +better suited to different use patterns. The firmware interface is what's
> +intended to be used by hardware managers in the kernel, while the copy interface
> +make sense for developers (since it avoids problems with namespaces).

The distinction between developer and production interfaces is
unfortunately meaningless in the real world. The developer interface
will get used in production if it is there, and having two interfaces
means we have to support two interfaces that do /exactly the same
thing/.

One interface only please.

What are the problems with namespaces that you see with the firmware
interface?

> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> index 18b2e25..5d36306 100644
> --- a/drivers/of/Kconfig
> +++ b/drivers/of/Kconfig
> @@ -91,4 +91,11 @@ config OF_OVERLAY
>  	select OF_DEVICE
>  	select OF_RESOLVE
>  
> +config OF_CONFIGFS
> +	bool "Device Tree Overlay ConfigFS interface"
> +	select CONFIGFS_FS
> +	select OF_OVERLAY
> +	help
> +	  Enable a simple user-space driven DT overlay interface.
> +
>  endmenu # OF
> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> index 7563f36..6f7f4f8 100644
> --- a/drivers/of/Makefile
> +++ b/drivers/of/Makefile
> @@ -1,4 +1,5 @@
>  obj-y = base.o device.o platform.o
> +obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
> --- /dev/null
> +++ b/drivers/of/configfs.c
> @@ -0,0 +1,332 @@
> +/*
> + * Configfs entries for device-tree
> + *
> + * Copyright (C) 2013 - Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> + *
> + * 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/ctype.h>
> +#include <linux/cpu.h>
> +#include <linux/module.h>
> +#include <linux/of.h>
> +#include <linux/of_fdt.h>
> +#include <linux/spinlock.h>
> +#include <linux/slab.h>
> +#include <linux/proc_fs.h>
> +#include <linux/configfs.h>
> +#include <linux/types.h>
> +#include <linux/stat.h>
> +#include <linux/limits.h>
> +#include <linux/file.h>
> +#include <linux/vmalloc.h>
> +#include <linux/firmware.h>
> +
> +#include "of_private.h"
> +
> +struct cfs_overlay_item {
> +	struct config_item	item;
> +
> +	char			path[PATH_MAX];

Hard coded maximum path length?

> +
> +	const struct firmware	*fw;
> +	struct device_node	*overlay;
> +	int			ov_id;
> +
> +	void			*dtbo;
> +	int			dtbo_size;
> +};
> +
> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
> +{
> +	int err;
> +
> +	/* unflatten the tree */
> +	of_fdt_unflatten_tree(blob, &overlay->overlay);
> +	if (overlay->overlay == NULL) {
> +		pr_err("%s: failed to unflatten tree\n", __func__);
> +		err = -EINVAL;
> +		goto out_err;
> +	}
> +	pr_debug("%s: unflattened OK\n", __func__);
> +
> +	/* mark it as detached */
> +	of_node_set_flag(overlay->overlay, OF_DETACHED);
> +
> +	/* perform resolution */
> +	err = of_resolve_phandles(overlay->overlay);
> +	if (err != 0) {
> +		pr_err("%s: Failed to resolve tree\n", __func__);
> +		goto out_err;
> +	}
> +	pr_debug("%s: resolved OK\n", __func__);
> +
> +	err = of_overlay_create(overlay->overlay);
> +	if (err < 0) {
> +		pr_err("%s: Failed to create overlay (err=%d)\n",
> +				__func__, err);
> +		goto out_err;
> +	}
> +	overlay->ov_id = err;
> +
> +out_err:
> +	return err;
> +}
> +
> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
> +		struct config_item *item)
> +{
> +	return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
> +}
> +
> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)	\
> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> +	__CONFIGFS_ATTR(_name, _mode, _show, _store)
> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)	\
> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> +	__CONFIGFS_ATTR_RO(_name, _show)
> +
> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
> +	__CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)	\
> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
> +	__CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
> +
> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
> +		char *page)
> +{
> +	return sprintf(page, "%s\n", overlay->path);
> +}
> +
> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
> +		const char *page, size_t count)
> +{
> +	const char *p = page;
> +	char *s;
> +	int err;
> +
> +	/* if it's set do not allow changes */
> +	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> +		return -EPERM;
> +
> +	/* copy to path buffer (and make sure it's always zero terminated */
> +	count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
> +	overlay->path[sizeof(overlay->path) - 1] = '\0';

kmemdup perhaps to get rid of the hard coded path length?

> +
> +	/* strip trailing newlines */
> +	s = overlay->path + strlen(overlay->path);
> +	while (s > overlay->path && *--s == '\n')
> +		*s = '\0';

strchr() will find trailing \n's for you.

> +
> +	pr_debug("%s: path is '%s'\n", __func__, overlay->path);
> +
> +	err = request_firmware(&overlay->fw, overlay->path, NULL);
> +	if (err != 0)
> +		goto out_err;
> +
> +	err = create_overlay(overlay, (void *)overlay->fw->data);
> +	if (err != 0)
> +		goto out_err;
> +
> +	return count;
> +
> +out_err:
> +
> +	release_firmware(overlay->fw);
> +	overlay->fw = NULL;
> +
> +	overlay->path[0] = '\0';
> +	return err;
> +}
> +
> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
> +		char *page)
> +{
> +	return sprintf(page, "%s\n",
> +			overlay->ov_id >= 0 ? "applied" : "unapplied");
> +}
> +
> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
> +		cfs_overlay_item_path_show, cfs_overlay_item_path_store);
> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
> +
> +static struct configfs_attribute *cfs_overlay_attrs[] = {
> +	&cfs_overlay_item_attr_path.attr,
> +	&cfs_overlay_item_attr_status.attr,
> +	NULL,
> +};
> +
> +ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
> +		void *buf, size_t max_count)
> +{
> +	pr_debug("%s: buf=%p max_count=%u\n", __func__,
> +			buf, max_count);
> +
> +	if (overlay->dtbo == NULL)
> +		return 0;
> +
> +	/* copy if buffer provided */
> +	if (buf != NULL) {
> +		/* the buffer must be large enough */
> +		if (overlay->dtbo_size > max_count)
> +			return -ENOSPC;

I've not dug into configfs code. Is it not possible to do a partial
read?

> +
> +		memcpy(buf, overlay->dtbo, overlay->dtbo_size);
> +	}
> +
> +	return overlay->dtbo_size;
> +}
> +
> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
> +		const void *buf, size_t count)
> +{
> +	int err;
> +
> +	/* if it's set do not allow changes */
> +	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> +		return -EPERM;
> +
> +	/* copy the contents */
> +	overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
> +	if (overlay->dtbo == NULL)
> +		return -ENOMEM;
> +
> +	overlay->dtbo_size = count;
> +
> +	err = create_overlay(overlay, overlay->dtbo);
> +	if (err != 0)
> +		goto out_err;
> +
> +	return count;
> +
> +out_err:
> +	kfree(overlay->dtbo);
> +	overlay->dtbo = NULL;
> +	overlay->dtbo_size = 0;
> +
> +	return err;
> +}
> +
> +CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
> +		cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
> +		NULL, SZ_1M);
> +
> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
> +	&cfs_overlay_item_bin_attr_dtbo.bin_attr,
> +	NULL,
> +};
> +
> +static void cfs_overlay_release(struct config_item *item)
> +{
> +	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
> +
> +	if (overlay->ov_id >= 0)
> +		of_overlay_destroy(overlay->ov_id);
> +	if (overlay->fw)
> +		release_firmware(overlay->fw);
> +	/* kfree with NULL is safe */
> +	kfree(overlay->dtbo);
> +	kfree(overlay);
> +}
> +
> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
> +CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
> +static struct configfs_item_operations cfs_overlay_item_ops = {
> +	.release		= cfs_overlay_release,
> +	.show_attribute		= cfs_overlay_item_attr_show,
> +	.store_attribute	= cfs_overlay_item_attr_store,
> +	.read_bin_attribute	= cfs_overlay_item_bin_attr_read,
> +	.write_bin_attribute	= cfs_overlay_item_bin_attr_write,
> +};
> +
> +static struct config_item_type cfs_overlay_type = {
> +	.ct_item_ops	= &cfs_overlay_item_ops,
> +	.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 cfs_overlay_item *overlay;
> +
> +	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> +	if (!overlay)
> +		return ERR_PTR(-ENOMEM);
> +	overlay->ov_id = -1;
> +
> +	config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
> +	return &overlay->item;
> +}
> +
> +static void cfs_overlay_group_drop_item(struct config_group *group,
> +		struct config_item *item)
> +{
> +	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
> +
> +	config_item_put(&overlay->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;
> +
> +struct config_group *of_cfs_def_groups[] = {
> +	&of_cfs_overlay_group,
> +	NULL
> +};
> +
> +static struct configfs_subsystem of_cfs_subsys = {
> +	.su_group = {
> +		.cg_item = {
> +			.ci_namebuf = "device-tree",
> +			.ci_type = &of_cfs_type,
> +		},
> +		.default_groups = of_cfs_def_groups,
> +	},
> +	.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);
> +
> +	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);
> -- 
> 1.7.12
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]     ` <20141204103104.7D8E0C40992-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
@ 2014-12-04 11:30       ` Pantelis Antoniou
       [not found]         ` <8795CB40-87D6-4683-8396-987A49E94574-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
  2014-12-05 19:53       ` Kees Cook
  1 sibling, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2014-12-04 11:30 UTC (permalink / raw)
  To: Grant Likely
  Cc: Kees Cook, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Hi Grant,

> On Dec 4, 2014, at 12:31 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> Kees,
> 
> I'm copying you on this email. This is a new interface being proposed
> for making live changes to the device tree at runtime, which gives
> pretty close to direct control to userspace of the Linux device model.
> Userspace can supply a blob that causes devices to get added or removed
> from /sys/devices, and can change how a device is accessed. This is
> definitely a security concern and I would love to get your opinion on
> the interface.
> 
> Naturally, the interface has been made root-only, but I don't think that
> is enough. I think we need a method which parts of the devicetree can be
> modified by a devicetree overlay. I've been thinking in terms of adding
> allow/deny properties that control whether or not changes can be made
> below a particular node, with the default for the whole tree to be
> 'deny'. That way the devicetree for the platform can restrict dynamic
> change to only the node that need to be modifed.
> 
> What do you think?
> 
> Pantelis, I've made comments below on the patch...
> 
> On Wed,  3 Dec 2014 13:23:28 +0200
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> wrote:
>> Add a runtime interface to using configfs for generic device tree overlay
>> usage. With it its possible to use device tree overlays without having
>> to use a per-platform overlay manager.
>> 
>> Please see Documentation/devicetree/configfs-overlays.txt for more info.
>> 
>> Changes since v2:
>> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
>> - Created a documentation entry
>> - Slight rewording in Kconfig
>> 
>> Changes since v1:
>> - of_resolve() -> of_resolve_phandles().
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>> Documentation/devicetree/configfs-overlays.txt |  31 +++
>> drivers/of/Kconfig                             |   7 +
>> drivers/of/Makefile                            |   1 +
>> drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
>> 4 files changed, 371 insertions(+)
>> create mode 100644 Documentation/devicetree/configfs-overlays.txt
>> create mode 100644 drivers/of/configfs.c
>> 
>> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
>> new file mode 100644
>> index 0000000..5fa43e0
>> --- /dev/null
>> +++ b/Documentation/devicetree/configfs-overlays.txt
>> @@ -0,0 +1,31 @@
>> +Howto use the configfs overlay interface.
>> +
>> +A device-tree configfs entry is created in /config/device-tree/overlays
>> +and and it is manipulated using standard file system I/O.
>> +Note that this is a debug level interface, for use by developers and
>> +not necessarily something accessed by normal users due to the
>> +security implications of having direct access to the kernel's device tree.
>> +
>> +* To create an overlay you mkdir the directory:
>> +
>> +	# mkdir /config/device-tree/overlays/foo
>> +
>> +* Either you echo the overlay firmware file to the path property file.
>> +
>> +	# echo foo.dtbo >/config/device-tree/overlays/foo/path
>> +
>> +* Or you cat the contents of the overlay to the dtbo file
>> +
>> +	# cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
>> +
>> +The overlay file will be applied, and devices will be created/destroyed
>> +as required.
>> +
>> +To remove it simply rmdir the directory.
>> +
>> +	# rmdir /config/device-tree/overlays/foo
>> +
>> +The rationalle of the dual interface (firmware & direct copy) is that each is
> 
> sp. rationale
> 

OK,

>> +better suited to different use patterns. The firmware interface is what's
>> +intended to be used by hardware managers in the kernel, while the copy interface
>> +make sense for developers (since it avoids problems with namespaces).
> 
> The distinction between developer and production interfaces is
> unfortunately meaningless in the real world. The developer interface
> will get used in production if it is there, and having two interfaces
> means we have to support two interfaces that do /exactly the same
> thing/.
> 
> One interface only please.
> 

I wish it was so simple :/


> What are the problems with namespaces that you see with the firmware
> interface?
> 

When passing a path to a driver, which namespace are you going to use
to perform the lookup?

For instance, if the user issued a load request for a path in a container
which is the namespace under which the firmware file is located? Is it the
non-container filesystem, or is it under the container filesystem that
issued the request. The firmware layer does not have an API to discern.


>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 18b2e25..5d36306 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -91,4 +91,11 @@ config OF_OVERLAY
>> 	select OF_DEVICE
>> 	select OF_RESOLVE
>> 
>> +config OF_CONFIGFS
>> +	bool "Device Tree Overlay ConfigFS interface"
>> +	select CONFIGFS_FS
>> +	select OF_OVERLAY
>> +	help
>> +	  Enable a simple user-space driven DT overlay interface.
>> +
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 7563f36..6f7f4f8 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,4 +1,5 @@
>> obj-y = base.o device.o platform.o
>> +obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
>> --- /dev/null
>> +++ b/drivers/of/configfs.c
>> @@ -0,0 +1,332 @@
>> +/*
>> + * Configfs entries for device-tree
>> + *
>> + * 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/ctype.h>
>> +#include <linux/cpu.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/slab.h>
>> +#include <linux/proc_fs.h>
>> +#include <linux/configfs.h>
>> +#include <linux/types.h>
>> +#include <linux/stat.h>
>> +#include <linux/limits.h>
>> +#include <linux/file.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/firmware.h>
>> +
>> +#include "of_private.h"
>> +
>> +struct cfs_overlay_item {
>> +	struct config_item	item;
>> +
>> +	char			path[PATH_MAX];
> 
> Hard coded maximum path length?
> 
>> +
>> +	const struct firmware	*fw;
>> +	struct device_node	*overlay;
>> +	int			ov_id;
>> +
>> +	void			*dtbo;
>> +	int			dtbo_size;
>> +};
>> +
>> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
>> +{
>> +	int err;
>> +
>> +	/* unflatten the tree */
>> +	of_fdt_unflatten_tree(blob, &overlay->overlay);
>> +	if (overlay->overlay == NULL) {
>> +		pr_err("%s: failed to unflatten tree\n", __func__);
>> +		err = -EINVAL;
>> +		goto out_err;
>> +	}
>> +	pr_debug("%s: unflattened OK\n", __func__);
>> +
>> +	/* mark it as detached */
>> +	of_node_set_flag(overlay->overlay, OF_DETACHED);
>> +
>> +	/* perform resolution */
>> +	err = of_resolve_phandles(overlay->overlay);
>> +	if (err != 0) {
>> +		pr_err("%s: Failed to resolve tree\n", __func__);
>> +		goto out_err;
>> +	}
>> +	pr_debug("%s: resolved OK\n", __func__);
>> +
>> +	err = of_overlay_create(overlay->overlay);
>> +	if (err < 0) {
>> +		pr_err("%s: Failed to create overlay (err=%d)\n",
>> +				__func__, err);
>> +		goto out_err;
>> +	}
>> +	overlay->ov_id = err;
>> +
>> +out_err:
>> +	return err;
>> +}
>> +
>> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
>> +		struct config_item *item)
>> +{
>> +	return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
>> +}
>> +
>> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
>> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)	\
>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>> +	__CONFIGFS_ATTR(_name, _mode, _show, _store)
>> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)	\
>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>> +	__CONFIGFS_ATTR_RO(_name, _show)
>> +
>> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
>> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>> +	__CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
>> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)	\
>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>> +	__CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
>> +
>> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
>> +		char *page)
>> +{
>> +	return sprintf(page, "%s\n", overlay->path);
>> +}
>> +
>> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
>> +		const char *page, size_t count)
>> +{
>> +	const char *p = page;
>> +	char *s;
>> +	int err;
>> +
>> +	/* if it's set do not allow changes */
>> +	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +		return -EPERM;
>> +
>> +	/* copy to path buffer (and make sure it's always zero terminated */
>> +	count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
>> +	overlay->path[sizeof(overlay->path) - 1] = '\0';
> 
> kmemdup perhaps to get rid of the hard coded path length?
> 

Yeah, I guess, it’s no biggie.

>> +
>> +	/* strip trailing newlines */
>> +	s = overlay->path + strlen(overlay->path);
>> +	while (s > overlay->path && *--s == '\n')
>> +		*s = '\0';
> 
> strchr() will find trailing \n's for you.
> 

Perhaps it shouldn’t be stripping at all, and require echo -n
 
>> +
>> +	pr_debug("%s: path is '%s'\n", __func__, overlay->path);
>> +
>> +	err = request_firmware(&overlay->fw, overlay->path, NULL);
>> +	if (err != 0)
>> +		goto out_err;
>> +
>> +	err = create_overlay(overlay, (void *)overlay->fw->data);
>> +	if (err != 0)
>> +		goto out_err;
>> +
>> +	return count;
>> +
>> +out_err:
>> +
>> +	release_firmware(overlay->fw);
>> +	overlay->fw = NULL;
>> +
>> +	overlay->path[0] = '\0';
>> +	return err;
>> +}
>> +
>> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
>> +		char *page)
>> +{
>> +	return sprintf(page, "%s\n",
>> +			overlay->ov_id >= 0 ? "applied" : "unapplied");
>> +}
>> +
>> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
>> +		cfs_overlay_item_path_show, cfs_overlay_item_path_store);
>> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
>> +
>> +static struct configfs_attribute *cfs_overlay_attrs[] = {
>> +	&cfs_overlay_item_attr_path.attr,
>> +	&cfs_overlay_item_attr_status.attr,
>> +	NULL,
>> +};
>> +
>> +ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
>> +		void *buf, size_t max_count)
>> +{
>> +	pr_debug("%s: buf=%p max_count=%u\n", __func__,
>> +			buf, max_count);
>> +
>> +	if (overlay->dtbo == NULL)
>> +		return 0;
>> +
>> +	/* copy if buffer provided */
>> +	if (buf != NULL) {
>> +		/* the buffer must be large enough */
>> +		if (overlay->dtbo_size > max_count)
>> +			return -ENOSPC;
> 
> I've not dug into configfs code. Is it not possible to do a partial
> read?
> 

The whole point of configfs is that the data are delivered in one-go.
It makes the users of configfs considerably simpler.

Otherwise there’s no point in it and you can just use sysfs.

>> +
>> +		memcpy(buf, overlay->dtbo, overlay->dtbo_size);
>> +	}
>> +
>> +	return overlay->dtbo_size;
>> +}
>> +
>> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
>> +		const void *buf, size_t count)
>> +{
>> +	int err;
>> +
>> +	/* if it's set do not allow changes */
>> +	if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +		return -EPERM;
>> +
>> +	/* copy the contents */
>> +	overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
>> +	if (overlay->dtbo == NULL)
>> +		return -ENOMEM;
>> +
>> +	overlay->dtbo_size = count;
>> +
>> +	err = create_overlay(overlay, overlay->dtbo);
>> +	if (err != 0)
>> +		goto out_err;
>> +
>> +	return count;
>> +
>> +out_err:
>> +	kfree(overlay->dtbo);
>> +	overlay->dtbo = NULL;
>> +	overlay->dtbo_size = 0;
>> +
>> +	return err;
>> +}
>> +
>> +CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
>> +		cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
>> +		NULL, SZ_1M);
>> +
>> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
>> +	&cfs_overlay_item_bin_attr_dtbo.bin_attr,
>> +	NULL,
>> +};
>> +
>> +static void cfs_overlay_release(struct config_item *item)
>> +{
>> +	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>> +
>> +	if (overlay->ov_id >= 0)
>> +		of_overlay_destroy(overlay->ov_id);
>> +	if (overlay->fw)
>> +		release_firmware(overlay->fw);
>> +	/* kfree with NULL is safe */
>> +	kfree(overlay->dtbo);
>> +	kfree(overlay);
>> +}
>> +
>> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
>> +CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
>> +static struct configfs_item_operations cfs_overlay_item_ops = {
>> +	.release		= cfs_overlay_release,
>> +	.show_attribute		= cfs_overlay_item_attr_show,
>> +	.store_attribute	= cfs_overlay_item_attr_store,
>> +	.read_bin_attribute	= cfs_overlay_item_bin_attr_read,
>> +	.write_bin_attribute	= cfs_overlay_item_bin_attr_write,
>> +};
>> +
>> +static struct config_item_type cfs_overlay_type = {
>> +	.ct_item_ops	= &cfs_overlay_item_ops,
>> +	.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 cfs_overlay_item *overlay;
>> +
>> +	overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
>> +	if (!overlay)
>> +		return ERR_PTR(-ENOMEM);
>> +	overlay->ov_id = -1;
>> +
>> +	config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
>> +	return &overlay->item;
>> +}
>> +
>> +static void cfs_overlay_group_drop_item(struct config_group *group,
>> +		struct config_item *item)
>> +{
>> +	struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>> +
>> +	config_item_put(&overlay->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;
>> +
>> +struct config_group *of_cfs_def_groups[] = {
>> +	&of_cfs_overlay_group,
>> +	NULL
>> +};
>> +
>> +static struct configfs_subsystem of_cfs_subsys = {
>> +	.su_group = {
>> +		.cg_item = {
>> +			.ci_namebuf = "device-tree",
>> +			.ci_type = &of_cfs_type,
>> +		},
>> +		.default_groups = of_cfs_def_groups,
>> +	},
>> +	.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);
>> +
>> +	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);
>> -- 
>> 1.7.12

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]         ` <8795CB40-87D6-4683-8396-987A49E94574-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
@ 2014-12-05 16:44           ` Grant Likely
       [not found]             ` <20141205164450.A9294C40B61-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Grant Likely @ 2014-12-05 16:44 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Kees Cook, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Thu, 4 Dec 2014 13:30:05 +0200
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> Hi Grant,
> 
> > On Dec 4, 2014, at 12:31 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> > 
> > Kees,
> > 
> > I'm copying you on this email. This is a new interface being proposed
> > for making live changes to the device tree at runtime, which gives
> > pretty close to direct control to userspace of the Linux device model.
> > Userspace can supply a blob that causes devices to get added or removed
> > from /sys/devices, and can change how a device is accessed. This is
> > definitely a security concern and I would love to get your opinion on
> > the interface.
> > 
> > Naturally, the interface has been made root-only, but I don't think that
> > is enough. I think we need a method which parts of the devicetree can be
> > modified by a devicetree overlay. I've been thinking in terms of adding
> > allow/deny properties that control whether or not changes can be made
> > below a particular node, with the default for the whole tree to be
> > 'deny'. That way the devicetree for the platform can restrict dynamic
> > change to only the node that need to be modifed.
> > 
> > What do you think?
> > 
> > Pantelis, I've made comments below on the patch...
> > 
> > On Wed,  3 Dec 2014 13:23:28 +0200
> > , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> > wrote:
> >> Add a runtime interface to using configfs for generic device tree overlay
> >> usage. With it its possible to use device tree overlays without having
> >> to use a per-platform overlay manager.
> >> 
> >> Please see Documentation/devicetree/configfs-overlays.txt for more info.
> >> 
> >> Changes since v2:
> >> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
> >> - Created a documentation entry
> >> - Slight rewording in Kconfig
> >> 
> >> Changes since v1:
> >> - of_resolve() -> of_resolve_phandles().
> >> 
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> ---
> >> Documentation/devicetree/configfs-overlays.txt |  31 +++
> >> drivers/of/Kconfig                             |   7 +
> >> drivers/of/Makefile                            |   1 +
> >> drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
> >> 4 files changed, 371 insertions(+)
> >> create mode 100644 Documentation/devicetree/configfs-overlays.txt
> >> create mode 100644 drivers/of/configfs.c
> >> 
> >> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
> >> new file mode 100644
> >> index 0000000..5fa43e0
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/configfs-overlays.txt
> >> @@ -0,0 +1,31 @@
> >> +Howto use the configfs overlay interface.
> >> +
> >> +A device-tree configfs entry is created in /config/device-tree/overlays
> >> +and and it is manipulated using standard file system I/O.
> >> +Note that this is a debug level interface, for use by developers and
> >> +not necessarily something accessed by normal users due to the
> >> +security implications of having direct access to the kernel's device tree.
> >> +
> >> +* To create an overlay you mkdir the directory:
> >> +
> >> +	# mkdir /config/device-tree/overlays/foo
> >> +
> >> +* Either you echo the overlay firmware file to the path property file.
> >> +
> >> +	# echo foo.dtbo >/config/device-tree/overlays/foo/path
> >> +
> >> +* Or you cat the contents of the overlay to the dtbo file
> >> +
> >> +	# cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
> >> +
> >> +The overlay file will be applied, and devices will be created/destroyed
> >> +as required.
> >> +
> >> +To remove it simply rmdir the directory.
> >> +
> >> +	# rmdir /config/device-tree/overlays/foo
> >> +
> >> +The rationalle of the dual interface (firmware & direct copy) is that each is
> > 
> > sp. rationale
> > 
> 
> OK,
> 
> >> +better suited to different use patterns. The firmware interface is what's
> >> +intended to be used by hardware managers in the kernel, while the copy interface
> >> +make sense for developers (since it avoids problems with namespaces).
> > 
> > The distinction between developer and production interfaces is
> > unfortunately meaningless in the real world. The developer interface
> > will get used in production if it is there, and having two interfaces
> > means we have to support two interfaces that do /exactly the same
> > thing/.
> > 
> > One interface only please.
> > 
> 
> I wish it was so simple :/
> 
> 
> > What are the problems with namespaces that you see with the firmware
> > interface?
> > 
> 
> When passing a path to a driver, which namespace are you going to use
> to perform the lookup?
> 
> For instance, if the user issued a load request for a path in a container
> which is the namespace under which the firmware file is located? Is it the
> non-container filesystem, or is it under the container filesystem that
> issued the request. The firmware layer does not have an API to discern.

Wait, what? Why on earth would you want to expose this interface in a container?

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]             ` <20141205164450.A9294C40B61-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
@ 2014-12-05 17:58               ` Pantelis Antoniou
       [not found]                 ` <E64BA68B-A339-4BB2-B453-B8A9165CB9B7-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2014-12-05 17:58 UTC (permalink / raw)
  To: Grant Likely
  Cc: Kees Cook, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Hi Grant,

> On Dec 5, 2014, at 18:44 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>> 
[snip]

>> For instance, if the user issued a load request for a path in a container
>> which is the namespace under which the firmware file is located? Is it the
>> non-container filesystem, or is it under the container filesystem that
>> issued the request. The firmware layer does not have an API to discern.
> 
> Wait, what? Why on earth would you want to expose this interface in a container?
> 

It was a complaint raised. 

Look at this thread.

http://permalink.gmane.org/gmane.linux.documentation/27655



> g.
> 

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]     ` <20141204103104.7D8E0C40992-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
  2014-12-04 11:30       ` Pantelis Antoniou
@ 2014-12-05 19:53       ` Kees Cook
       [not found]         ` <CAGXu5jKZYrGpSwdpG8Pk=0N87Z1yT3S=utzCnQgvZ+4YeA2hKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  1 sibling, 1 reply; 16+ messages in thread
From: Kees Cook @ 2014-12-05 19:53 UTC (permalink / raw)
  To: Grant Likely
  Cc: Pantelis Antoniou, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou

On Thu, Dec 4, 2014 at 2:31 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> Kees,
>
> I'm copying you on this email. This is a new interface being proposed
> for making live changes to the device tree at runtime, which gives
> pretty close to direct control to userspace of the Linux device model.
> Userspace can supply a blob that causes devices to get added or removed
> from /sys/devices, and can change how a device is accessed. This is
> definitely a security concern and I would love to get your opinion on
> the interface.
>
> Naturally, the interface has been made root-only, but I don't think that
> is enough. I think we need a method which parts of the devicetree can be
> modified by a devicetree overlay. I've been thinking in terms of adding
> allow/deny properties that control whether or not changes can be made
> below a particular node, with the default for the whole tree to be
> 'deny'. That way the devicetree for the platform can restrict dynamic
> change to only the node that need to be modifed.
>
> What do you think?

I'm less familiar with device tree, but given the things that I'm
imagining can be twiddled via this interface, it looks extremely
powerful. Could devices be changed out from under drivers, or physical
RAM remapped, etc?

Coming from the perspective of drawing a bright line between kernel
and the root user (which tends to start with disabling kernel module
loading), I would say that there at least needs to be a high-level
one-way "off" switch for the interface so that systems that have this
interface can choose to turn it off during initial boot, etc.

If the tree node ACL you're considering could be used like this, that
seems like a good approach, as long as the non-deny nodes don't allow
attaching devices that could access memory in some way, since then
they could be used to flip the deny bits on other nodes and the
slippery slope starts... :)

Hopefully that's useful?

-Kees

>
> Pantelis, I've made comments below on the patch...
>
> On Wed,  3 Dec 2014 13:23:28 +0200
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>  wrote:
>> Add a runtime interface to using configfs for generic device tree overlay
>> usage. With it its possible to use device tree overlays without having
>> to use a per-platform overlay manager.
>>
>> Please see Documentation/devicetree/configfs-overlays.txt for more info.
>>
>> Changes since v2:
>> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
>> - Created a documentation entry
>> - Slight rewording in Kconfig
>>
>> Changes since v1:
>> - of_resolve() -> of_resolve_phandles().
>>
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> ---
>>  Documentation/devicetree/configfs-overlays.txt |  31 +++
>>  drivers/of/Kconfig                             |   7 +
>>  drivers/of/Makefile                            |   1 +
>>  drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
>>  4 files changed, 371 insertions(+)
>>  create mode 100644 Documentation/devicetree/configfs-overlays.txt
>>  create mode 100644 drivers/of/configfs.c
>>
>> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
>> new file mode 100644
>> index 0000000..5fa43e0
>> --- /dev/null
>> +++ b/Documentation/devicetree/configfs-overlays.txt
>> @@ -0,0 +1,31 @@
>> +Howto use the configfs overlay interface.
>> +
>> +A device-tree configfs entry is created in /config/device-tree/overlays
>> +and and it is manipulated using standard file system I/O.
>> +Note that this is a debug level interface, for use by developers and
>> +not necessarily something accessed by normal users due to the
>> +security implications of having direct access to the kernel's device tree.
>> +
>> +* To create an overlay you mkdir the directory:
>> +
>> +     # mkdir /config/device-tree/overlays/foo
>> +
>> +* Either you echo the overlay firmware file to the path property file.
>> +
>> +     # echo foo.dtbo >/config/device-tree/overlays/foo/path
>> +
>> +* Or you cat the contents of the overlay to the dtbo file
>> +
>> +     # cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
>> +
>> +The overlay file will be applied, and devices will be created/destroyed
>> +as required.
>> +
>> +To remove it simply rmdir the directory.
>> +
>> +     # rmdir /config/device-tree/overlays/foo
>> +
>> +The rationalle of the dual interface (firmware & direct copy) is that each is
>
> sp. rationale
>
>> +better suited to different use patterns. The firmware interface is what's
>> +intended to be used by hardware managers in the kernel, while the copy interface
>> +make sense for developers (since it avoids problems with namespaces).
>
> The distinction between developer and production interfaces is
> unfortunately meaningless in the real world. The developer interface
> will get used in production if it is there, and having two interfaces
> means we have to support two interfaces that do /exactly the same
> thing/.
>
> One interface only please.
>
> What are the problems with namespaces that you see with the firmware
> interface?
>
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 18b2e25..5d36306 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -91,4 +91,11 @@ config OF_OVERLAY
>>       select OF_DEVICE
>>       select OF_RESOLVE
>>
>> +config OF_CONFIGFS
>> +     bool "Device Tree Overlay ConfigFS interface"
>> +     select CONFIGFS_FS
>> +     select OF_OVERLAY
>> +     help
>> +       Enable a simple user-space driven DT overlay interface.
>> +
>>  endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 7563f36..6f7f4f8 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,4 +1,5 @@
>>  obj-y = base.o device.o platform.o
>> +obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
>> --- /dev/null
>> +++ b/drivers/of/configfs.c
>> @@ -0,0 +1,332 @@
>> +/*
>> + * Configfs entries for device-tree
>> + *
>> + * Copyright (C) 2013 - Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
>> + *
>> + * 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/ctype.h>
>> +#include <linux/cpu.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_fdt.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/slab.h>
>> +#include <linux/proc_fs.h>
>> +#include <linux/configfs.h>
>> +#include <linux/types.h>
>> +#include <linux/stat.h>
>> +#include <linux/limits.h>
>> +#include <linux/file.h>
>> +#include <linux/vmalloc.h>
>> +#include <linux/firmware.h>
>> +
>> +#include "of_private.h"
>> +
>> +struct cfs_overlay_item {
>> +     struct config_item      item;
>> +
>> +     char                    path[PATH_MAX];
>
> Hard coded maximum path length?
>
>> +
>> +     const struct firmware   *fw;
>> +     struct device_node      *overlay;
>> +     int                     ov_id;
>> +
>> +     void                    *dtbo;
>> +     int                     dtbo_size;
>> +};
>> +
>> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
>> +{
>> +     int err;
>> +
>> +     /* unflatten the tree */
>> +     of_fdt_unflatten_tree(blob, &overlay->overlay);
>> +     if (overlay->overlay == NULL) {
>> +             pr_err("%s: failed to unflatten tree\n", __func__);
>> +             err = -EINVAL;
>> +             goto out_err;
>> +     }
>> +     pr_debug("%s: unflattened OK\n", __func__);
>> +
>> +     /* mark it as detached */
>> +     of_node_set_flag(overlay->overlay, OF_DETACHED);
>> +
>> +     /* perform resolution */
>> +     err = of_resolve_phandles(overlay->overlay);
>> +     if (err != 0) {
>> +             pr_err("%s: Failed to resolve tree\n", __func__);
>> +             goto out_err;
>> +     }
>> +     pr_debug("%s: resolved OK\n", __func__);
>> +
>> +     err = of_overlay_create(overlay->overlay);
>> +     if (err < 0) {
>> +             pr_err("%s: Failed to create overlay (err=%d)\n",
>> +                             __func__, err);
>> +             goto out_err;
>> +     }
>> +     overlay->ov_id = err;
>> +
>> +out_err:
>> +     return err;
>> +}
>> +
>> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
>> +             struct config_item *item)
>> +{
>> +     return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
>> +}
>> +
>> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
>> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)   \
>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
>> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)       \
>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>> +     __CONFIGFS_ATTR_RO(_name, _show)
>> +
>> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
>> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>> +     __CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
>> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)      \
>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>> +     __CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
>> +
>> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
>> +             char *page)
>> +{
>> +     return sprintf(page, "%s\n", overlay->path);
>> +}
>> +
>> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
>> +             const char *page, size_t count)
>> +{
>> +     const char *p = page;
>> +     char *s;
>> +     int err;
>> +
>> +     /* if it's set do not allow changes */
>> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +             return -EPERM;
>> +
>> +     /* copy to path buffer (and make sure it's always zero terminated */
>> +     count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
>> +     overlay->path[sizeof(overlay->path) - 1] = '\0';
>
> kmemdup perhaps to get rid of the hard coded path length?
>
>> +
>> +     /* strip trailing newlines */
>> +     s = overlay->path + strlen(overlay->path);
>> +     while (s > overlay->path && *--s == '\n')
>> +             *s = '\0';
>
> strchr() will find trailing \n's for you.
>
>> +
>> +     pr_debug("%s: path is '%s'\n", __func__, overlay->path);
>> +
>> +     err = request_firmware(&overlay->fw, overlay->path, NULL);
>> +     if (err != 0)
>> +             goto out_err;
>> +
>> +     err = create_overlay(overlay, (void *)overlay->fw->data);
>> +     if (err != 0)
>> +             goto out_err;
>> +
>> +     return count;
>> +
>> +out_err:
>> +
>> +     release_firmware(overlay->fw);
>> +     overlay->fw = NULL;
>> +
>> +     overlay->path[0] = '\0';
>> +     return err;
>> +}
>> +
>> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
>> +             char *page)
>> +{
>> +     return sprintf(page, "%s\n",
>> +                     overlay->ov_id >= 0 ? "applied" : "unapplied");
>> +}
>> +
>> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
>> +             cfs_overlay_item_path_show, cfs_overlay_item_path_store);
>> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
>> +
>> +static struct configfs_attribute *cfs_overlay_attrs[] = {
>> +     &cfs_overlay_item_attr_path.attr,
>> +     &cfs_overlay_item_attr_status.attr,
>> +     NULL,
>> +};
>> +
>> +ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
>> +             void *buf, size_t max_count)
>> +{
>> +     pr_debug("%s: buf=%p max_count=%u\n", __func__,
>> +                     buf, max_count);
>> +
>> +     if (overlay->dtbo == NULL)
>> +             return 0;
>> +
>> +     /* copy if buffer provided */
>> +     if (buf != NULL) {
>> +             /* the buffer must be large enough */
>> +             if (overlay->dtbo_size > max_count)
>> +                     return -ENOSPC;
>
> I've not dug into configfs code. Is it not possible to do a partial
> read?
>
>> +
>> +             memcpy(buf, overlay->dtbo, overlay->dtbo_size);
>> +     }
>> +
>> +     return overlay->dtbo_size;
>> +}
>> +
>> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
>> +             const void *buf, size_t count)
>> +{
>> +     int err;
>> +
>> +     /* if it's set do not allow changes */
>> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +             return -EPERM;
>> +
>> +     /* copy the contents */
>> +     overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
>> +     if (overlay->dtbo == NULL)
>> +             return -ENOMEM;
>> +
>> +     overlay->dtbo_size = count;
>> +
>> +     err = create_overlay(overlay, overlay->dtbo);
>> +     if (err != 0)
>> +             goto out_err;
>> +
>> +     return count;
>> +
>> +out_err:
>> +     kfree(overlay->dtbo);
>> +     overlay->dtbo = NULL;
>> +     overlay->dtbo_size = 0;
>> +
>> +     return err;
>> +}
>> +
>> +CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
>> +             cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
>> +             NULL, SZ_1M);
>> +
>> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
>> +     &cfs_overlay_item_bin_attr_dtbo.bin_attr,
>> +     NULL,
>> +};
>> +
>> +static void cfs_overlay_release(struct config_item *item)
>> +{
>> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>> +
>> +     if (overlay->ov_id >= 0)
>> +             of_overlay_destroy(overlay->ov_id);
>> +     if (overlay->fw)
>> +             release_firmware(overlay->fw);
>> +     /* kfree with NULL is safe */
>> +     kfree(overlay->dtbo);
>> +     kfree(overlay);
>> +}
>> +
>> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
>> +CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
>> +static struct configfs_item_operations cfs_overlay_item_ops = {
>> +     .release                = cfs_overlay_release,
>> +     .show_attribute         = cfs_overlay_item_attr_show,
>> +     .store_attribute        = cfs_overlay_item_attr_store,
>> +     .read_bin_attribute     = cfs_overlay_item_bin_attr_read,
>> +     .write_bin_attribute    = cfs_overlay_item_bin_attr_write,
>> +};
>> +
>> +static struct config_item_type cfs_overlay_type = {
>> +     .ct_item_ops    = &cfs_overlay_item_ops,
>> +     .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 cfs_overlay_item *overlay;
>> +
>> +     overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
>> +     if (!overlay)
>> +             return ERR_PTR(-ENOMEM);
>> +     overlay->ov_id = -1;
>> +
>> +     config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
>> +     return &overlay->item;
>> +}
>> +
>> +static void cfs_overlay_group_drop_item(struct config_group *group,
>> +             struct config_item *item)
>> +{
>> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>> +
>> +     config_item_put(&overlay->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;
>> +
>> +struct config_group *of_cfs_def_groups[] = {
>> +     &of_cfs_overlay_group,
>> +     NULL
>> +};
>> +
>> +static struct configfs_subsystem of_cfs_subsys = {
>> +     .su_group = {
>> +             .cg_item = {
>> +                     .ci_namebuf = "device-tree",
>> +                     .ci_type = &of_cfs_type,
>> +             },
>> +             .default_groups = of_cfs_def_groups,
>> +     },
>> +     .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);
>> +
>> +     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);
>> --
>> 1.7.12
>>
>



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]         ` <CAGXu5jKZYrGpSwdpG8Pk=0N87Z1yT3S=utzCnQgvZ+4YeA2hKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2014-12-05 21:27           ` Pantelis Antoniou
       [not found]             ` <0CD21000-1C09-47D7-AE3B-3DAE942CB861-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
  2014-12-08 17:34           ` Grant Likely
  1 sibling, 1 reply; 16+ messages in thread
From: Pantelis Antoniou @ 2014-12-05 21:27 UTC (permalink / raw)
  To: Kees Cook
  Cc: Grant Likely, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Hi Kees,

> On Dec 5, 2014, at 21:53 , Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
> 
> On Thu, Dec 4, 2014 at 2:31 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>> Kees,
>> 
>> I'm copying you on this email. This is a new interface being proposed
>> for making live changes to the device tree at runtime, which gives
>> pretty close to direct control to userspace of the Linux device model.
>> Userspace can supply a blob that causes devices to get added or removed
>> from /sys/devices, and can change how a device is accessed. This is
>> definitely a security concern and I would love to get your opinion on
>> the interface.
>> 
>> Naturally, the interface has been made root-only, but I don't think that
>> is enough. I think we need a method which parts of the devicetree can be
>> modified by a devicetree overlay. I've been thinking in terms of adding
>> allow/deny properties that control whether or not changes can be made
>> below a particular node, with the default for the whole tree to be
>> 'deny'. That way the devicetree for the platform can restrict dynamic
>> change to only the node that need to be modifed.
>> 
>> What do you think?
> 
> I'm less familiar with device tree, but given the things that I'm
> imagining can be twiddled via this interface, it looks extremely
> powerful. Could devices be changed out from under drivers, or physical
> RAM remapped, etc?
> 

Devices sure, RAM remapped not yet ;)

> Coming from the perspective of drawing a bright line between kernel
> and the root user (which tends to start with disabling kernel module
> loading), I would say that there at least needs to be a high-level
> one-way "off" switch for the interface so that systems that have this
> interface can choose to turn it off during initial boot, etc.
> 

Sure, that’s easy enough to add. Is a sysfs property good enough?

> If the tree node ACL you're considering could be used like this, that
> seems like a good approach, as long as the non-deny nodes don't allow
> attaching devices that could access memory in some way, since then
> they could be used to flip the deny bits on other nodes and the
> slippery slope starts... :)
> 

Devices will be created potentially; those have a probe method, which can
can make anything happen.

This is exactly like from a security point of view as loading modules.

> Hopefully that's useful?
> 

Yes.

> -Kees
> 

Regards

— Pantelis

>> 
>> Pantelis, I've made comments below on the patch...
>> 
>> On Wed,  3 Dec 2014 13:23:28 +0200
>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>> wrote:
>>> Add a runtime interface to using configfs for generic device tree overlay
>>> usage. With it its possible to use device tree overlays without having
>>> to use a per-platform overlay manager.
>>> 
>>> Please see Documentation/devicetree/configfs-overlays.txt for more info.
>>> 
>>> Changes since v2:
>>> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
>>> - Created a documentation entry
>>> - Slight rewording in Kconfig
>>> 
>>> Changes since v1:
>>> - of_resolve() -> of_resolve_phandles().
>>> 
>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>> ---
>>> Documentation/devicetree/configfs-overlays.txt |  31 +++
>>> drivers/of/Kconfig                             |   7 +
>>> drivers/of/Makefile                            |   1 +
>>> drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
>>> 4 files changed, 371 insertions(+)
>>> create mode 100644 Documentation/devicetree/configfs-overlays.txt
>>> create mode 100644 drivers/of/configfs.c
>>> 
>>> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
>>> new file mode 100644
>>> index 0000000..5fa43e0
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/configfs-overlays.txt
>>> @@ -0,0 +1,31 @@
>>> +Howto use the configfs overlay interface.
>>> +
>>> +A device-tree configfs entry is created in /config/device-tree/overlays
>>> +and and it is manipulated using standard file system I/O.
>>> +Note that this is a debug level interface, for use by developers and
>>> +not necessarily something accessed by normal users due to the
>>> +security implications of having direct access to the kernel's device tree.
>>> +
>>> +* To create an overlay you mkdir the directory:
>>> +
>>> +     # mkdir /config/device-tree/overlays/foo
>>> +
>>> +* Either you echo the overlay firmware file to the path property file.
>>> +
>>> +     # echo foo.dtbo >/config/device-tree/overlays/foo/path
>>> +
>>> +* Or you cat the contents of the overlay to the dtbo file
>>> +
>>> +     # cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
>>> +
>>> +The overlay file will be applied, and devices will be created/destroyed
>>> +as required.
>>> +
>>> +To remove it simply rmdir the directory.
>>> +
>>> +     # rmdir /config/device-tree/overlays/foo
>>> +
>>> +The rationalle of the dual interface (firmware & direct copy) is that each is
>> 
>> sp. rationale
>> 
>>> +better suited to different use patterns. The firmware interface is what's
>>> +intended to be used by hardware managers in the kernel, while the copy interface
>>> +make sense for developers (since it avoids problems with namespaces).
>> 
>> The distinction between developer and production interfaces is
>> unfortunately meaningless in the real world. The developer interface
>> will get used in production if it is there, and having two interfaces
>> means we have to support two interfaces that do /exactly the same
>> thing/.
>> 
>> One interface only please.
>> 
>> What are the problems with namespaces that you see with the firmware
>> interface?
>> 
>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>>> index 18b2e25..5d36306 100644
>>> --- a/drivers/of/Kconfig
>>> +++ b/drivers/of/Kconfig
>>> @@ -91,4 +91,11 @@ config OF_OVERLAY
>>>      select OF_DEVICE
>>>      select OF_RESOLVE
>>> 
>>> +config OF_CONFIGFS
>>> +     bool "Device Tree Overlay ConfigFS interface"
>>> +     select CONFIGFS_FS
>>> +     select OF_OVERLAY
>>> +     help
>>> +       Enable a simple user-space driven DT overlay interface.
>>> +
>>> endmenu # OF
>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>>> index 7563f36..6f7f4f8 100644
>>> --- a/drivers/of/Makefile
>>> +++ b/drivers/of/Makefile
>>> @@ -1,4 +1,5 @@
>>> obj-y = base.o device.o platform.o
>>> +obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
>>> --- /dev/null
>>> +++ b/drivers/of/configfs.c
>>> @@ -0,0 +1,332 @@
>>> +/*
>>> + * Configfs entries for device-tree
>>> + *
>>> + * 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/ctype.h>
>>> +#include <linux/cpu.h>
>>> +#include <linux/module.h>
>>> +#include <linux/of.h>
>>> +#include <linux/of_fdt.h>
>>> +#include <linux/spinlock.h>
>>> +#include <linux/slab.h>
>>> +#include <linux/proc_fs.h>
>>> +#include <linux/configfs.h>
>>> +#include <linux/types.h>
>>> +#include <linux/stat.h>
>>> +#include <linux/limits.h>
>>> +#include <linux/file.h>
>>> +#include <linux/vmalloc.h>
>>> +#include <linux/firmware.h>
>>> +
>>> +#include "of_private.h"
>>> +
>>> +struct cfs_overlay_item {
>>> +     struct config_item      item;
>>> +
>>> +     char                    path[PATH_MAX];
>> 
>> Hard coded maximum path length?
>> 
>>> +
>>> +     const struct firmware   *fw;
>>> +     struct device_node      *overlay;
>>> +     int                     ov_id;
>>> +
>>> +     void                    *dtbo;
>>> +     int                     dtbo_size;
>>> +};
>>> +
>>> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
>>> +{
>>> +     int err;
>>> +
>>> +     /* unflatten the tree */
>>> +     of_fdt_unflatten_tree(blob, &overlay->overlay);
>>> +     if (overlay->overlay == NULL) {
>>> +             pr_err("%s: failed to unflatten tree\n", __func__);
>>> +             err = -EINVAL;
>>> +             goto out_err;
>>> +     }
>>> +     pr_debug("%s: unflattened OK\n", __func__);
>>> +
>>> +     /* mark it as detached */
>>> +     of_node_set_flag(overlay->overlay, OF_DETACHED);
>>> +
>>> +     /* perform resolution */
>>> +     err = of_resolve_phandles(overlay->overlay);
>>> +     if (err != 0) {
>>> +             pr_err("%s: Failed to resolve tree\n", __func__);
>>> +             goto out_err;
>>> +     }
>>> +     pr_debug("%s: resolved OK\n", __func__);
>>> +
>>> +     err = of_overlay_create(overlay->overlay);
>>> +     if (err < 0) {
>>> +             pr_err("%s: Failed to create overlay (err=%d)\n",
>>> +                             __func__, err);
>>> +             goto out_err;
>>> +     }
>>> +     overlay->ov_id = err;
>>> +
>>> +out_err:
>>> +     return err;
>>> +}
>>> +
>>> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
>>> +             struct config_item *item)
>>> +{
>>> +     return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
>>> +}
>>> +
>>> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
>>> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)   \
>>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>>> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
>>> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)       \
>>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>>> +     __CONFIGFS_ATTR_RO(_name, _show)
>>> +
>>> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
>>> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
>>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>>> +     __CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
>>> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)      \
>>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>>> +     __CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
>>> +
>>> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
>>> +             char *page)
>>> +{
>>> +     return sprintf(page, "%s\n", overlay->path);
>>> +}
>>> +
>>> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
>>> +             const char *page, size_t count)
>>> +{
>>> +     const char *p = page;
>>> +     char *s;
>>> +     int err;
>>> +
>>> +     /* if it's set do not allow changes */
>>> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>>> +             return -EPERM;
>>> +
>>> +     /* copy to path buffer (and make sure it's always zero terminated */
>>> +     count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
>>> +     overlay->path[sizeof(overlay->path) - 1] = '\0';
>> 
>> kmemdup perhaps to get rid of the hard coded path length?
>> 
>>> +
>>> +     /* strip trailing newlines */
>>> +     s = overlay->path + strlen(overlay->path);
>>> +     while (s > overlay->path && *--s == '\n')
>>> +             *s = '\0';
>> 
>> strchr() will find trailing \n's for you.
>> 
>>> +
>>> +     pr_debug("%s: path is '%s'\n", __func__, overlay->path);
>>> +
>>> +     err = request_firmware(&overlay->fw, overlay->path, NULL);
>>> +     if (err != 0)
>>> +             goto out_err;
>>> +
>>> +     err = create_overlay(overlay, (void *)overlay->fw->data);
>>> +     if (err != 0)
>>> +             goto out_err;
>>> +
>>> +     return count;
>>> +
>>> +out_err:
>>> +
>>> +     release_firmware(overlay->fw);
>>> +     overlay->fw = NULL;
>>> +
>>> +     overlay->path[0] = '\0';
>>> +     return err;
>>> +}
>>> +
>>> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
>>> +             char *page)
>>> +{
>>> +     return sprintf(page, "%s\n",
>>> +                     overlay->ov_id >= 0 ? "applied" : "unapplied");
>>> +}
>>> +
>>> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
>>> +             cfs_overlay_item_path_show, cfs_overlay_item_path_store);
>>> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
>>> +
>>> +static struct configfs_attribute *cfs_overlay_attrs[] = {
>>> +     &cfs_overlay_item_attr_path.attr,
>>> +     &cfs_overlay_item_attr_status.attr,
>>> +     NULL,
>>> +};
>>> +
>>> +ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
>>> +             void *buf, size_t max_count)
>>> +{
>>> +     pr_debug("%s: buf=%p max_count=%u\n", __func__,
>>> +                     buf, max_count);
>>> +
>>> +     if (overlay->dtbo == NULL)
>>> +             return 0;
>>> +
>>> +     /* copy if buffer provided */
>>> +     if (buf != NULL) {
>>> +             /* the buffer must be large enough */
>>> +             if (overlay->dtbo_size > max_count)
>>> +                     return -ENOSPC;
>> 
>> I've not dug into configfs code. Is it not possible to do a partial
>> read?
>> 
>>> +
>>> +             memcpy(buf, overlay->dtbo, overlay->dtbo_size);
>>> +     }
>>> +
>>> +     return overlay->dtbo_size;
>>> +}
>>> +
>>> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
>>> +             const void *buf, size_t count)
>>> +{
>>> +     int err;
>>> +
>>> +     /* if it's set do not allow changes */
>>> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>>> +             return -EPERM;
>>> +
>>> +     /* copy the contents */
>>> +     overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
>>> +     if (overlay->dtbo == NULL)
>>> +             return -ENOMEM;
>>> +
>>> +     overlay->dtbo_size = count;
>>> +
>>> +     err = create_overlay(overlay, overlay->dtbo);
>>> +     if (err != 0)
>>> +             goto out_err;
>>> +
>>> +     return count;
>>> +
>>> +out_err:
>>> +     kfree(overlay->dtbo);
>>> +     overlay->dtbo = NULL;
>>> +     overlay->dtbo_size = 0;
>>> +
>>> +     return err;
>>> +}
>>> +
>>> +CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
>>> +             cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
>>> +             NULL, SZ_1M);
>>> +
>>> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
>>> +     &cfs_overlay_item_bin_attr_dtbo.bin_attr,
>>> +     NULL,
>>> +};
>>> +
>>> +static void cfs_overlay_release(struct config_item *item)
>>> +{
>>> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>>> +
>>> +     if (overlay->ov_id >= 0)
>>> +             of_overlay_destroy(overlay->ov_id);
>>> +     if (overlay->fw)
>>> +             release_firmware(overlay->fw);
>>> +     /* kfree with NULL is safe */
>>> +     kfree(overlay->dtbo);
>>> +     kfree(overlay);
>>> +}
>>> +
>>> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
>>> +CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
>>> +static struct configfs_item_operations cfs_overlay_item_ops = {
>>> +     .release                = cfs_overlay_release,
>>> +     .show_attribute         = cfs_overlay_item_attr_show,
>>> +     .store_attribute        = cfs_overlay_item_attr_store,
>>> +     .read_bin_attribute     = cfs_overlay_item_bin_attr_read,
>>> +     .write_bin_attribute    = cfs_overlay_item_bin_attr_write,
>>> +};
>>> +
>>> +static struct config_item_type cfs_overlay_type = {
>>> +     .ct_item_ops    = &cfs_overlay_item_ops,
>>> +     .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 cfs_overlay_item *overlay;
>>> +
>>> +     overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
>>> +     if (!overlay)
>>> +             return ERR_PTR(-ENOMEM);
>>> +     overlay->ov_id = -1;
>>> +
>>> +     config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
>>> +     return &overlay->item;
>>> +}
>>> +
>>> +static void cfs_overlay_group_drop_item(struct config_group *group,
>>> +             struct config_item *item)
>>> +{
>>> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>>> +
>>> +     config_item_put(&overlay->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;
>>> +
>>> +struct config_group *of_cfs_def_groups[] = {
>>> +     &of_cfs_overlay_group,
>>> +     NULL
>>> +};
>>> +
>>> +static struct configfs_subsystem of_cfs_subsys = {
>>> +     .su_group = {
>>> +             .cg_item = {
>>> +                     .ci_namebuf = "device-tree",
>>> +                     .ci_type = &of_cfs_type,
>>> +             },
>>> +             .default_groups = of_cfs_def_groups,
>>> +     },
>>> +     .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);
>>> +
>>> +     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);
>>> --
>>> 1.7.12
>>> 
>> 
> 
> 
> 
> -- 
> Kees Cook
> Chrome OS Security
> --
> To unsubscribe from this list: send the line "unsubscribe devicetree" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]             ` <0CD21000-1C09-47D7-AE3B-3DAE942CB861-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
@ 2014-12-05 22:01               ` Kees Cook
       [not found]                 ` <CAGXu5j+txFEtMSXab9LjcbvL5rhbPc94wimBMOyQj_Fcf3QZjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Kees Cook @ 2014-12-05 22:01 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Grant Likely, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Fri, Dec 5, 2014 at 1:27 PM, Pantelis Antoniou
<panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> Hi Kees,
>
>> On Dec 5, 2014, at 21:53 , Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org> wrote:
>>
>> On Thu, Dec 4, 2014 at 2:31 AM, Grant Likely <grant.likely@secretlab.ca> wrote:
>>> Kees,
>>>
>>> I'm copying you on this email. This is a new interface being proposed
>>> for making live changes to the device tree at runtime, which gives
>>> pretty close to direct control to userspace of the Linux device model.
>>> Userspace can supply a blob that causes devices to get added or removed
>>> from /sys/devices, and can change how a device is accessed. This is
>>> definitely a security concern and I would love to get your opinion on
>>> the interface.
>>>
>>> Naturally, the interface has been made root-only, but I don't think that
>>> is enough. I think we need a method which parts of the devicetree can be
>>> modified by a devicetree overlay. I've been thinking in terms of adding
>>> allow/deny properties that control whether or not changes can be made
>>> below a particular node, with the default for the whole tree to be
>>> 'deny'. That way the devicetree for the platform can restrict dynamic
>>> change to only the node that need to be modifed.
>>>
>>> What do you think?
>>
>> I'm less familiar with device tree, but given the things that I'm
>> imagining can be twiddled via this interface, it looks extremely
>> powerful. Could devices be changed out from under drivers, or physical
>> RAM remapped, etc?
>>
>
> Devices sure, RAM remapped not yet ;)
>
>> Coming from the perspective of drawing a bright line between kernel
>> and the root user (which tends to start with disabling kernel module
>> loading), I would say that there at least needs to be a high-level
>> one-way "off" switch for the interface so that systems that have this
>> interface can choose to turn it off during initial boot, etc.
>>
>
> Sure, that’s easy enough to add. Is a sysfs property good enough?

As long as it's one-way and controls the whole interface (or tree), I
think that'd be great. I'd normally only done this with sysctls
before, but Johannes Berg just did a one-way sysfs flag at my urging
in devcoredump, so it would probably look pretty similar:
https://lkml.org/lkml/2014/11/13/610

>
>> If the tree node ACL you're considering could be used like this, that
>> seems like a good approach, as long as the non-deny nodes don't allow
>> attaching devices that could access memory in some way, since then
>> they could be used to flip the deny bits on other nodes and the
>> slippery slope starts... :)
>>
>
> Devices will be created potentially; those have a probe method, which can
> can make anything happen.
>
> This is exactly like from a security point of view as loading modules.

Just a random idea, but would it make sense to just tie the logic to
the modules_disabled flag? Probably not, but thought I'd throw it out
there.

-Kees

>
>> Hopefully that's useful?
>>
>
> Yes.
>
>> -Kees
>>
>
> Regards
>
> — Pantelis
>
>>>
>>> Pantelis, I've made comments below on the patch...
>>>
>>> On Wed,  3 Dec 2014 13:23:28 +0200
>>> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>> wrote:
>>>> Add a runtime interface to using configfs for generic device tree overlay
>>>> usage. With it its possible to use device tree overlays without having
>>>> to use a per-platform overlay manager.
>>>>
>>>> Please see Documentation/devicetree/configfs-overlays.txt for more info.
>>>>
>>>> Changes since v2:
>>>> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
>>>> - Created a documentation entry
>>>> - Slight rewording in Kconfig
>>>>
>>>> Changes since v1:
>>>> - of_resolve() -> of_resolve_phandles().
>>>>
>>>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
>>>> ---
>>>> Documentation/devicetree/configfs-overlays.txt |  31 +++
>>>> drivers/of/Kconfig                             |   7 +
>>>> drivers/of/Makefile                            |   1 +
>>>> drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
>>>> 4 files changed, 371 insertions(+)
>>>> create mode 100644 Documentation/devicetree/configfs-overlays.txt
>>>> create mode 100644 drivers/of/configfs.c
>>>>
>>>> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
>>>> new file mode 100644
>>>> index 0000000..5fa43e0
>>>> --- /dev/null
>>>> +++ b/Documentation/devicetree/configfs-overlays.txt
>>>> @@ -0,0 +1,31 @@
>>>> +Howto use the configfs overlay interface.
>>>> +
>>>> +A device-tree configfs entry is created in /config/device-tree/overlays
>>>> +and and it is manipulated using standard file system I/O.
>>>> +Note that this is a debug level interface, for use by developers and
>>>> +not necessarily something accessed by normal users due to the
>>>> +security implications of having direct access to the kernel's device tree.
>>>> +
>>>> +* To create an overlay you mkdir the directory:
>>>> +
>>>> +     # mkdir /config/device-tree/overlays/foo
>>>> +
>>>> +* Either you echo the overlay firmware file to the path property file.
>>>> +
>>>> +     # echo foo.dtbo >/config/device-tree/overlays/foo/path
>>>> +
>>>> +* Or you cat the contents of the overlay to the dtbo file
>>>> +
>>>> +     # cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
>>>> +
>>>> +The overlay file will be applied, and devices will be created/destroyed
>>>> +as required.
>>>> +
>>>> +To remove it simply rmdir the directory.
>>>> +
>>>> +     # rmdir /config/device-tree/overlays/foo
>>>> +
>>>> +The rationalle of the dual interface (firmware & direct copy) is that each is
>>>
>>> sp. rationale
>>>
>>>> +better suited to different use patterns. The firmware interface is what's
>>>> +intended to be used by hardware managers in the kernel, while the copy interface
>>>> +make sense for developers (since it avoids problems with namespaces).
>>>
>>> The distinction between developer and production interfaces is
>>> unfortunately meaningless in the real world. The developer interface
>>> will get used in production if it is there, and having two interfaces
>>> means we have to support two interfaces that do /exactly the same
>>> thing/.
>>>
>>> One interface only please.
>>>
>>> What are the problems with namespaces that you see with the firmware
>>> interface?
>>>
>>>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>>>> index 18b2e25..5d36306 100644
>>>> --- a/drivers/of/Kconfig
>>>> +++ b/drivers/of/Kconfig
>>>> @@ -91,4 +91,11 @@ config OF_OVERLAY
>>>>      select OF_DEVICE
>>>>      select OF_RESOLVE
>>>>
>>>> +config OF_CONFIGFS
>>>> +     bool "Device Tree Overlay ConfigFS interface"
>>>> +     select CONFIGFS_FS
>>>> +     select OF_OVERLAY
>>>> +     help
>>>> +       Enable a simple user-space driven DT overlay interface.
>>>> +
>>>> endmenu # OF
>>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>>>> index 7563f36..6f7f4f8 100644
>>>> --- a/drivers/of/Makefile
>>>> +++ b/drivers/of/Makefile
>>>> @@ -1,4 +1,5 @@
>>>> obj-y = base.o device.o platform.o
>>>> +obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
>>>> --- /dev/null
>>>> +++ b/drivers/of/configfs.c
>>>> @@ -0,0 +1,332 @@
>>>> +/*
>>>> + * Configfs entries for device-tree
>>>> + *
>>>> + * 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/ctype.h>
>>>> +#include <linux/cpu.h>
>>>> +#include <linux/module.h>
>>>> +#include <linux/of.h>
>>>> +#include <linux/of_fdt.h>
>>>> +#include <linux/spinlock.h>
>>>> +#include <linux/slab.h>
>>>> +#include <linux/proc_fs.h>
>>>> +#include <linux/configfs.h>
>>>> +#include <linux/types.h>
>>>> +#include <linux/stat.h>
>>>> +#include <linux/limits.h>
>>>> +#include <linux/file.h>
>>>> +#include <linux/vmalloc.h>
>>>> +#include <linux/firmware.h>
>>>> +
>>>> +#include "of_private.h"
>>>> +
>>>> +struct cfs_overlay_item {
>>>> +     struct config_item      item;
>>>> +
>>>> +     char                    path[PATH_MAX];
>>>
>>> Hard coded maximum path length?
>>>
>>>> +
>>>> +     const struct firmware   *fw;
>>>> +     struct device_node      *overlay;
>>>> +     int                     ov_id;
>>>> +
>>>> +     void                    *dtbo;
>>>> +     int                     dtbo_size;
>>>> +};
>>>> +
>>>> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
>>>> +{
>>>> +     int err;
>>>> +
>>>> +     /* unflatten the tree */
>>>> +     of_fdt_unflatten_tree(blob, &overlay->overlay);
>>>> +     if (overlay->overlay == NULL) {
>>>> +             pr_err("%s: failed to unflatten tree\n", __func__);
>>>> +             err = -EINVAL;
>>>> +             goto out_err;
>>>> +     }
>>>> +     pr_debug("%s: unflattened OK\n", __func__);
>>>> +
>>>> +     /* mark it as detached */
>>>> +     of_node_set_flag(overlay->overlay, OF_DETACHED);
>>>> +
>>>> +     /* perform resolution */
>>>> +     err = of_resolve_phandles(overlay->overlay);
>>>> +     if (err != 0) {
>>>> +             pr_err("%s: Failed to resolve tree\n", __func__);
>>>> +             goto out_err;
>>>> +     }
>>>> +     pr_debug("%s: resolved OK\n", __func__);
>>>> +
>>>> +     err = of_overlay_create(overlay->overlay);
>>>> +     if (err < 0) {
>>>> +             pr_err("%s: Failed to create overlay (err=%d)\n",
>>>> +                             __func__, err);
>>>> +             goto out_err;
>>>> +     }
>>>> +     overlay->ov_id = err;
>>>> +
>>>> +out_err:
>>>> +     return err;
>>>> +}
>>>> +
>>>> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
>>>> +             struct config_item *item)
>>>> +{
>>>> +     return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
>>>> +}
>>>> +
>>>> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
>>>> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)   \
>>>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>>>> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
>>>> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)       \
>>>> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
>>>> +     __CONFIGFS_ATTR_RO(_name, _show)
>>>> +
>>>> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
>>>> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
>>>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>>>> +     __CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
>>>> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)      \
>>>> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
>>>> +     __CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
>>>> +
>>>> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
>>>> +             char *page)
>>>> +{
>>>> +     return sprintf(page, "%s\n", overlay->path);
>>>> +}
>>>> +
>>>> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
>>>> +             const char *page, size_t count)
>>>> +{
>>>> +     const char *p = page;
>>>> +     char *s;
>>>> +     int err;
>>>> +
>>>> +     /* if it's set do not allow changes */
>>>> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>>>> +             return -EPERM;
>>>> +
>>>> +     /* copy to path buffer (and make sure it's always zero terminated */
>>>> +     count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
>>>> +     overlay->path[sizeof(overlay->path) - 1] = '\0';
>>>
>>> kmemdup perhaps to get rid of the hard coded path length?
>>>
>>>> +
>>>> +     /* strip trailing newlines */
>>>> +     s = overlay->path + strlen(overlay->path);
>>>> +     while (s > overlay->path && *--s == '\n')
>>>> +             *s = '\0';
>>>
>>> strchr() will find trailing \n's for you.
>>>
>>>> +
>>>> +     pr_debug("%s: path is '%s'\n", __func__, overlay->path);
>>>> +
>>>> +     err = request_firmware(&overlay->fw, overlay->path, NULL);
>>>> +     if (err != 0)
>>>> +             goto out_err;
>>>> +
>>>> +     err = create_overlay(overlay, (void *)overlay->fw->data);
>>>> +     if (err != 0)
>>>> +             goto out_err;
>>>> +
>>>> +     return count;
>>>> +
>>>> +out_err:
>>>> +
>>>> +     release_firmware(overlay->fw);
>>>> +     overlay->fw = NULL;
>>>> +
>>>> +     overlay->path[0] = '\0';
>>>> +     return err;
>>>> +}
>>>> +
>>>> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
>>>> +             char *page)
>>>> +{
>>>> +     return sprintf(page, "%s\n",
>>>> +                     overlay->ov_id >= 0 ? "applied" : "unapplied");
>>>> +}
>>>> +
>>>> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
>>>> +             cfs_overlay_item_path_show, cfs_overlay_item_path_store);
>>>> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
>>>> +
>>>> +static struct configfs_attribute *cfs_overlay_attrs[] = {
>>>> +     &cfs_overlay_item_attr_path.attr,
>>>> +     &cfs_overlay_item_attr_status.attr,
>>>> +     NULL,
>>>> +};
>>>> +
>>>> +ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
>>>> +             void *buf, size_t max_count)
>>>> +{
>>>> +     pr_debug("%s: buf=%p max_count=%u\n", __func__,
>>>> +                     buf, max_count);
>>>> +
>>>> +     if (overlay->dtbo == NULL)
>>>> +             return 0;
>>>> +
>>>> +     /* copy if buffer provided */
>>>> +     if (buf != NULL) {
>>>> +             /* the buffer must be large enough */
>>>> +             if (overlay->dtbo_size > max_count)
>>>> +                     return -ENOSPC;
>>>
>>> I've not dug into configfs code. Is it not possible to do a partial
>>> read?
>>>
>>>> +
>>>> +             memcpy(buf, overlay->dtbo, overlay->dtbo_size);
>>>> +     }
>>>> +
>>>> +     return overlay->dtbo_size;
>>>> +}
>>>> +
>>>> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
>>>> +             const void *buf, size_t count)
>>>> +{
>>>> +     int err;
>>>> +
>>>> +     /* if it's set do not allow changes */
>>>> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>>>> +             return -EPERM;
>>>> +
>>>> +     /* copy the contents */
>>>> +     overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
>>>> +     if (overlay->dtbo == NULL)
>>>> +             return -ENOMEM;
>>>> +
>>>> +     overlay->dtbo_size = count;
>>>> +
>>>> +     err = create_overlay(overlay, overlay->dtbo);
>>>> +     if (err != 0)
>>>> +             goto out_err;
>>>> +
>>>> +     return count;
>>>> +
>>>> +out_err:
>>>> +     kfree(overlay->dtbo);
>>>> +     overlay->dtbo = NULL;
>>>> +     overlay->dtbo_size = 0;
>>>> +
>>>> +     return err;
>>>> +}
>>>> +
>>>> +CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
>>>> +             cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
>>>> +             NULL, SZ_1M);
>>>> +
>>>> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
>>>> +     &cfs_overlay_item_bin_attr_dtbo.bin_attr,
>>>> +     NULL,
>>>> +};
>>>> +
>>>> +static void cfs_overlay_release(struct config_item *item)
>>>> +{
>>>> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>>>> +
>>>> +     if (overlay->ov_id >= 0)
>>>> +             of_overlay_destroy(overlay->ov_id);
>>>> +     if (overlay->fw)
>>>> +             release_firmware(overlay->fw);
>>>> +     /* kfree with NULL is safe */
>>>> +     kfree(overlay->dtbo);
>>>> +     kfree(overlay);
>>>> +}
>>>> +
>>>> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
>>>> +CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
>>>> +static struct configfs_item_operations cfs_overlay_item_ops = {
>>>> +     .release                = cfs_overlay_release,
>>>> +     .show_attribute         = cfs_overlay_item_attr_show,
>>>> +     .store_attribute        = cfs_overlay_item_attr_store,
>>>> +     .read_bin_attribute     = cfs_overlay_item_bin_attr_read,
>>>> +     .write_bin_attribute    = cfs_overlay_item_bin_attr_write,
>>>> +};
>>>> +
>>>> +static struct config_item_type cfs_overlay_type = {
>>>> +     .ct_item_ops    = &cfs_overlay_item_ops,
>>>> +     .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 cfs_overlay_item *overlay;
>>>> +
>>>> +     overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
>>>> +     if (!overlay)
>>>> +             return ERR_PTR(-ENOMEM);
>>>> +     overlay->ov_id = -1;
>>>> +
>>>> +     config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
>>>> +     return &overlay->item;
>>>> +}
>>>> +
>>>> +static void cfs_overlay_group_drop_item(struct config_group *group,
>>>> +             struct config_item *item)
>>>> +{
>>>> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
>>>> +
>>>> +     config_item_put(&overlay->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;
>>>> +
>>>> +struct config_group *of_cfs_def_groups[] = {
>>>> +     &of_cfs_overlay_group,
>>>> +     NULL
>>>> +};
>>>> +
>>>> +static struct configfs_subsystem of_cfs_subsys = {
>>>> +     .su_group = {
>>>> +             .cg_item = {
>>>> +                     .ci_namebuf = "device-tree",
>>>> +                     .ci_type = &of_cfs_type,
>>>> +             },
>>>> +             .default_groups = of_cfs_def_groups,
>>>> +     },
>>>> +     .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);
>>>> +
>>>> +     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);
>>>> --
>>>> 1.7.12
>>>>
>>>
>>
>>
>>
>> --
>> Kees Cook
>> Chrome OS Security
>> --
>> To unsubscribe from this list: send the line "unsubscribe devicetree" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>



-- 
Kees Cook
Chrome OS Security
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]         ` <CAGXu5jKZYrGpSwdpG8Pk=0N87Z1yT3S=utzCnQgvZ+4YeA2hKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2014-12-05 21:27           ` Pantelis Antoniou
@ 2014-12-08 17:34           ` Grant Likely
  1 sibling, 0 replies; 16+ messages in thread
From: Grant Likely @ 2014-12-08 17:34 UTC (permalink / raw)
  To: Kees Cook
  Cc: Pantelis Antoniou, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou

On Fri, 5 Dec 2014 11:53:04 -0800
, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
 wrote:
> On Thu, Dec 4, 2014 at 2:31 AM, Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> > Kees,
> >
> > I'm copying you on this email. This is a new interface being proposed
> > for making live changes to the device tree at runtime, which gives
> > pretty close to direct control to userspace of the Linux device model.
> > Userspace can supply a blob that causes devices to get added or removed
> > from /sys/devices, and can change how a device is accessed. This is
> > definitely a security concern and I would love to get your opinion on
> > the interface.
> >
> > Naturally, the interface has been made root-only, but I don't think that
> > is enough. I think we need a method which parts of the devicetree can be
> > modified by a devicetree overlay. I've been thinking in terms of adding
> > allow/deny properties that control whether or not changes can be made
> > below a particular node, with the default for the whole tree to be
> > 'deny'. That way the devicetree for the platform can restrict dynamic
> > change to only the node that need to be modifed.
> >
> > What do you think?
> 
> I'm less familiar with device tree, but given the things that I'm
> imagining can be twiddled via this interface, it looks extremely
> powerful. Could devices be changed out from under drivers, or physical
> RAM remapped, etc?

Here's where we're at right now:
- Platform, spi and i2c devices can be added and removed dynamically by
  adding/removing/enabling/disabling nodes. The platform devices can be
  crafted to bind to any platform_driver and be associated with any
  region of memory
- The /data/ about physical RAM can be altered, but the kernel won't
  register the change. It is conceivable that this interface would be
  used to hotplug RAM, but there is no support at present to do so

> Coming from the perspective of drawing a bright line between kernel
> and the root user (which tends to start with disabling kernel module
> loading), I would say that there at least needs to be a high-level
> one-way "off" switch for the interface so that systems that have this
> interface can choose to turn it off during initial boot, etc.

Agreed.

> If the tree node ACL you're considering could be used like this, that
> seems like a good approach, as long as the non-deny nodes don't allow
> attaching devices that could access memory in some way, since then
> they could be used to flip the deny bits on other nodes and the
> slippery slope starts... :)

This is a potential problem. We do have a way to restrict the RAM that a
device node is able to reference ('ranges' property needs to specify
windows, not a blanket unity mapping), but it requires the creator of the
device tree to actually craft the ranges property correctly.

> Hopefully that's useful?

Yes, very useful. Thanks.

> 
> -Kees
> 
> >
> > Pantelis, I've made comments below on the patch...
> >
> > On Wed,  3 Dec 2014 13:23:28 +0200
> > , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >  wrote:
> >> Add a runtime interface to using configfs for generic device tree overlay
> >> usage. With it its possible to use device tree overlays without having
> >> to use a per-platform overlay manager.
> >>
> >> Please see Documentation/devicetree/configfs-overlays.txt for more info.
> >>
> >> Changes since v2:
> >> - Removed ifdef CONFIG_OF_OVERLAY (since for now it's required)
> >> - Created a documentation entry
> >> - Slight rewording in Kconfig
> >>
> >> Changes since v1:
> >> - of_resolve() -> of_resolve_phandles().
> >>
> >> Signed-off-by: Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> >> ---
> >>  Documentation/devicetree/configfs-overlays.txt |  31 +++
> >>  drivers/of/Kconfig                             |   7 +
> >>  drivers/of/Makefile                            |   1 +
> >>  drivers/of/configfs.c                          | 332 +++++++++++++++++++++++++
> >>  4 files changed, 371 insertions(+)
> >>  create mode 100644 Documentation/devicetree/configfs-overlays.txt
> >>  create mode 100644 drivers/of/configfs.c
> >>
> >> diff --git a/Documentation/devicetree/configfs-overlays.txt b/Documentation/devicetree/configfs-overlays.txt
> >> new file mode 100644
> >> index 0000000..5fa43e0
> >> --- /dev/null
> >> +++ b/Documentation/devicetree/configfs-overlays.txt
> >> @@ -0,0 +1,31 @@
> >> +Howto use the configfs overlay interface.
> >> +
> >> +A device-tree configfs entry is created in /config/device-tree/overlays
> >> +and and it is manipulated using standard file system I/O.
> >> +Note that this is a debug level interface, for use by developers and
> >> +not necessarily something accessed by normal users due to the
> >> +security implications of having direct access to the kernel's device tree.
> >> +
> >> +* To create an overlay you mkdir the directory:
> >> +
> >> +     # mkdir /config/device-tree/overlays/foo
> >> +
> >> +* Either you echo the overlay firmware file to the path property file.
> >> +
> >> +     # echo foo.dtbo >/config/device-tree/overlays/foo/path
> >> +
> >> +* Or you cat the contents of the overlay to the dtbo file
> >> +
> >> +     # cat foo.dtbo >/config/device-tree/overlays/foo/dtbo
> >> +
> >> +The overlay file will be applied, and devices will be created/destroyed
> >> +as required.
> >> +
> >> +To remove it simply rmdir the directory.
> >> +
> >> +     # rmdir /config/device-tree/overlays/foo
> >> +
> >> +The rationalle of the dual interface (firmware & direct copy) is that each is
> >
> > sp. rationale
> >
> >> +better suited to different use patterns. The firmware interface is what's
> >> +intended to be used by hardware managers in the kernel, while the copy interface
> >> +make sense for developers (since it avoids problems with namespaces).
> >
> > The distinction between developer and production interfaces is
> > unfortunately meaningless in the real world. The developer interface
> > will get used in production if it is there, and having two interfaces
> > means we have to support two interfaces that do /exactly the same
> > thing/.
> >
> > One interface only please.
> >
> > What are the problems with namespaces that you see with the firmware
> > interface?
> >
> >> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
> >> index 18b2e25..5d36306 100644
> >> --- a/drivers/of/Kconfig
> >> +++ b/drivers/of/Kconfig
> >> @@ -91,4 +91,11 @@ config OF_OVERLAY
> >>       select OF_DEVICE
> >>       select OF_RESOLVE
> >>
> >> +config OF_CONFIGFS
> >> +     bool "Device Tree Overlay ConfigFS interface"
> >> +     select CONFIGFS_FS
> >> +     select OF_OVERLAY
> >> +     help
> >> +       Enable a simple user-space driven DT overlay interface.
> >> +
> >>  endmenu # OF
> >> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
> >> index 7563f36..6f7f4f8 100644
> >> --- a/drivers/of/Makefile
> >> +++ b/drivers/of/Makefile
> >> @@ -1,4 +1,5 @@
> >>  obj-y = base.o device.o platform.o
> >> +obj-$(CONFIG_OF_CONFIGFS) += 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 0000000..1434ade
> >> --- /dev/null
> >> +++ b/drivers/of/configfs.c
> >> @@ -0,0 +1,332 @@
> >> +/*
> >> + * Configfs entries for device-tree
> >> + *
> >> + * Copyright (C) 2013 - Pantelis Antoniou <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
> >> + *
> >> + * 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/ctype.h>
> >> +#include <linux/cpu.h>
> >> +#include <linux/module.h>
> >> +#include <linux/of.h>
> >> +#include <linux/of_fdt.h>
> >> +#include <linux/spinlock.h>
> >> +#include <linux/slab.h>
> >> +#include <linux/proc_fs.h>
> >> +#include <linux/configfs.h>
> >> +#include <linux/types.h>
> >> +#include <linux/stat.h>
> >> +#include <linux/limits.h>
> >> +#include <linux/file.h>
> >> +#include <linux/vmalloc.h>
> >> +#include <linux/firmware.h>
> >> +
> >> +#include "of_private.h"
> >> +
> >> +struct cfs_overlay_item {
> >> +     struct config_item      item;
> >> +
> >> +     char                    path[PATH_MAX];
> >
> > Hard coded maximum path length?
> >
> >> +
> >> +     const struct firmware   *fw;
> >> +     struct device_node      *overlay;
> >> +     int                     ov_id;
> >> +
> >> +     void                    *dtbo;
> >> +     int                     dtbo_size;
> >> +};
> >> +
> >> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
> >> +{
> >> +     int err;
> >> +
> >> +     /* unflatten the tree */
> >> +     of_fdt_unflatten_tree(blob, &overlay->overlay);
> >> +     if (overlay->overlay == NULL) {
> >> +             pr_err("%s: failed to unflatten tree\n", __func__);
> >> +             err = -EINVAL;
> >> +             goto out_err;
> >> +     }
> >> +     pr_debug("%s: unflattened OK\n", __func__);
> >> +
> >> +     /* mark it as detached */
> >> +     of_node_set_flag(overlay->overlay, OF_DETACHED);
> >> +
> >> +     /* perform resolution */
> >> +     err = of_resolve_phandles(overlay->overlay);
> >> +     if (err != 0) {
> >> +             pr_err("%s: Failed to resolve tree\n", __func__);
> >> +             goto out_err;
> >> +     }
> >> +     pr_debug("%s: resolved OK\n", __func__);
> >> +
> >> +     err = of_overlay_create(overlay->overlay);
> >> +     if (err < 0) {
> >> +             pr_err("%s: Failed to create overlay (err=%d)\n",
> >> +                             __func__, err);
> >> +             goto out_err;
> >> +     }
> >> +     overlay->ov_id = err;
> >> +
> >> +out_err:
> >> +     return err;
> >> +}
> >> +
> >> +static inline struct cfs_overlay_item *to_cfs_overlay_item(
> >> +             struct config_item *item)
> >> +{
> >> +     return item ? container_of(item, struct cfs_overlay_item, item) : NULL;
> >> +}
> >> +
> >> +CONFIGFS_ATTR_STRUCT(cfs_overlay_item);
> >> +#define CFS_OVERLAY_ITEM_ATTR(_name, _mode, _show, _store)   \
> >> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> >> +     __CONFIGFS_ATTR(_name, _mode, _show, _store)
> >> +#define CFS_OVERLAY_ITEM_ATTR_RO(_name, _show)       \
> >> +struct cfs_overlay_item_attribute cfs_overlay_item_attr_##_name = \
> >> +     __CONFIGFS_ATTR_RO(_name, _show)
> >> +
> >> +CONFIGFS_BIN_ATTR_STRUCT(cfs_overlay_item);
> >> +#define CFS_OVERLAY_ITEM_BIN_ATTR(_name, _mode, _read, _write, _priv, _max) \
> >> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
> >> +     __CONFIGFS_BIN_ATTR(_name, _mode, _read, _write, _priv, _max)
> >> +#define CFS_OVERLAY_ITEM_BIN_ATTR_RO(_name, _read, _priv, _max)      \
> >> +struct cfs_overlay_item_bin_attribute cfs_overlay_item_bin_attr_##_name = \
> >> +     __CONFIGFS_BIN_ATTR_RO(_name, _read, _priv, _max)
> >> +
> >> +static ssize_t cfs_overlay_item_path_show(struct cfs_overlay_item *overlay,
> >> +             char *page)
> >> +{
> >> +     return sprintf(page, "%s\n", overlay->path);
> >> +}
> >> +
> >> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
> >> +             const char *page, size_t count)
> >> +{
> >> +     const char *p = page;
> >> +     char *s;
> >> +     int err;
> >> +
> >> +     /* if it's set do not allow changes */
> >> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> >> +             return -EPERM;
> >> +
> >> +     /* copy to path buffer (and make sure it's always zero terminated */
> >> +     count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
> >> +     overlay->path[sizeof(overlay->path) - 1] = '\0';
> >
> > kmemdup perhaps to get rid of the hard coded path length?
> >
> >> +
> >> +     /* strip trailing newlines */
> >> +     s = overlay->path + strlen(overlay->path);
> >> +     while (s > overlay->path && *--s == '\n')
> >> +             *s = '\0';
> >
> > strchr() will find trailing \n's for you.
> >
> >> +
> >> +     pr_debug("%s: path is '%s'\n", __func__, overlay->path);
> >> +
> >> +     err = request_firmware(&overlay->fw, overlay->path, NULL);
> >> +     if (err != 0)
> >> +             goto out_err;
> >> +
> >> +     err = create_overlay(overlay, (void *)overlay->fw->data);
> >> +     if (err != 0)
> >> +             goto out_err;
> >> +
> >> +     return count;
> >> +
> >> +out_err:
> >> +
> >> +     release_firmware(overlay->fw);
> >> +     overlay->fw = NULL;
> >> +
> >> +     overlay->path[0] = '\0';
> >> +     return err;
> >> +}
> >> +
> >> +static ssize_t cfs_overlay_item_status_show(struct cfs_overlay_item *overlay,
> >> +             char *page)
> >> +{
> >> +     return sprintf(page, "%s\n",
> >> +                     overlay->ov_id >= 0 ? "applied" : "unapplied");
> >> +}
> >> +
> >> +CFS_OVERLAY_ITEM_ATTR(path, S_IRUGO | S_IWUSR,
> >> +             cfs_overlay_item_path_show, cfs_overlay_item_path_store);
> >> +CFS_OVERLAY_ITEM_ATTR_RO(status, cfs_overlay_item_status_show);
> >> +
> >> +static struct configfs_attribute *cfs_overlay_attrs[] = {
> >> +     &cfs_overlay_item_attr_path.attr,
> >> +     &cfs_overlay_item_attr_status.attr,
> >> +     NULL,
> >> +};
> >> +
> >> +ssize_t cfs_overlay_item_dtbo_read(struct cfs_overlay_item *overlay,
> >> +             void *buf, size_t max_count)
> >> +{
> >> +     pr_debug("%s: buf=%p max_count=%u\n", __func__,
> >> +                     buf, max_count);
> >> +
> >> +     if (overlay->dtbo == NULL)
> >> +             return 0;
> >> +
> >> +     /* copy if buffer provided */
> >> +     if (buf != NULL) {
> >> +             /* the buffer must be large enough */
> >> +             if (overlay->dtbo_size > max_count)
> >> +                     return -ENOSPC;
> >
> > I've not dug into configfs code. Is it not possible to do a partial
> > read?
> >
> >> +
> >> +             memcpy(buf, overlay->dtbo, overlay->dtbo_size);
> >> +     }
> >> +
> >> +     return overlay->dtbo_size;
> >> +}
> >> +
> >> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
> >> +             const void *buf, size_t count)
> >> +{
> >> +     int err;
> >> +
> >> +     /* if it's set do not allow changes */
> >> +     if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> >> +             return -EPERM;
> >> +
> >> +     /* copy the contents */
> >> +     overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
> >> +     if (overlay->dtbo == NULL)
> >> +             return -ENOMEM;
> >> +
> >> +     overlay->dtbo_size = count;
> >> +
> >> +     err = create_overlay(overlay, overlay->dtbo);
> >> +     if (err != 0)
> >> +             goto out_err;
> >> +
> >> +     return count;
> >> +
> >> +out_err:
> >> +     kfree(overlay->dtbo);
> >> +     overlay->dtbo = NULL;
> >> +     overlay->dtbo_size = 0;
> >> +
> >> +     return err;
> >> +}
> >> +
> >> +CFS_OVERLAY_ITEM_BIN_ATTR(dtbo, S_IRUGO | S_IWUSR,
> >> +             cfs_overlay_item_dtbo_read, cfs_overlay_item_dtbo_write,
> >> +             NULL, SZ_1M);
> >> +
> >> +static struct configfs_bin_attribute *cfs_overlay_bin_attrs[] = {
> >> +     &cfs_overlay_item_bin_attr_dtbo.bin_attr,
> >> +     NULL,
> >> +};
> >> +
> >> +static void cfs_overlay_release(struct config_item *item)
> >> +{
> >> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
> >> +
> >> +     if (overlay->ov_id >= 0)
> >> +             of_overlay_destroy(overlay->ov_id);
> >> +     if (overlay->fw)
> >> +             release_firmware(overlay->fw);
> >> +     /* kfree with NULL is safe */
> >> +     kfree(overlay->dtbo);
> >> +     kfree(overlay);
> >> +}
> >> +
> >> +CONFIGFS_ATTR_OPS(cfs_overlay_item);
> >> +CONFIGFS_BIN_ATTR_OPS(cfs_overlay_item);
> >> +static struct configfs_item_operations cfs_overlay_item_ops = {
> >> +     .release                = cfs_overlay_release,
> >> +     .show_attribute         = cfs_overlay_item_attr_show,
> >> +     .store_attribute        = cfs_overlay_item_attr_store,
> >> +     .read_bin_attribute     = cfs_overlay_item_bin_attr_read,
> >> +     .write_bin_attribute    = cfs_overlay_item_bin_attr_write,
> >> +};
> >> +
> >> +static struct config_item_type cfs_overlay_type = {
> >> +     .ct_item_ops    = &cfs_overlay_item_ops,
> >> +     .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 cfs_overlay_item *overlay;
> >> +
> >> +     overlay = kzalloc(sizeof(*overlay), GFP_KERNEL);
> >> +     if (!overlay)
> >> +             return ERR_PTR(-ENOMEM);
> >> +     overlay->ov_id = -1;
> >> +
> >> +     config_item_init_type_name(&overlay->item, name, &cfs_overlay_type);
> >> +     return &overlay->item;
> >> +}
> >> +
> >> +static void cfs_overlay_group_drop_item(struct config_group *group,
> >> +             struct config_item *item)
> >> +{
> >> +     struct cfs_overlay_item *overlay = to_cfs_overlay_item(item);
> >> +
> >> +     config_item_put(&overlay->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;
> >> +
> >> +struct config_group *of_cfs_def_groups[] = {
> >> +     &of_cfs_overlay_group,
> >> +     NULL
> >> +};
> >> +
> >> +static struct configfs_subsystem of_cfs_subsys = {
> >> +     .su_group = {
> >> +             .cg_item = {
> >> +                     .ci_namebuf = "device-tree",
> >> +                     .ci_type = &of_cfs_type,
> >> +             },
> >> +             .default_groups = of_cfs_def_groups,
> >> +     },
> >> +     .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);
> >> +
> >> +     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);
> >> --
> >> 1.7.12
> >>
> >
> 
> 
> 
> -- 
> Kees Cook
> Chrome OS Security

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]                 ` <CAGXu5j+txFEtMSXab9LjcbvL5rhbPc94wimBMOyQj_Fcf3QZjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2014-12-08 17:39                   ` Grant Likely
  0 siblings, 0 replies; 16+ messages in thread
From: Grant Likely @ 2014-12-08 17:39 UTC (permalink / raw)
  To: Kees Cook, Pantelis Antoniou
  Cc: Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Fri, 5 Dec 2014 14:01:44 -0800
, Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
 wrote:
> On Fri, Dec 5, 2014 at 1:27 PM, Pantelis Antoniou
> <panto-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org> wrote:
> >
> > Devices will be created potentially; those have a probe method, which can
> > can make anything happen.
> >
> > This is exactly like from a security point of view as loading modules.
> 
> Just a random idea, but would it make sense to just tie the logic to
> the modules_disabled flag? Probably not, but thought I'd throw it out
> there.

I think it might. This opens a whole new window for manipulating the
kernel state. If existing userspace disables modules, but is booted on a
new kernel, then there is suddenly a new window for affecting change on
the kernel that it didn't know about.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]                 ` <E64BA68B-A339-4BB2-B453-B8A9165CB9B7-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
@ 2014-12-08 18:25                   ` Grant Likely
       [not found]                     ` <20141208182531.41BD6C40D73-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
  0 siblings, 1 reply; 16+ messages in thread
From: Grant Likely @ 2014-12-08 18:25 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Kees Cook, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

On Fri, 5 Dec 2014 19:58:27 +0200
, Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
 wrote:
> Hi Grant,
> 
> > On Dec 5, 2014, at 18:44 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> >> 
> [snip]
> 
> >> For instance, if the user issued a load request for a path in a container
> >> which is the namespace under which the firmware file is located? Is it the
> >> non-container filesystem, or is it under the container filesystem that
> >> issued the request. The firmware layer does not have an API to discern.
> > 
> > Wait, what? Why on earth would you want to expose this interface in a container?
> > 
> 
> It was a complaint raised. 
> 
> Look at this thread.
> 
> http://permalink.gmane.org/gmane.linux.documentation/27655

I've replied on that thread. I don't think the issues raised there
warrent two completely separate methods for loading the data into the
kernel. I want to see one method only.

g.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]                     ` <20141208182531.41BD6C40D73-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
@ 2014-12-08 18:29                       ` Pantelis Antoniou
  0 siblings, 0 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2014-12-08 18:29 UTC (permalink / raw)
  To: Grant Likely
  Cc: Kees Cook, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Hi Grant,

> On Dec 8, 2014, at 20:25 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
> 
> On Fri, 5 Dec 2014 19:58:27 +0200
> , Pantelis Antoniou <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
> wrote:
>> Hi Grant,
>> 
>>> On Dec 5, 2014, at 18:44 , Grant Likely <grant.likely-s3s/WqlpOiPyB63q8FvJNQ@public.gmane.org> wrote:
>>>> 
>> [snip]
>> 
>>>> For instance, if the user issued a load request for a path in a container
>>>> which is the namespace under which the firmware file is located? Is it the
>>>> non-container filesystem, or is it under the container filesystem that
>>>> issued the request. The firmware layer does not have an API to discern.
>>> 
>>> Wait, what? Why on earth would you want to expose this interface in a container?
>>> 
>> 
>> It was a complaint raised. 
>> 
>> Look at this thread.
>> 
>> http://permalink.gmane.org/gmane.linux.documentation/27655
> 
> I've replied on that thread. I don't think the issues raised there
> warrent two completely separate methods for loading the data into the
> kernel. I want to see one method only.
> 

The firmware interface is required and is the simplest one.

If that’s OK, let’s go with that.

> g.
> 

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found] ` <1417605808-23327-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
  2014-12-04 10:31   ` Grant Likely
@ 2015-05-13 14:40   ` Geert Uytterhoeven
       [not found]     ` <CAMuHMdXST+xv7PquiP+HPk8WyydbNRh_0EmgPfOJCsitWZ0WuA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2015-05-13 15:31   ` Geert Uytterhoeven
  2 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2015-05-13 14:40 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Grant Likely, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou

Hi Pantelis,

On Wed, Dec 3, 2014 at 12:23 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
> new file mode 100644
> index 0000000..1434ade
> --- /dev/null
> +++ b/drivers/of/configfs.c
> @@ -0,0 +1,332 @@

> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)

After "[PATCH] of/fdt: Make fdt blob input parameters of unflatten functions
const", you can make blob const ... (continue below) ...

> +{

[...]

> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
> +               const char *page, size_t count)
> +{
> +       const char *p = page;
> +       char *s;
> +       int err;
> +
> +       /* if it's set do not allow changes */
> +       if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> +               return -EPERM;
> +
> +       /* copy to path buffer (and make sure it's always zero terminated */
> +       count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
> +       overlay->path[sizeof(overlay->path) - 1] = '\0';

snprintf() always zero-terminates the destination buffer, so you can just do

        count = snprintf(overlay->path, sizeof(overlay->path), "%s", p);

You may want to check if count >= sizeofoverlay->path), to catch truncation.

> +
> +       /* strip trailing newlines */
> +       s = overlay->path + strlen(overlay->path);
> +       while (s > overlay->path && *--s == '\n')
> +               *s = '\0';
> +
> +       pr_debug("%s: path is '%s'\n", __func__, overlay->path);
> +
> +       err = request_firmware(&overlay->fw, overlay->path, NULL);
> +       if (err != 0)
> +               goto out_err;
> +
> +       err = create_overlay(overlay, (void *)overlay->fw->data);

... (continuation) ... and drop the cast.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]     ` <CAMuHMdXST+xv7PquiP+HPk8WyydbNRh_0EmgPfOJCsitWZ0WuA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2015-05-13 14:43       ` Pantelis Antoniou
  0 siblings, 0 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2015-05-13 14:43 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Grant Likely, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Hi Geert,

> On May 13, 2015, at 17:40 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> 
> Hi Pantelis,
> 
> On Wed, Dec 3, 2014 at 12:23 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c
>> new file mode 100644
>> index 0000000..1434ade
>> --- /dev/null
>> +++ b/drivers/of/configfs.c
>> @@ -0,0 +1,332 @@
> 
>> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
> 
> After "[PATCH] of/fdt: Make fdt blob input parameters of unflatten functions
> const", you can make blob const ... (continue below) …
> 

Yes, seen it, thanks.

>> +{
> 
> [...]
> 
>> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
>> +               const char *page, size_t count)
>> +{
>> +       const char *p = page;
>> +       char *s;
>> +       int err;
>> +
>> +       /* if it's set do not allow changes */
>> +       if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +               return -EPERM;
>> +
>> +       /* copy to path buffer (and make sure it's always zero terminated */
>> +       count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
>> +       overlay->path[sizeof(overlay->path) - 1] = '\0';
> 
> snprintf() always zero-terminates the destination buffer, so you can just do
> 
>        count = snprintf(overlay->path, sizeof(overlay->path), "%s", p);
> 
> You may want to check if count >= sizeofoverlay->path), to catch truncation.
> 

Hmm, good catch. The manual zero termination is just a habit I carried from
user-space programming.

>> +
>> +       /* strip trailing newlines */
>> +       s = overlay->path + strlen(overlay->path);
>> +       while (s > overlay->path && *--s == '\n')
>> +               *s = '\0';
>> +
>> +       pr_debug("%s: path is '%s'\n", __func__, overlay->path);
>> +
>> +       err = request_firmware(&overlay->fw, overlay->path, NULL);
>> +       if (err != 0)
>> +               goto out_err;
>> +
>> +       err = create_overlay(overlay, (void *)overlay->fw->data);
> 
> ... (continuation) ... and drop the cast.
> 

OK

> Gr{oetje,eeting}s,
> 
>                        Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

Regards

— Pantelis

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found] ` <1417605808-23327-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
  2014-12-04 10:31   ` Grant Likely
  2015-05-13 14:40   ` Geert Uytterhoeven
@ 2015-05-13 15:31   ` Geert Uytterhoeven
       [not found]     ` <CAMuHMdXdw42PpWe4C1Kf1uLz8NxHLX12+875qhoxoUUUi--C7Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
  2 siblings, 1 reply; 16+ messages in thread
From: Geert Uytterhoeven @ 2015-05-13 15:31 UTC (permalink / raw)
  To: Pantelis Antoniou
  Cc: Grant Likely, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA, Pantelis Antoniou

Hi Pantelis,

On Wed, Dec 3, 2014 at 12:23 PM, Pantelis Antoniou
<pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
> Add a runtime interface to using configfs for generic device tree overlay
> usage. With it its possible to use device tree overlays without having
> to use a per-platform overlay manager.

When loading more than one overlay, I got strange behavior:

    __allocate_fw_buf: fw-r8a7791-koelsch-exio-a-scifb0.dtbo buf=ee27b140
    (NULL device *): firmware: direct-loading firmware
r8a7791-koelsch-exio-a-scifb0.dtbo
    fw_set_page_data: fw-r8a7791-koelsch-exio-a-scifb0.dtbo
buf=ee27b140 data=f0bee000 size=820

    [ loaded overlay is activated!! ]

    __fw_free_buf: fw-r8a7791-koelsch-exio-a-scifb0.dtbo buf=ee27b140
data=f0bee000 size=820
    __allocate_fw_buf: fw-8a7791-koelsch-exio-a-scifb0.dtbo buf=ee3d96c0
    (NULL device *): Direct firmware load for
8a7791-koelsch-exio-a-scifb0.dtbo failed with error -2
    __fw_free_buf: fw-8a7791-koelsch-exio-a-scifb0.dtbo buf=ee3d96c0
data=  (null) size=0

After that:
  - The overlay itself works fine,
  - The "status" file in configfs says "applied".
  - The "path" file in configfs is empty?

Note that an "r" was dropped from the filename in front of the "8a7791".
Hence I suspected some obscure memory corruption bug....


> --- /dev/null
> +++ b/drivers/of/configfs.c
> @@ -0,0 +1,332 @@

> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
> +{
> +       int err;
> +
> +       /* unflatten the tree */
> +       of_fdt_unflatten_tree(blob, &overlay->overlay);
> +       if (overlay->overlay == NULL) {
> +               pr_err("%s: failed to unflatten tree\n", __func__);
> +               err = -EINVAL;
> +               goto out_err;
> +       }
> +       pr_debug("%s: unflattened OK\n", __func__);
> +
> +       /* mark it as detached */
> +       of_node_set_flag(overlay->overlay, OF_DETACHED);
> +
> +       /* perform resolution */
> +       err = of_resolve_phandles(overlay->overlay);
> +       if (err != 0) {
> +               pr_err("%s: Failed to resolve tree\n", __func__);
> +               goto out_err;
> +       }
> +       pr_debug("%s: resolved OK\n", __func__);
> +
> +       err = of_overlay_create(overlay->overlay);
> +       if (err < 0) {
> +               pr_err("%s: Failed to create overlay (err=%d)\n",
> +                               __func__, err);
> +               goto out_err;
> +       }
> +       overlay->ov_id = err;
> +
> +out_err:
> +       return err;

This returns either a strict negative error code, or a positive overlay ID.

> +}

> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
> +               const char *page, size_t count)
> +{
> +       const char *p = page;
> +       char *s;
> +       int err;
> +
> +       /* if it's set do not allow changes */
> +       if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> +               return -EPERM;
> +
> +       /* copy to path buffer (and make sure it's always zero terminated */
> +       count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
> +       overlay->path[sizeof(overlay->path) - 1] = '\0';
> +
> +       /* strip trailing newlines */
> +       s = overlay->path + strlen(overlay->path);
> +       while (s > overlay->path && *--s == '\n')
> +               *s = '\0';
> +
> +       pr_debug("%s: path is '%s'\n", __func__, overlay->path);
> +
> +       err = request_firmware(&overlay->fw, overlay->path, NULL);
> +       if (err != 0)
> +               goto out_err;
> +
> +       err = create_overlay(overlay, (void *)overlay->fw->data);
> +       if (err != 0)

Woops, this test should be "if (err < 0)"!
For the first overlay, err == 0, and everything works fine.
For subsequent overlays, err > 0, and ...

> +               goto out_err;
> +
> +       return count;
> +
> +out_err:
> +
> +       release_firmware(overlay->fw);
> +       overlay->fw = NULL;
> +
> +       overlay->path[0] = '\0';

... the path is cleared...

> +       return err;

...and a positive number is returned, smaller than the passed filename length.
This makes VFS believe the write was truncated, hence it retries, writing only
the remaining part of the filename again. That explains the dropped first
character of the filename, and the firmware load failure.

> +}

> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
> +               const void *buf, size_t count)
> +{
> +       int err;
> +
> +       /* if it's set do not allow changes */
> +       if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
> +               return -EPERM;
> +
> +       /* copy the contents */
> +       overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
> +       if (overlay->dtbo == NULL)
> +               return -ENOMEM;
> +
> +       overlay->dtbo_size = count;
> +
> +       err = create_overlay(overlay, overlay->dtbo);
> +       if (err != 0)

Same here: "if (err < 0)"

> +               goto out_err;
> +
> +       return count;
> +
> +out_err:
> +       kfree(overlay->dtbo);
> +       overlay->dtbo = NULL;
> +       overlay->dtbo_size = 0;
> +
> +       return err;
> +}

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

* Re: [PATCH] OF: DT-Overlay configfs interface (v3)
       [not found]     ` <CAMuHMdXdw42PpWe4C1Kf1uLz8NxHLX12+875qhoxoUUUi--C7Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2015-05-13 15:35       ` Pantelis Antoniou
  0 siblings, 0 replies; 16+ messages in thread
From: Pantelis Antoniou @ 2015-05-13 15:35 UTC (permalink / raw)
  To: Geert Uytterhoeven
  Cc: Grant Likely, Rob Herring, Guenter Roeck, Matt Porter,
	devicetree-u79uwXL29TY76Z2rM5mHXA

Hi Geert,

> On May 13, 2015, at 18:31 , Geert Uytterhoeven <geert-Td1EMuHUCqxL1ZNQvxDV9g@public.gmane.org> wrote:
> 
> Hi Pantelis,
> 
> On Wed, Dec 3, 2014 at 12:23 PM, Pantelis Antoniou
> <pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org> wrote:
>> Add a runtime interface to using configfs for generic device tree overlay
>> usage. With it its possible to use device tree overlays without having
>> to use a per-platform overlay manager.
> 
> When loading more than one overlay, I got strange behavior:
> 
>    __allocate_fw_buf: fw-r8a7791-koelsch-exio-a-scifb0.dtbo buf=ee27b140
>    (NULL device *): firmware: direct-loading firmware
> r8a7791-koelsch-exio-a-scifb0.dtbo
>    fw_set_page_data: fw-r8a7791-koelsch-exio-a-scifb0.dtbo
> buf=ee27b140 data=f0bee000 size=820
> 
>    [ loaded overlay is activated!! ]
> 
>    __fw_free_buf: fw-r8a7791-koelsch-exio-a-scifb0.dtbo buf=ee27b140
> data=f0bee000 size=820
>    __allocate_fw_buf: fw-8a7791-koelsch-exio-a-scifb0.dtbo buf=ee3d96c0
>    (NULL device *): Direct firmware load for
> 8a7791-koelsch-exio-a-scifb0.dtbo failed with error -2
>    __fw_free_buf: fw-8a7791-koelsch-exio-a-scifb0.dtbo buf=ee3d96c0
> data=  (null) size=0
> 
> After that:
>  - The overlay itself works fine,
>  - The "status" file in configfs says "applied".
>  - The "path" file in configfs is empty?
> 
> Note that an "r" was dropped from the filename in front of the "8a7791".
> Hence I suspected some obscure memory corruption bug....
> 
> 
>> --- /dev/null
>> +++ b/drivers/of/configfs.c
>> @@ -0,0 +1,332 @@
> 
>> +static int create_overlay(struct cfs_overlay_item *overlay, void *blob)
>> +{
>> +       int err;
>> +
>> +       /* unflatten the tree */
>> +       of_fdt_unflatten_tree(blob, &overlay->overlay);
>> +       if (overlay->overlay == NULL) {
>> +               pr_err("%s: failed to unflatten tree\n", __func__);
>> +               err = -EINVAL;
>> +               goto out_err;
>> +       }
>> +       pr_debug("%s: unflattened OK\n", __func__);
>> +
>> +       /* mark it as detached */
>> +       of_node_set_flag(overlay->overlay, OF_DETACHED);
>> +
>> +       /* perform resolution */
>> +       err = of_resolve_phandles(overlay->overlay);
>> +       if (err != 0) {
>> +               pr_err("%s: Failed to resolve tree\n", __func__);
>> +               goto out_err;
>> +       }
>> +       pr_debug("%s: resolved OK\n", __func__);
>> +
>> +       err = of_overlay_create(overlay->overlay);
>> +       if (err < 0) {
>> +               pr_err("%s: Failed to create overlay (err=%d)\n",
>> +                               __func__, err);
>> +               goto out_err;
>> +       }
>> +       overlay->ov_id = err;
>> +
>> +out_err:
>> +       return err;
> 
> This returns either a strict negative error code, or a positive overlay ID.
> 
>> +}
> 
>> +static ssize_t cfs_overlay_item_path_store(struct cfs_overlay_item *overlay,
>> +               const char *page, size_t count)
>> +{
>> +       const char *p = page;
>> +       char *s;
>> +       int err;
>> +
>> +       /* if it's set do not allow changes */
>> +       if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +               return -EPERM;
>> +
>> +       /* copy to path buffer (and make sure it's always zero terminated */
>> +       count = snprintf(overlay->path, sizeof(overlay->path) - 1, "%s", p);
>> +       overlay->path[sizeof(overlay->path) - 1] = '\0';
>> +
>> +       /* strip trailing newlines */
>> +       s = overlay->path + strlen(overlay->path);
>> +       while (s > overlay->path && *--s == '\n')
>> +               *s = '\0';
>> +
>> +       pr_debug("%s: path is '%s'\n", __func__, overlay->path);
>> +
>> +       err = request_firmware(&overlay->fw, overlay->path, NULL);
>> +       if (err != 0)
>> +               goto out_err;
>> +
>> +       err = create_overlay(overlay, (void *)overlay->fw->data);
>> +       if (err != 0)
> 
> Woops, this test should be "if (err < 0)"!
> For the first overlay, err == 0, and everything works fine.
> For subsequent overlays, err > 0, and …
> 
You are absolutely correct! Good catch.


>> +               goto out_err;
>> +
>> +       return count;
>> +
>> +out_err:
>> +
>> +       release_firmware(overlay->fw);
>> +       overlay->fw = NULL;
>> +
>> +       overlay->path[0] = '\0';
> 
> ... the path is cleared...
> 
>> +       return err;
> 
> ...and a positive number is returned, smaller than the passed filename length.
> This makes VFS believe the write was truncated, hence it retries, writing only
> the remaining part of the filename again. That explains the dropped first
> character of the filename, and the firmware load failure.
> 
>> +}
> 
>> +ssize_t cfs_overlay_item_dtbo_write(struct cfs_overlay_item *overlay,
>> +               const void *buf, size_t count)
>> +{
>> +       int err;
>> +
>> +       /* if it's set do not allow changes */
>> +       if (overlay->path[0] != '\0' || overlay->dtbo_size > 0)
>> +               return -EPERM;
>> +
>> +       /* copy the contents */
>> +       overlay->dtbo = kmemdup(buf, count, GFP_KERNEL);
>> +       if (overlay->dtbo == NULL)
>> +               return -ENOMEM;
>> +
>> +       overlay->dtbo_size = count;
>> +
>> +       err = create_overlay(overlay, overlay->dtbo);
>> +       if (err != 0)
> 
> Same here: "if (err < 0)"
> 
>> +               goto out_err;
>> +
>> +       return count;
>> +
>> +out_err:
>> +       kfree(overlay->dtbo);
>> +       overlay->dtbo = NULL;
>> +       overlay->dtbo_size = 0;
>> +
>> +       return err;
>> +}
> 
> Gr{oetje,eeting}s,
> 
>                        Geert
> 

I will apply the fixes you pointed out. Good catch.

> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                -- Linus Torvalds

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 16+ messages in thread

end of thread, other threads:[~2015-05-13 15:35 UTC | newest]

Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-12-03 11:23 [PATCH] OF: DT-Overlay configfs interface (v3) Pantelis Antoniou
     [not found] ` <1417605808-23327-1-git-send-email-pantelis.antoniou-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
2014-12-04 10:31   ` Grant Likely
     [not found]     ` <20141204103104.7D8E0C40992-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
2014-12-04 11:30       ` Pantelis Antoniou
     [not found]         ` <8795CB40-87D6-4683-8396-987A49E94574-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
2014-12-05 16:44           ` Grant Likely
     [not found]             ` <20141205164450.A9294C40B61-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
2014-12-05 17:58               ` Pantelis Antoniou
     [not found]                 ` <E64BA68B-A339-4BB2-B453-B8A9165CB9B7-OWPKS81ov/FWk0Htik3J/w@public.gmane.org>
2014-12-08 18:25                   ` Grant Likely
     [not found]                     ` <20141208182531.41BD6C40D73-WNowdnHR2B42iJbIjFUEsiwD8/FfD2ys@public.gmane.org>
2014-12-08 18:29                       ` Pantelis Antoniou
2014-12-05 19:53       ` Kees Cook
     [not found]         ` <CAGXu5jKZYrGpSwdpG8Pk=0N87Z1yT3S=utzCnQgvZ+4YeA2hKg-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-12-05 21:27           ` Pantelis Antoniou
     [not found]             ` <0CD21000-1C09-47D7-AE3B-3DAE942CB861-wVdstyuyKrO8r51toPun2/C9HSW9iNxf@public.gmane.org>
2014-12-05 22:01               ` Kees Cook
     [not found]                 ` <CAGXu5j+txFEtMSXab9LjcbvL5rhbPc94wimBMOyQj_Fcf3QZjw-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2014-12-08 17:39                   ` Grant Likely
2014-12-08 17:34           ` Grant Likely
2015-05-13 14:40   ` Geert Uytterhoeven
     [not found]     ` <CAMuHMdXST+xv7PquiP+HPk8WyydbNRh_0EmgPfOJCsitWZ0WuA-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-13 14:43       ` Pantelis Antoniou
2015-05-13 15:31   ` Geert Uytterhoeven
     [not found]     ` <CAMuHMdXdw42PpWe4C1Kf1uLz8NxHLX12+875qhoxoUUUi--C7Q-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2015-05-13 15:35       ` Pantelis Antoniou

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.