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/coreboot/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/coreboot/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 <[email protected]>
Cc: Tzung-Bi Shih <[email protected]>
Reviewed-by: Guenter Roeck <[email protected]>
Reviewed-by: Julius Werner <[email protected]>
Tested-by: Jack Rosenthal <[email protected]>
Signed-off-by: Jack Rosenthal <[email protected]>
---
.../ABI/testing/sysfs-firmware-coreboot | 49 ++++
drivers/firmware/google/Kconfig | 8 +
drivers/firmware/google/Makefile | 3 +
drivers/firmware/google/cbmem.c | 225 ++++++++++++++++++
drivers/firmware/google/coreboot_table.c | 10 +
drivers/firmware/google/coreboot_table.h | 16 ++
6 files changed, 311 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
create mode 100644 drivers/firmware/google/cbmem.c
diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
new file mode 100644
index 000000000000..c003eb515d0c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
@@ -0,0 +1,49 @@
+What: /sys/firmware/coreboot/
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ Kernel objects associated with the Coreboot-based BIOS firmware.
+
+What: /sys/firmware/coreboot/cbmem/
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ Coreboot provides a variety of information in CBMEM. This
+ directory contains each CBMEM entry, which can be found via
+ Coreboot tables.
+
+What: /sys/firmware/coreboot/cbmem/<id>/
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+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/coreboot/cbmem/<id>/address
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ The memory address that the CBMEM entry's data begins at.
+
+What: /sys/firmware/coreboot/cbmem/<id>/size
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ The size of the data being stored.
+
+What: /sys/firmware/coreboot/cbmem/<id>/id
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ The CBMEM id corresponding to the entry.
+
+What: /sys/firmware/coreboot/cbmem/<id>/mem
+Date: August 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ A file exposing read/write access to the entry's data. Note
+ that this file does not support mmap(), and should be used for
+ basic data access only. Users requiring mmap() should read the
+ address and size files, and mmap() /dev/mem.
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..b0f7a24fd90a 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/coreboot/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..835950a14fa6
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,225 @@
+// 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;
+struct cbmem_entry_attr {
+ struct kobj_attribute kobj_attr;
+ struct cbmem_entry *entry;
+};
+
+struct cbmem_entry {
+ struct kobject *kobj;
+ struct coreboot_device *dev;
+ struct bin_attribute mem_file;
+ char *mem_file_buf;
+ struct cbmem_entry_attr address_file;
+ struct cbmem_entry_attr size_file;
+ struct cbmem_entry_attr id_file;
+};
+
+static struct cbmem_entry_attr *to_cbmem_entry_attr(struct kobj_attribute *a)
+{
+ return container_of(a, struct cbmem_entry_attr, kobj_attr);
+}
+
+static ssize_t cbmem_entry_mem_read(struct file *filp, struct kobject *kobp,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t pos, size_t count)
+{
+ struct cbmem_entry *entry = bin_attr->private;
+
+ return memory_read_from_buffer(buf, count, &pos, entry->mem_file_buf,
+ bin_attr->size);
+}
+
+static ssize_t cbmem_entry_mem_write(struct file *filp, struct kobject *kobp,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t pos, size_t count)
+{
+ struct cbmem_entry *entry = bin_attr->private;
+
+ if (pos < 0 || pos >= bin_attr->size)
+ return -EINVAL;
+ if (count > bin_attr->size - pos)
+ count = bin_attr->size - pos;
+
+ memcpy(entry->mem_file_buf + pos, buf, count);
+ return count;
+}
+
+static ssize_t cbmem_entry_address_show(struct kobject *kobj,
+ struct kobj_attribute *a, char *buf)
+{
+ struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+ return sysfs_emit(buf, "0x%llx\n",
+ entry_attr->entry->dev->cbmem_entry.address);
+}
+
+static ssize_t cbmem_entry_size_show(struct kobject *kobj,
+ struct kobj_attribute *a, char *buf)
+{
+ struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+ return sysfs_emit(buf, "0x%x\n",
+ entry_attr->entry->dev->cbmem_entry.entry_size);
+}
+
+static ssize_t cbmem_entry_id_show(struct kobject *kobj,
+ struct kobj_attribute *a, char *buf)
+{
+ struct cbmem_entry_attr *entry_attr = to_cbmem_entry_attr(a);
+
+ return sysfs_emit(buf, "0x%08x\n",
+ entry_attr->entry->dev->cbmem_entry.id);
+}
+
+static int cbmem_entry_probe(struct coreboot_device *dev)
+{
+ struct cbmem_entry *entry;
+ char *kobj_name;
+ int ret;
+
+ entry = devm_kzalloc(&dev->dev, sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ dev_set_drvdata(&dev->dev, entry);
+ entry->dev = dev;
+ entry->mem_file_buf =
+ devm_memremap(&entry->dev->dev, entry->dev->cbmem_entry.address,
+ entry->dev->cbmem_entry.entry_size, MEMREMAP_WB);
+ if (!entry->mem_file_buf)
+ return -ENOMEM;
+
+ kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL, "%08x",
+ entry->dev->cbmem_entry.id);
+ if (!kobj_name)
+ return -ENOMEM;
+
+ entry->kobj = kobject_create_and_add(kobj_name, cbmem_kobj);
+ if (!entry->kobj)
+ return -ENOMEM;
+
+ sysfs_bin_attr_init(&entry->mem_file);
+ entry->mem_file.attr.name = "mem";
+ entry->mem_file.attr.mode = 0600;
+ entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
+ entry->mem_file.read = cbmem_entry_mem_read;
+ entry->mem_file.write = cbmem_entry_mem_write;
+ entry->mem_file.private = entry;
+ ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
+ if (ret)
+ goto free_kobj;
+
+ sysfs_attr_init(&entry->address_file.kobj_attr.attr);
+ entry->address_file.kobj_attr.attr.name = "address";
+ entry->address_file.kobj_attr.attr.mode = 0444;
+ entry->address_file.kobj_attr.show = cbmem_entry_address_show;
+ entry->address_file.entry = entry;
+ ret = sysfs_create_file(entry->kobj,
+ &entry->address_file.kobj_attr.attr);
+ if (ret)
+ goto free_mem_file;
+
+ sysfs_attr_init(&entry->size_file.kobj_attr.attr);
+ entry->size_file.kobj_attr.attr.name = "size";
+ entry->size_file.kobj_attr.attr.mode = 0444;
+ entry->size_file.kobj_attr.show = cbmem_entry_size_show;
+ entry->size_file.entry = entry;
+ ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+ if (ret)
+ goto free_address_file;
+
+ sysfs_attr_init(&entry->id_file.kobj_attr.attr);
+ entry->id_file.kobj_attr.attr.name = "id";
+ entry->id_file.kobj_attr.attr.mode = 0444;
+ entry->id_file.kobj_attr.show = cbmem_entry_id_show;
+ entry->id_file.entry = entry;
+ ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+ if (ret)
+ goto free_size_file;
+
+ return 0;
+
+free_size_file:
+ sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+free_address_file:
+ sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+free_mem_file:
+ sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+free_kobj:
+ kobject_put(entry->kobj);
+ return ret;
+}
+
+static void cbmem_entry_remove(struct coreboot_device *dev)
+{
+ struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
+
+ sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+ sysfs_remove_file(entry->kobj, &entry->address_file.kobj_attr.attr);
+ sysfs_remove_file(entry->kobj, &entry->size_file.kobj_attr.attr);
+ sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+ kobject_put(entry->kobj);
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+ .probe = cbmem_entry_probe,
+ .remove = cbmem_entry_remove,
+ .drv = {
+ .name = "cbmem",
+ .owner = THIS_MODULE,
+ },
+ .tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+ int ret;
+
+ cbmem_kobj = kobject_create_and_add("cbmem", coreboot_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 <[email protected]>");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.c b/drivers/firmware/google/coreboot_table.c
index c52bcaa9def6..a3e2720e4638 100644
--- a/drivers/firmware/google/coreboot_table.c
+++ b/drivers/firmware/google/coreboot_table.c
@@ -14,16 +14,21 @@
#include <linux/init.h>
#include <linux/io.h>
#include <linux/kernel.h>
+#include <linux/kobject.h>
#include <linux/module.h>
#include <linux/of.h>
#include <linux/platform_device.h>
#include <linux/slab.h>
+#include <linux/sysfs.h>
#include "coreboot_table.h"
#define CB_DEV(d) container_of(d, struct coreboot_device, dev)
#define CB_DRV(d) container_of(d, struct coreboot_driver, drv)
+struct kobject *coreboot_kobj;
+EXPORT_SYMBOL(coreboot_kobj);
+
static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
{
struct coreboot_device *device = CB_DEV(dev);
@@ -157,6 +162,10 @@ static int coreboot_table_probe(struct platform_device *pdev)
}
memunmap(ptr);
+ coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
+ if (!coreboot_kobj)
+ return -ENOMEM;
+
return ret;
}
@@ -170,6 +179,7 @@ static int coreboot_table_remove(struct platform_device *pdev)
{
bus_for_each_dev(&coreboot_bus_type, NULL, NULL, __cb_dev_unregister);
bus_unregister(&coreboot_bus_type);
+ kobject_put(coreboot_kobj);
return 0;
}
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..76c31e6e5376 100644
--- a/drivers/firmware/google/coreboot_table.h
+++ b/drivers/firmware/google/coreboot_table.h
@@ -14,6 +14,11 @@
#include <linux/device.h>
+struct kobject;
+
+/* This is /sys/firmware/coreboot */
+extern struct kobject *coreboot_kobj;
+
/* Coreboot table header structure */
struct coreboot_table_header {
char signature[4];
@@ -39,6 +44,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,6 +80,7 @@ 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;
};
};
--
2.37.3.998.g577e59143f-goog
On Tue, Sep 27, 2022 at 02:55:51PM -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/coreboot/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/coreboot/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 <[email protected]>
> Cc: Tzung-Bi Shih <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> Reviewed-by: Julius Werner <[email protected]>
> Tested-by: Jack Rosenthal <[email protected]>
> Signed-off-by: Jack Rosenthal <[email protected]>
> ---
> .../ABI/testing/sysfs-firmware-coreboot | 49 ++++
> drivers/firmware/google/Kconfig | 8 +
> drivers/firmware/google/Makefile | 3 +
> drivers/firmware/google/cbmem.c | 225 ++++++++++++++++++
> drivers/firmware/google/coreboot_table.c | 10 +
> drivers/firmware/google/coreboot_table.h | 16 ++
> 6 files changed, 311 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
> create mode 100644 drivers/firmware/google/cbmem.c
>
Hi,
This is the friendly patch-bot of Greg Kroah-Hartman. You have sent him
a patch that has triggered this response. He used to manually respond
to these common problems, but in order to save his sanity (he kept
writing the same thing over and over, yet to different people), I was
created. Hopefully you will not take offence and will fix the problem
in your patch and resubmit it so that it can be accepted into the Linux
kernel tree.
You are receiving this message because of the following common error(s)
as indicated below:
- This looks like a new version of a previously submitted patch, but you
did not list below the --- line any changes from the previous version.
Please read the section entitled "The canonical patch format" in the
kernel file, Documentation/SubmittingPatches for what needs to be done
here to properly describe this.
If you wish to discuss this problem further, or you have questions about
how to resolve this issue, please feel free to respond to this email and
Greg will reply once he has dug out from the pending patches received
from other developers.
thanks,
greg k-h's patch email bot
On Tue, Sep 27, 2022 at 02:55:51PM -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/coreboot/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/coreboot/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 <[email protected]>
> Cc: Tzung-Bi Shih <[email protected]>
> Reviewed-by: Guenter Roeck <[email protected]>
> Reviewed-by: Julius Werner <[email protected]>
> Tested-by: Jack Rosenthal <[email protected]>
> Signed-off-by: Jack Rosenthal <[email protected]>
> ---
Why is this a RESEND?
I don't remember seeing any of the 10 previous versions, but hey, I
can't remember much...
> .../ABI/testing/sysfs-firmware-coreboot | 49 ++++
> drivers/firmware/google/Kconfig | 8 +
> drivers/firmware/google/Makefile | 3 +
> drivers/firmware/google/cbmem.c | 225 ++++++++++++++++++
> drivers/firmware/google/coreboot_table.c | 10 +
> drivers/firmware/google/coreboot_table.h | 16 ++
> 6 files changed, 311 insertions(+)
> create mode 100644 Documentation/ABI/testing/sysfs-firmware-coreboot
> create mode 100644 drivers/firmware/google/cbmem.c
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-coreboot b/Documentation/ABI/testing/sysfs-firmware-coreboot
> new file mode 100644
> index 000000000000..c003eb515d0c
> --- /dev/null
> +++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
> @@ -0,0 +1,49 @@
> +What: /sys/firmware/coreboot/
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + Kernel objects associated with the Coreboot-based BIOS firmware.
Why is "coreboot" needed here? Will this work for all coreboot
implementations?
Why is coreboot files being in a
> +What: /sys/firmware/coreboot/cbmem/<id>/address
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + The memory address that the CBMEM entry's data begins at.
Raw memory addresses? Why?
> +What: /sys/firmware/coreboot/cbmem/<id>/size
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + The size of the data being stored.
Nothing is being "stored" here, it is being read.
> +What: /sys/firmware/coreboot/cbmem/<id>/mem
> +Date: August 2022
> +Contact: Jack Rosenthal <[email protected]>
> +Description:
> + A file exposing read/write access to the entry's data. Note
> + that this file does not support mmap(), and should be used for
> + basic data access only. Users requiring mmap() should read the
> + address and size files, and mmap() /dev/mem.
Why doesn't mmap work?
> diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
> index 983e07dc022e..b0f7a24fd90a 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/coreboot/cbmem.
What's the security implications of exposing all of this information?
> + sysfs_bin_attr_init(&entry->mem_file);
> + entry->mem_file.attr.name = "mem";
> + entry->mem_file.attr.mode = 0600;
> + entry->mem_file.size = entry->dev->cbmem_entry.entry_size;
> + entry->mem_file.read = cbmem_entry_mem_read;
> + entry->mem_file.write = cbmem_entry_mem_write;
> + entry->mem_file.private = entry;
> + ret = sysfs_create_bin_file(entry->kobj, &entry->mem_file);
> + if (ret)
> + goto free_kobj;
> +
> + sysfs_attr_init(&entry->address_file.kobj_attr.attr);
> + entry->address_file.kobj_attr.attr.name = "address";
> + entry->address_file.kobj_attr.attr.mode = 0444;
> + entry->address_file.kobj_attr.show = cbmem_entry_address_show;
> + entry->address_file.entry = entry;
> + ret = sysfs_create_file(entry->kobj,
> + &entry->address_file.kobj_attr.attr);
> + if (ret)
> + goto free_mem_file;
> +
> + sysfs_attr_init(&entry->size_file.kobj_attr.attr);
> + entry->size_file.kobj_attr.attr.name = "size";
> + entry->size_file.kobj_attr.attr.mode = 0444;
> + entry->size_file.kobj_attr.show = cbmem_entry_size_show;
> + entry->size_file.entry = entry;
> + ret = sysfs_create_file(entry->kobj, &entry->size_file.kobj_attr.attr);
> + if (ret)
> + goto free_address_file;
> +
> + sysfs_attr_init(&entry->id_file.kobj_attr.attr);
> + entry->id_file.kobj_attr.attr.name = "id";
> + entry->id_file.kobj_attr.attr.mode = 0444;
> + entry->id_file.kobj_attr.show = cbmem_entry_id_show;
> + entry->id_file.entry = entry;
> + ret = sysfs_create_file(entry->kobj, &entry->id_file.kobj_attr.attr);
> + if (ret)
> + goto free_size_file;
Please use an attribute group so that this all happens automatically and
you do not have to hand add and remove files.
That will make this logic much simpler and cleaner.
> +struct kobject *coreboot_kobj;
> +EXPORT_SYMBOL(coreboot_kobj);
EXPORT_SYMBOL_GPL() for kobjects please.
> +
> static int coreboot_bus_match(struct device *dev, struct device_driver *drv)
> {
> struct coreboot_device *device = CB_DEV(dev);
> @@ -157,6 +162,10 @@ static int coreboot_table_probe(struct platform_device *pdev)
> }
> memunmap(ptr);
>
> + coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
> + if (!coreboot_kobj)
> + return -ENOMEM;
> +
> return ret;
> }
>
> @@ -170,6 +179,7 @@ static int coreboot_table_remove(struct platform_device *pdev)
> {
> bus_for_each_dev(&coreboot_bus_type, NULL, NULL, __cb_dev_unregister);
> bus_unregister(&coreboot_bus_type);
> + kobject_put(coreboot_kobj);
> return 0;
> }
>
> diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
> index beb778674acd..76c31e6e5376 100644
> --- a/drivers/firmware/google/coreboot_table.h
> +++ b/drivers/firmware/google/coreboot_table.h
> @@ -14,6 +14,11 @@
>
> #include <linux/device.h>
>
> +struct kobject;
> +
> +/* This is /sys/firmware/coreboot */
> +extern struct kobject *coreboot_kobj;
> +
> /* Coreboot table header structure */
> struct coreboot_table_header {
> char signature[4];
> @@ -39,6 +44,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;
As these cross the user/kernel boundry shouldn't they be __u32 and
__u64? Or is that not how coreboot works?
thanks,
greg k-h
On 2022-09-28 at 08:14 +0200, Greg KH wrote:
> Why is this a RESEND?
>
> I don't remember seeing any of the 10 previous versions, but hey, I
> can't remember much...
I sent the last 10 versions (including just v10) to the mailing lists
without including you. Realizing I should include you, I resent. Sorry
about that.
> Why is "coreboot" needed here?
I just did this for more heiarchy, and it's not necessary. This is just
/sys/firmware/cbmem in the latest patchset.
> Will this work for all coreboot implementations?
Yes, CBMEM has existed in coreboot since 2009, so it'll work with any
non-antique version.
> Raw memory addresses? Why?
Two reasons:
(1) It's useful information while debugging changes you've made to
coreboot (e.g., if you were adding a new data structure to cbmem).
(2) The "cbmem" command line tool exposes this information, and it would
be nice to entirely replace it's current /dev/mem interface with this
sysfs interface once it's widely available.
> Nothing is being "stored" here, it is being read.
Right, fixed in v11, thanks.
> Why doesn't mmap work?
Coreboot won't guarantee page alignment. I updated this text in v11 to
mention that.
> What's the security implications of exposing all of this information?
Nothing really. These are data structures from firmware such as the
nvram, boot log, flash layout, etc. It's not something that would
contain secret information.
The biggest consideration is that we allow access to memory regions.
Since the coreboot table may be a CBMEM region itself, we just make sure
to copy the entry's size from the coreboot table before it's available
in sysfs so manipulation of the coreboot table won't allow arbitrary
memory access.
> Please use an attribute group so that this all happens automatically and
> you do not have to hand add and remove files.
>
> That will make this logic much simpler and cleaner.
Thanks, v11 has it =)
> > +/* Corresponds to LB_TAG_CBMEM_ENTRY */
> > +struct lb_cbmem_entry {
> > + u32 tag;
> > + u32 size;
> > +
> > + u64 address;
> > + u32 entry_size;
> > + u32 id;
>
> As these cross the user/kernel boundry shouldn't they be __u32 and
> __u64? Or is that not how coreboot works?
This data structure isn't exposed to user space in its raw format (we
give access to it via %x-encoded output from the sysfs files).
> > Will this work for all coreboot implementations?
>
> Yes, CBMEM has existed in coreboot since 2009, so it'll work with any
> non-antique version.
FWIW the lb_cbmem_entry tags that this implementation relies on are a
bit newer (2015), but the basic sentiment remains. This is just meant
to add support for the coreboot versions that expose this data, it's
not going to hurt earlier ones that don't (it's just going to create
an empty sysfs directory for those).