linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v11] firmware: google: Implement cbmem in sysfs driver
@ 2022-09-29 23:44 Jack Rosenthal
  2022-09-30  6:32 ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Jack Rosenthal @ 2022-09-29 23:44 UTC (permalink / raw)
  To: linux-kernel, chrome-platform, gregkh
  Cc: Jack Rosenthal, Stephen Boyd, Tzung-Bi Shih, Guenter Roeck,
	Julius Werner

The CBMEM area is a downward-growing memory region used by coreboot to
dynamically allocate tagged data structures ("CBMEM entries") that
remain resident during boot.

This implements a driver which exports access to the CBMEM entries
via sysfs under /sys/firmware/cbmem/<id>.

This implementation is quite versatile.  Examples of how it could be
used are given below:

* Tools like util/cbmem from the coreboot tree could use this driver
  instead of finding CBMEM in /dev/mem directly.  Alternatively,
  firmware developers debugging an issue may find the sysfs interface
  more ergonomic than the cbmem tool and choose to use it directly.

* The crossystem tool, which exposes verified boot variables, can use
  this driver to read the vboot work buffer.

* Tools which read the BIOS SPI flash (e.g., flashrom) can find the
  flash layout in CBMEM directly, which is significantly faster than
  searching the flash directly.

Write access is provided to all CBMEM regions via
/sys/firmware/cbmem/<id>/mem, as the existing cbmem tooling updates
this memory region, and envisioned use cases with crossystem
can benefit from updating memory regions.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <swboyd@chromium.org>
Cc: Tzung-Bi Shih <tzungbi@kernel.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
Tested-by: Jack Rosenthal <jrosenth@chromium.org>
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
---
Changes in v11:
* Changed /sys/firmware/coreboot/cbmem -> /sys/firmware/cbmem
* cbmem.c uses attribute groups to initialize files, which is much
  cleaner.  The attributes are added under the device kobject, which
  is now symlinked into /sys/firmware/cbmem.
* Changed documentation text as suggested by greg k-h

 .../ABI/testing/sysfs-firmware-cbmem          |  43 +++++
 drivers/firmware/google/Kconfig               |   8 +
 drivers/firmware/google/Makefile              |   3 +
 drivers/firmware/google/cbmem.c               | 180 ++++++++++++++++++
 drivers/firmware/google/coreboot_table.h      |  16 ++
 5 files changed, 250 insertions(+)
 create mode 100644 Documentation/ABI/testing/sysfs-firmware-cbmem
 create mode 100644 drivers/firmware/google/cbmem.c

diff --git a/Documentation/ABI/testing/sysfs-firmware-cbmem b/Documentation/ABI/testing/sysfs-firmware-cbmem
new file mode 100644
index 000000000000..f769104ac4cd
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-cbmem
@@ -0,0 +1,43 @@
+What:		/sys/firmware/cbmem/
+Date:		August 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		Coreboot provides a variety of data structures in CBMEM.  This
+		directory contains each CBMEM entry, which can be found via
+		Coreboot tables.
+
+What:		/sys/firmware/cbmem/<id>/
+Date:		August 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		Each CBMEM entry is given a directory based on the id
+		corresponding to the entry.  A list of ids known to coreboot can
+		be found in the coreboot source tree at
+		``src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h``.
+
+What:		/sys/firmware/cbmem/<id>/address
+Date:		August 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		The pyhsical memory address that the CBMEM entry's data begins
+		at.
+
+What:		/sys/firmware/cbmem/<id>/size
+Date:		August 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		The size of the CBMEM entry's data.
+
+What:		/sys/firmware/cbmem/<id>/id
+Date:		August 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		The CBMEM id corresponding to the entry.
+
+What:		/sys/firmware/cbmem/<id>/mem
+Date:		August 2022
+Contact:	Jack Rosenthal <jrosenth@chromium.org>
+Description:
+		A file exposing read/write access to the entry's data.  Note
+		that this file does not support mmap(), as coreboot
+		does not guarantee that the data will be page-aligned.
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..485934cc9663 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -19,6 +19,14 @@ config GOOGLE_SMI
 	  driver provides an interface for reading and writing NVRAM
 	  variables.
 
