Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752352AbaKYK3U (ORCPT ); Tue, 25 Nov 2014 05:29:20 -0500 Received: from mail-wi0-f174.google.com ([209.85.212.174]:42696 "EHLO mail-wi0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752211AbaKYK3K (ORCPT ); Tue, 25 Nov 2014 05:29:10 -0500 From: Grant Likely Subject: Re: [PATCH v8 3/8] OF: DT-Overlay configfs interface (v2) To: Pantelis Antoniou Cc: Rob Herring , Stephen Warren , Matt Porter , Koen Kooi , Greg Kroah-Hartman , Alison Chaiken , Dinh Nguyen , Jan Lubbe , Alexander Sverdlin , Michael Stickel , Guenter Roeck , Dirk Behme , Alan Tull , Sascha Hauer , Michael Bohan , Ionut Nicu , Michal Simek , Matt Ranostay , Joel Becker , devicetree@vger.kernel.org, Wolfram Sang , linux-i2c@vger.kernel.org, Mark Brown , linux-spi@vger.kernel.org, linux-kernel@vger.kernel.org, Pete Popov , Dan Malek , Georgi Vlaev , Pantelis Antoniou , Pantelis Antoniou In-Reply-To: <1414528565-10907-4-git-send-email-pantelis.antoniou@konsulko.com> References: <1414528565-10907-1-git-send-email-pantelis.antoniou@konsulko.com> <1414528565-10907-4-git-send-email-pantelis.antoniou@konsulko.com> Date: Tue, 25 Nov 2014 10:28:44 +0000 Message-Id: <20141125102844.79BAEC44089@trevor.secretlab.ca> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi Pantelis, Comments below... On Tue, 28 Oct 2014 22:36:00 +0200 , Pantelis Antoniou wrote: > Add a runtime interface to using configfs for generic device tree overlay > usage. > > A device-tree configfs entry is created in /config/device-tree/overlays > > * 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. > > To remove it simply rmdir the directory. > > # rmdir /config/device-tree/overlays/foo The above is documentation on how to use it, but it isn't a good commit message. The above should be moved into a documentation file (if it isn't already and I've missed it) and the commit message should give detail about what was needed, what was changed to make it happen, and what the result of the new code is. > > Changes since v1: > * of_resolve() -> of_resolve_phandles(). > > Signed-off-by: Pantelis Antoniou > --- > drivers/of/Kconfig | 7 ++ > drivers/of/Makefile | 1 + > drivers/of/configfs.c | 340 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 3 files changed, 348 insertions(+) > create mode 100644 drivers/of/configfs.c > > diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig > index aa315c4..d59ba40 100644 > --- a/drivers/of/Kconfig > +++ b/drivers/of/Kconfig > @@ -90,4 +90,11 @@ config OF_OVERLAY > select OF_DEVICE > select OF_RESOLVE > > +config OF_CONFIGFS > + bool "OpenFirmware Overlay ConfigFS interface" Despite all the APIs being prefixed with OpenFirmware, this isn't an OpenFirmware interface, it is a device tree interface. > + select CONFIGFS_FS > + select OF_OVERLAY > + help > + Enable a simple user-space driver DT overlay interface. > + > endmenu # OF > diff --git a/drivers/of/Makefile b/drivers/of/Makefile > index 1bfe462..6d12949 100644 > --- a/drivers/of/Makefile > +++ b/drivers/of/Makefile > @@ -15,6 +15,7 @@ obj-$(CONFIG_OF_MTD) += of_mtd.o > obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o > obj-$(CONFIG_OF_RESOLVE) += resolver.o > obj-$(CONFIG_OF_OVERLAY) += overlay.o > +obj-$(CONFIG_OF_CONFIGFS) += configfs.o Tip: Insert lines in the middle of the block, roughly alphabetically sorted. It cuts down on merge conflicts that way. > > CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt > CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt > diff --git a/drivers/of/configfs.c b/drivers/of/configfs.c > new file mode 100644 > index 0000000..932f572 > --- /dev/null > +++ b/drivers/of/configfs.c > @@ -0,0 +1,340 @@ > +/* > + * Configfs entries for device-tree > + * > + * Copyright (C) 2013 - Pantelis Antoniou > + * > + * 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 > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > + > +#include "of_private.h" > + > +#ifdef CONFIG_OF_OVERLAY ??? This file shouldn't even be compiled if CONFIG_OF_OVERLAY isn't selected. Why is this here? > + > +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((void *)blob, &overlay->overlay); blob is already void* > + 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); World writable? Am I reading this correctly? DT modifications are privileged. A user can potentially get arbitrary access to i2c, spi, gpio or other things that shouldn't be allowed. This feature give access right into the Linux driver model. Before this can be merged, it needs to be locked down, and there needs to be documentation about how it is locked down. Owned by root is only the first step, there also needs to be some rules about which nodes can be modified by the configfs interface. By default think no modifications should be allowed on a tree unless there are properties somewhere in the tree that explicitly allow modifications to be performed. Regardless, there needs to be a proposal made about the security model so that it can be discussed and analyzed by folks better versed in security that either of us. I would like to get Kees Cook to take a look at what we are doing here. > +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, > +}; > + > +#endif /* CONFIG_OF_OVERLAY */ > + > +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[] = { > +#ifdef CONFIG_OF_OVERLAY > + &of_cfs_overlay_group, > +#endif > + 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); > +#ifdef CONFIG_OF_OVERLAY > + config_group_init_type_name(&of_cfs_overlay_group, "overlays", > + &overlays_type); > +#endif > + > + 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 linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/