+config GOOGLE_CBMEM
+	tristate "CBMEM entries in sysfs"
+	depends on GOOGLE_COREBOOT_TABLE
+	help
+	  This option enables the kernel to search for Coreboot CBMEM
+	  entries, and expose the memory for each entry in sysfs under
+	  /sys/firmware/cbmem.
+
 config GOOGLE_COREBOOT_TABLE
 	tristate "Coreboot Table Access"
 	depends on HAS_IOMEM && (ACPI || OF)
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..8151e323cc43 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -7,5 +7,8 @@ obj-$(CONFIG_GOOGLE_MEMCONSOLE)            += memconsole.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT)   += memconsole-coreboot.o
 obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
 
+# Must come after coreboot_table.o, as this driver depends on that bus type.
+obj-$(CONFIG_GOOGLE_CBMEM)		+= cbmem.o
+
 vpd-sysfs-y := vpd.o vpd_decode.o
 obj-$(CONFIG_GOOGLE_VPD)		+= vpd-sysfs.o
diff --git a/drivers/firmware/google/cbmem.c b/drivers/firmware/google/cbmem.c
new file mode 100644
index 000000000000..fe814589f641
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,180 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google LLC
+ */
+
+#include <linux/device.h>
+#include <linux/init.h>
+#include <linux/io.h>
+#include <linux/kernel.h>
+#include <linux/kobject.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+#include <linux/sysfs.h>
+
+#include "coreboot_table.h"
+
+#define LB_TAG_CBMEM_ENTRY 0x31
+
+static struct kobject *cbmem_kobj;
+struct cbmem_entry {
+	char *mem_file_buf;
+	u32 size;
+	char *link_name;
+};
+
+static struct cbmem_entry *to_cbmem_entry(struct kobject *kobj)
+{
+	return dev_get_drvdata(kobj_to_dev(kobj));
+}
+
+static ssize_t mem_read(struct file *filp, struct kobject *kobj,
+			struct bin_attribute *bin_attr, char *buf, loff_t pos,
+			size_t count)
+{
+	struct cbmem_entry *entry = to_cbmem_entry(kobj);
+
+	return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
+				       entry->size);
+}
+
+static ssize_t mem_write(struct file *filp, struct kobject *kobj,
+			 struct bin_attribute *bin_attr, char *buf, loff_t pos,
+			 size_t count)
+{
+	struct cbmem_entry *entry = to_cbmem_entry(kobj);
+
+	if (pos < 0 || pos >= entry->size)
+		return -EINVAL;
+	if (count > entry->size - pos)
+		count = entry->size - pos;
+
+	memcpy(entry->mem_file_buf + pos, buf, count);
+	return count;
+}
+static BIN_ATTR_ADMIN_RW(mem, 0);
+
+static ssize_t address_show(struct device *dev, struct device_attribute *attr,
+			    char *buf)
+{
+	struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
+
+	return sysfs_emit(buf, "0x%llx\n", cbdev->cbmem_entry.address);
+}
+static DEVICE_ATTR_RO(address);
+
+static ssize_t size_show(struct device *dev, struct device_attribute *attr,
+			 char *buf)
+{
+	struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
+
+	return sysfs_emit(buf, "0x%x\n", cbdev->cbmem_entry.entry_size);
+}
+static DEVICE_ATTR_RO(size);
+
+static ssize_t id_show(struct device *dev, struct device_attribute *attr,
+		       char *buf)
+{
+	struct coreboot_device *cbdev = dev_to_coreboot_device(dev);
+
+	return sysfs_emit(buf, "0x%08x\n", cbdev->cbmem_entry.id);
+}
+static DEVICE_ATTR_RO(id);
+
+static struct attribute *attrs[] = {
+	&dev_attr_address.attr,
+	&dev_attr_size.attr,
+	&dev_attr_id.attr,
+	NULL,
+};
+
+static struct bin_attribute *bin_attrs[] = {
+	&bin_attr_mem,
+	NULL,
+};
+
+static const struct attribute_group cbmem_entry_group = {
+	.attrs = attrs,
+	.bin_attrs = bin_attrs,
+};
+
+static const struct attribute_group *dev_groups[] = {
+	&cbmem_entry_group,
+	NULL,
+};
+
+static int cbmem_entry_probe(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry;
+
+	entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
+	if (!entry)
+		return -ENOMEM;
+
+	dev_set_drvdata(&dev->dev, entry);
+	entry->mem_file_buf = devm_memremap(&dev->dev, dev->cbmem_entry.address,
+					    dev->cbmem_entry.entry_size,
+					    MEMREMAP_WB);
+	if (!entry->mem_file_buf)
+		return -ENOMEM;
+
+	entry->size = dev->cbmem_entry.entry_size;
+
+	entry->link_name = devm_kasprintf(&dev->dev, GFP_KERNEL, "%08x",
+					  dev->cbmem_entry.id);
+	if (!entry->link_name)
+		return -ENOMEM;
+
+	return sysfs_create_link(cbmem_kobj, &dev->dev.kobj, entry->link_name);
+}
+
+static void cbmem_entry_remove(struct coreboot_device *dev)
+{
+	struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
+
+	sysfs_remove_link(cbmem_kobj, entry->link_name);
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+	.probe = cbmem_entry_probe,
+	.remove = cbmem_entry_remove,
+	.drv = {
+		.name = "cbmem",
+		.owner = THIS_MODULE,
+		.dev_groups = dev_groups,
+	},
+	.tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+	int ret;
+
+	cbmem_kobj = kobject_create_and_add("cbmem", firmware_kobj);
+	if (!cbmem_kobj)
+		return -ENOMEM;
+
+	ret = coreboot_driver_register(&cbmem_entry_driver);
+	if (ret) {
+		kobject_put(cbmem_kobj);
+		return ret;
+	}
+
+	return 0;
+}
+module_init(cbmem_init);
+
+static void __exit cbmem_exit(void)
+{
+	kobject_put(cbmem_kobj);
+	coreboot_driver_unregister(&cbmem_entry_driver);
+}
+module_exit(cbmem_exit);
+
+MODULE_AUTHOR("Jack Rosenthal <jrosenth@chromium.org>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..4666fddf28d6 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -39,6 +39,16 @@ struct lb_cbmem_ref {
 	u64 cbmem_addr;
 };
 
+/* Corresponds to LB_TAG_CBMEM_ENTRY */
+struct lb_cbmem_entry {
+	u32 tag;
+	u32 size;
+
+	u64 address;
+	u32 entry_size;
+	u32 id;
+};
+
 /* Describes framebuffer setup by coreboot */
 struct lb_framebuffer {
 	u32 tag;
@@ -65,10 +75,16 @@ struct coreboot_device {
 	union {
 		struct coreboot_table_entry entry;
 		struct lb_cbmem_ref cbmem_ref;
+		struct lb_cbmem_entry cbmem_entry;
 		struct lb_framebuffer framebuffer;
 	};
 };
 
+static inline struct coreboot_device *dev_to_coreboot_device(struct device *dev)
+{
+	return container_of(dev, struct coreboot_device, dev);
+}
+
 /* A driver for handling devices described in coreboot tables. */
 struct coreboot_driver {
 	int (*probe)(struct coreboot_device *);
-- 
2.38.0.rc1.362.ged0d419d3c-goog


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

* Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver
  2022-09-29 23:44 [PATCH v11] firmware: google: Implement cbmem in sysfs driver Jack Rosenthal
@ 2022-09-30  6:32 ` Greg KH
  2022-09-30 22:14   ` Jack Rosenthal
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2022-09-30  6:32 UTC (permalink / raw)
  To: Jack Rosenthal
  Cc: linux-kernel, chrome-platform, Stephen Boyd, Tzung-Bi Shih,
	Guenter Roeck, Julius Werner

On Thu, Sep 29, 2022 at 05:44:32PM -0600, Jack Rosenthal wrote:
> The CBMEM area is a downward-growing memory region used by coreboot to
> dynamically allocate tagged data structures ("CBMEM entries") that
> remain resident during boot.
> 
> This implements a driver which exports access to the CBMEM entries
> via sysfs under /sys/firmware/cbmem/<id>.
> 
> This implementation is quite versatile.  Examples of how it could be
> used are given below:
> 
> * Tools like util/cbmem from the coreboot tree could use this driver
>   instead of finding CBMEM in /dev/mem directly.  Alternatively,
>   firmware developers debugging an issue may find the sysfs interface
>   more ergonomic than the cbmem tool and choose to use it directly.
> 
> * The crossystem tool, which exposes verified boot variables, can use
>   this driver to read the vboot work buffer.
> 
> * Tools which read the BIOS SPI flash (e.g., flashrom) can find the
>   flash layout in CBMEM directly, which is significantly faster than
>   searching the flash directly.
> 
> Write access is provided to all CBMEM regions via
> /sys/firmware/cbmem/<id>/mem, as the existing cbmem tooling updates
> this memory region, and envisioned use cases with crossystem
> can benefit from updating memory regions.
> 
> Link: https://issuetracker.google.com/239604743
> Cc: Stephen Boyd <swboyd@chromium.org>
> Cc: Tzung-Bi Shih <tzungbi@kernel.org>
> Reviewed-by: Guenter Roeck <groeck@chromium.org>
> Reviewed-by: Julius Werner <jwerner@chromium.org>
> Tested-by: Jack Rosenthal <jrosenth@chromium.org>
> Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
> ---
> Changes in v11:
> * Changed /sys/firmware/coreboot/cbmem -> /sys/firmware/cbmem
> * cbmem.c uses attribute groups to initialize files, which is much
>   cleaner.  The attributes are added under the device kobject, which
>   is now symlinked into /sys/firmware/cbmem.

symlink?  Ick, no, do not do that at all please.

As these are device attributes, just stick with them.  Don't do a crazy
symlink into a non-device-attribute portion of the sysfs tree, by doing
that you break all userspace tools and stuff like libudev will never
even see these attributes.


> * Changed documentation text as suggested by greg k-h
> 
>  .../ABI/testing/sysfs-firmware-cbmem          |  43 +++++
>  drivers/firmware/google/Kconfig               |   8 +
>  drivers/firmware/google/Makefile              |   3 +
>  drivers/firmware/google/cbmem.c               | 180 ++++++++++++++++++
>  drivers/firmware/google/coreboot_table.h      |  16 ++
>  5 files changed, 250 insertions(+)
>  create mode 100644 Documentation/ABI/testing/sysfs-firmware-cbmem
>  create mode 100644 drivers/firmware/google/cbmem.c
> 
> diff --git a/Documentation/ABI/testing/sysfs-firmware-cbmem b/Documentation/ABI/testing/sysfs-firmware-cbmem
> new file mode 100644
> index 000000000000..f769104ac4cd
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-cbmem
> @@ -0,0 +1,43 @@
> +What:		/sys/firmware/cbmem/
> +Date:		August 2022
> +Contact:	Jack Rosenthal <jrosenth@chromium.org>
> +Description:
> +		Coreboot provides a variety of data structures in CBMEM.  This
> +		directory contains each CBMEM entry, which can be found via
> +		Coreboot tables.

What happened to the coreboot name?

Why cbmem?  What is CBMEM?

And just stick with the attributes under the cbmem coreboot device in
the device tree, don't use /sys/firmware/.

Also, I asked before, but some note about "exposing all of these bios
values to userspace is not a security issue at all" would be nice, if
only to point at in a few years and say "wow we were naive"...

thanks,

greg k-h

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

* Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver
  2022-09-30  6:32 ` Greg KH
@ 2022-09-30 22:14   ` Jack Rosenthal
  2022-10-01  7:33     ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Jack Rosenthal @ 2022-09-30 22:14 UTC (permalink / raw)
  To: Greg KH
  Cc: linux-kernel, chrome-platform, Stephen Boyd, Tzung-Bi Shih,
	Guenter Roeck, Julius Werner

On 2022-09-30 at 08:32 +0200, Greg KH wrote:
> symlink?  Ick, no, do not do that at all please.
> 
> As these are device attributes, just stick with them.  Don't do a crazy
> symlink into a non-device-attribute portion of the sysfs tree, by doing
> that you break all userspace tools and stuff like libudev will never
> even see these attributes.

I guess I can fill in some info here about the use case needed:
userspace tools (in this case, a tool called "crossystem") needs to look
up a CBMEM entry by ID and read it.  So, being able to find a fixed path
like /sys/firmware/cbmem/<id>/mem is significantly easier than scanning
all /sys/bus/coreboot/devices/coreboot*/id to find the right device
first.

What exactly do we break here by adding symlinks?  udev won't look into
/sys/firmware, right?

Or, is there another good alternative that we could use to find a CBMEM
entry by its id without needing to scan thru all coreboot bus type
devices?  Setting the device name to something more predictable (e.g.,
"cbmem-<id>") would require the coreboot bus type to "look ahead" and
notice it's a CBMEM entry before registering the device, which wouldn't
exactly be all that clean.

> > +What:		/sys/firmware/cbmem/
> > +Date:		August 2022
> > +Contact:	Jack Rosenthal <jrosenth@chromium.org>
> > +Description:
> > +		Coreboot provides a variety of data structures in CBMEM.  This
> > +		directory contains each CBMEM entry, which can be found via
> > +		Coreboot tables.
> 
> What happened to the coreboot name?

I removed it as it seemed like from your last message you didn't want
it?

> Why cbmem?  What is CBMEM?

I can add this to the next patch once I get clarifications on the above.

> Also, I asked before, but some note about "exposing all of these bios
> values to userspace is not a security issue at all" would be nice, if
> only to point at in a few years and say "wow we were naive"...

Right, I'll add this too.

Thanks,

Jack

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

* Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver
  2022-09-30 22:14   ` Jack Rosenthal
@ 2022-10-01  7:33     ` Greg KH
  2022-10-04  0:56       ` Julius Werner
  0 siblings, 1 reply; 6+ messages in thread
From: Greg KH @ 2022-10-01  7:33 UTC (permalink / raw)
  To: Jack Rosenthal
  Cc: linux-kernel, chrome-platform, Stephen Boyd, Tzung-Bi Shih,
	Guenter Roeck, Julius Werner

On Fri, Sep 30, 2022 at 04:14:41PM -0600, Jack Rosenthal wrote:
> On 2022-09-30 at 08:32 +0200, Greg KH wrote:
> > symlink?  Ick, no, do not do that at all please.
> > 
> > As these are device attributes, just stick with them.  Don't do a crazy
> > symlink into a non-device-attribute portion of the sysfs tree, by doing
> > that you break all userspace tools and stuff like libudev will never
> > even see these attributes.
> 
> I guess I can fill in some info here about the use case needed:
> userspace tools (in this case, a tool called "crossystem") needs to look
> up a CBMEM entry by ID and read it.  So, being able to find a fixed path
> like /sys/firmware/cbmem/<id>/mem is significantly easier than scanning
> all /sys/bus/coreboot/devices/coreboot*/id to find the right device
> first.

No, this is a device, make these device attributes, don't polute sysfs
with random symlinks from a device into the firmware location, that's
not ok.

And again, your current code means that tools like udev and libraries
will not see these attributes at all.  Stick with what every other
device in the kernel does please, consistancy is good.

> What exactly do we break here by adding symlinks?  udev won't look into
> /sys/firmware, right?

Exactly.  You want that to work.

> Or, is there another good alternative that we could use to find a CBMEM
> entry by its id without needing to scan thru all coreboot bus type
> devices?  Setting the device name to something more predictable (e.g.,
> "cbmem-<id>") would require the coreboot bus type to "look ahead" and
> notice it's a CBMEM entry before registering the device, which wouldn't
> exactly be all that clean.

All devices on a bus can call themselves whatever they want, of course
they should be naming themselves based on their device type, why
wouldn't they?

Or put an attribute of the type in the directory of the coreboot device
and iterate on that.  It's a simple udev rule for matching on an
attribute value on a bus.

> > > +What:		/sys/firmware/cbmem/
> > > +Date:		August 2022
> > > +Contact:	Jack Rosenthal <jrosenth@chromium.org>
> > > +Description:
> > > +		Coreboot provides a variety of data structures in CBMEM.  This
> > > +		directory contains each CBMEM entry, which can be found via
> > > +		Coreboot tables.
> > 
> > What happened to the coreboot name?
> 
> I removed it as it seemed like from your last message you didn't want
> it?

I do not recall saying that, I was just confused that it had not been
used before.

Please do not use cbmem.

thanks,

greg k-h

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

* Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver
  2022-10-01  7:33     ` Greg KH
@ 2022-10-04  0:56       ` Julius Werner
  2022-10-04  8:45         ` Greg KH
  0 siblings, 1 reply; 6+ messages in thread
From: Julius Werner @ 2022-10-04  0:56 UTC (permalink / raw)
  To: Greg KH
  Cc: Jack Rosenthal, LKML, chrome-platform, Stephen Boyd,
	Tzung-Bi Shih, Guenter Roeck, Julius Werner

> No, this is a device, make these device attributes, don't polute sysfs
> with random symlinks from a device into the firmware location, that's
> not ok.
>
> And again, your current code means that tools like udev and libraries
> will not see these attributes at all.  Stick with what every other
> device in the kernel does please, consistancy is good.
>
> > What exactly do we break here by adding symlinks?  udev won't look into
> > /sys/firmware, right?
>
> Exactly.  You want that to work.

I feel like there might be some misunderstanding here... we're not
adding these symlinks for udev. Of course, if someone wanted to match
one of these entries with udev, they would do that by matching the
device entry directly, and there is an `id` device attribute for that.
We're not breaking that use case, and for those tools we totally agree
that the normal device node matching by attribute makes most sense.

But we have a different use case where userspace tools that need to
access one specific ID are frequently called on-demand (e.g. from the
command line or by another process), not only once on device
registration like udev. For that kind of use case, always searching
through every single node to find the right ID is cumbersome and
time-wasting. We're just trying to create a convenience symlink to
make that use case easier and faster. The sysfs device framework
itself already has plenty of convenience symlinks like that... e.g. if
I wanted to find all devices on the coreboot bus, without symlinks I'd
have to iterate through everything in /sys/devices/ and compare each
`subsystem` attribute to check if it matches that bus. But for
convenience sysfs automatically creates symlinks in
/sys/bus/coreboot/devices/. Not sure how this case is so different?

If you really don't like the links, do you have an alternative
suggestion how we could allow our tools to find a single ID quickly
without having to iterate through all entries every time? (I guess
using the device name works but it's a bit cumbersome because then the
bus driver has to dig in deep and inspect device-type-specific details
on registration that would normally only be handled by the device
driver.)

> Also, I asked before, but some note about "exposing all of these bios values to userspace is not a security issue at all" would be nice, if only to point at in a few years and say "wow we were naive"...

To clarify this one some more, it's not so much that there can be no
security implications at all from this, but more that these spaces
have always been accessible through the /dev/mem interface already. So
we're not opening any new holes, we're just providing a new Kconfig
that allows exposing a subset of the attack surface that can already
be exposed via /dev/mem in a more controlled way. Of course users
should still understand the implications of each of these nodes before
they're granting access permissions to it, that's why we're creating
them with umask 0600.

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

* Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver
  2022-10-04  0:56       ` Julius Werner
@ 2022-10-04  8:45         ` Greg KH
  0 siblings, 0 replies; 6+ messages in thread
From: Greg KH @ 2022-10-04  8:45 UTC (permalink / raw)
  To: Julius Werner
  Cc: Jack Rosenthal, LKML, chrome-platform, Stephen Boyd,
	Tzung-Bi Shih, Guenter Roeck

On Mon, Oct 03, 2022 at 05:56:35PM -0700, Julius Werner wrote:
> > No, this is a device, make these device attributes, don't polute sysfs
> > with random symlinks from a device into the firmware location, that's
> > not ok.
> >
> > And again, your current code means that tools like udev and libraries
> > will not see these attributes at all.  Stick with what every other
> > device in the kernel does please, consistancy is good.
> >
> > > What exactly do we break here by adding symlinks?  udev won't look into
> > > /sys/firmware, right?
> >
> > Exactly.  You want that to work.
> 
> I feel like there might be some misunderstanding here... we're not
> adding these symlinks for udev. Of course, if someone wanted to match
> one of these entries with udev, they would do that by matching the
> device entry directly, and there is an `id` device attribute for that.
> We're not breaking that use case, and for those tools we totally agree
> that the normal device node matching by attribute makes most sense.

Ok, if you aren't adding a symlink for udev, then don't add a symlink at
all as no one will need it :)

symlinks are either for showing a real device relationship or for when
we mess things up and need to keep a compatible view of the sysfs tree
because we moved things and userspace tools would break if we didn't.
Don't add them if you do not absolutely need them.

> But we have a different use case where userspace tools that need to
> access one specific ID are frequently called on-demand (e.g. from the
> command line or by another process), not only once on device
> registration like udev. For that kind of use case, always searching
> through every single node to find the right ID is cumbersome and
> time-wasting.

Really?  Iterating over all devices in /sys/bus/BUSTYPE/devices/ is
slow?  Have you tested it and found a performance issue?  This is all in
memory, how slow is it really?

> We're just trying to create a convenience symlink to
> make that use case easier and faster. The sysfs device framework
> itself already has plenty of convenience symlinks like that... e.g. if
> I wanted to find all devices on the coreboot bus, without symlinks I'd
> have to iterate through everything in /sys/devices/ and compare each
> `subsystem` attribute to check if it matches that bus. But for
> convenience sysfs automatically creates symlinks in
> /sys/bus/coreboot/devices/. Not sure how this case is so different?

See above.  You are a device, use the bus symlinks properly for your bus
and all is fine.  Don't try to link to /sys/firmware/ just because your
userspace programmers don't like to use a loop :)

> If you really don't like the links, do you have an alternative
> suggestion how we could allow our tools to find a single ID quickly
> without having to iterate through all entries every time? (I guess
> using the device name works but it's a bit cumbersome because then the
> bus driver has to dig in deep and inspect device-type-specific details
> on registration that would normally only be handled by the device
> driver.)

Again, just go over the list of devices in your bus and all is good.
How slow/complex is it really and why is your tiny bus/device special
from all the rest of the kernel to deserve special treatment here?  What
is so unique about it?

> > Also, I asked before, but some note about "exposing all of these bios values to userspace is not a security issue at all" would be nice, if only to point at in a few years and say "wow we were naive"...
> 
> To clarify this one some more, it's not so much that there can be no
> security implications at all from this, but more that these spaces
> have always been accessible through the /dev/mem interface already. So
> we're not opening any new holes, we're just providing a new Kconfig
> that allows exposing a subset of the attack surface that can already
> be exposed via /dev/mem in a more controlled way. Of course users
> should still understand the implications of each of these nodes before
> they're granting access permissions to it, that's why we're creating
> them with umask 0600.

/dev/mem is locked down on many systems for good reason.  You are now
allowing systems that previously did not let anyone access /dev/mem to
read these values even if they are root.  I hope you have updated your
selinux policies to handle this properly, and again, please document the
heck out of it so that we all know what this is really doing.

thanks,

greg k-h

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

end of thread, other threads:[~2022-10-04  8:46 UTC | newest]

Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-09-29 23:44 [PATCH v11] firmware: google: Implement cbmem in sysfs driver Jack Rosenthal
2022-09-30  6:32 ` Greg KH
2022-09-30 22:14   ` Jack Rosenthal
2022-10-01  7:33     ` Greg KH
2022-10-04  0:56       ` Julius Werner
2022-10-04  8:45         ` Greg KH

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).