2022-07-26 23:55:45

by Jack Rosenthal

[permalink] [raw]
Subject: [PATCH] firmware: google: Implement vboot workbuf in sysfs

The vboot workbuf is a large data structure exposed by coreboot for
payloads and the operating system to use. Existing tools like
crossystem re-create this structure in userspace by reading the
deprecated vboot v1 VBSD structure from FDT/ACPI, and translating it
back to a vboot v2 workbuf. Since not all data required for vboot v2
is available in the VBSD structure, crossystem also has to shell out
to tools like flashrom to fill in the missing bits.

Since coreboot commit a8bda43, this workbuf is also available in
CBMEM, and since workbuf struct version 2 (vboot commit ecdca93), this
data also contains the vboot context, making it useful. Exposing it
via sysfs enables tools like crossystem significantly easier access to
read and manipulate vboot variables.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Julius Werner <[email protected]>
Tested-by: Jack Rosenthal <[email protected]>
Signed-off-by: Jack Rosenthal <[email protected]>
---
drivers/firmware/google/Kconfig | 8 ++
drivers/firmware/google/Makefile | 1 +
drivers/firmware/google/vboot.c | 173 +++++++++++++++++++++++++++++++
3 files changed, 182 insertions(+)
create mode 100644 drivers/firmware/google/vboot.c

diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..f0e691cb9496 100644
--- a/drivers/firmware/google/Kconfig
+++ b/drivers/firmware/google/Kconfig
@@ -67,6 +67,14 @@ config GOOGLE_MEMCONSOLE_COREBOOT
the coreboot table. If found, this log is exported to userland
in the file /sys/firmware/log.

+config GOOGLE_VBOOT_SYSFS
+ tristate "Verified Boot in sysfs"
+ depends on GOOGLE_COREBOOT_TABLE
+ help
+ This option enables the kernel to search for the Verified Boot
+ workbuf coreboot table. If found, the workbuf is exported
+ to userland in /sys/firmware/vboot.
+
config GOOGLE_VPD
tristate "Vital Product Data"
depends on GOOGLE_COREBOOT_TABLE
diff --git a/drivers/firmware/google/Makefile b/drivers/firmware/google/Makefile
index d17caded5d88..f8db06764725 100644
--- a/drivers/firmware/google/Makefile
+++ b/drivers/firmware/google/Makefile
@@ -6,6 +6,7 @@ obj-$(CONFIG_GOOGLE_FRAMEBUFFER_COREBOOT) += framebuffer-coreboot.o
obj-$(CONFIG_GOOGLE_MEMCONSOLE) += memconsole.o
obj-$(CONFIG_GOOGLE_MEMCONSOLE_COREBOOT) += memconsole-coreboot.o
obj-$(CONFIG_GOOGLE_MEMCONSOLE_X86_LEGACY) += memconsole-x86-legacy.o
+obj-$(CONFIG_GOOGLE_VBOOT_SYSFS) += vboot.o

vpd-sysfs-y := vpd.o vpd_decode.o
obj-$(CONFIG_GOOGLE_VPD) += vpd-sysfs.o
diff --git a/drivers/firmware/google/vboot.c b/drivers/firmware/google/vboot.c
new file mode 100644
index 000000000000..25feb1a33dcd
--- /dev/null
+++ b/drivers/firmware/google/vboot.c
@@ -0,0 +1,173 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * vboot.c
+ *
+ * Driver for exporting the vboot workbuf to sysfs.
+ *
+ * Copyright 2022 Google Inc.
+ */
+
+#include <linux/capability.h>
+#include <linux/ctype.h>
+#include <linux/dev_printk.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_VBOOT_WORKBUF 0x34
+
+/* "V2SD" = vb2_shared_data.magic */
+#define VB2_SHARED_DATA_MAGIC 0x44533256
+#define VB2_CONTEXT_MAX_SIZE_V2 192
+#define VB2_CONTEXT_MAX_SIZE_V3 384
+
+struct workbuf {
+ u32 magic;
+ u16 version_major;
+ u16 version_minor;
+ union {
+ /*
+ * V1 not supported as workbuf and vboot context
+ * located separately.
+ */
+ struct {
+ u8 context_padding[VB2_CONTEXT_MAX_SIZE_V2];
+ u32 workbuf_size;
+ } v2;
+ struct {
+ u8 context_padding[VB2_CONTEXT_MAX_SIZE_V3];
+ u32 workbuf_size;
+ } v3;
+ };
+};
+
+static struct kobject *vboot_kobj;
+
+static struct {
+ u8 *buf;
+ u32 size;
+ struct bin_attribute bin_attr;
+} workbuf_info;
+
+static ssize_t workbuf_read(struct file *filp, struct kobject *kobp,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t pos, size_t count)
+{
+ return memory_read_from_buffer(buf, count, &pos, workbuf_info.buf,
+ workbuf_info.size);
+}
+
+static ssize_t workbuf_write(struct file *filp, struct kobject *kobp,
+ struct bin_attribute *bin_attr, char *buf,
+ loff_t pos, size_t count)
+{
+ if (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ if (pos < 0 || pos >= workbuf_info.size)
+ return -EINVAL;
+ if (count > workbuf_info.size - pos)
+ count = workbuf_info.size - pos;
+
+ memcpy(workbuf_info.buf + pos, buf, count);
+ return count;
+}
+
+/*
+ * This function should only be called during initialization -- prior
+ * to workbuf being added to sysfs. The workbuf can be manipulated
+ * via sysfs, and thus, we should not trust the size in the workbuf
+ * after we've given the user write access to the buffer.
+ */
+static int get_workbuf_size(struct coreboot_device *dev, u32 *size_out)
+{
+ int ret = 0;
+ struct workbuf *workbuf;
+
+ workbuf = memremap(dev->cbmem_ref.cbmem_addr, sizeof(struct workbuf),
+ MEMREMAP_WB);
+
+ if (workbuf->magic != VB2_SHARED_DATA_MAGIC) {
+ dev_err(dev, "%s: workbuf has invalid magic number\n",
+ __func__);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+ switch (workbuf->version_major) {
+ case 2:
+ *size_out = workbuf->v2.workbuf_size;
+ break;
+ case 3:
+ *size_out = workbuf->v3.workbuf_size;
+ break;
+ default:
+ dev_err(dev, "%s: workbuf v%hu.%hu not supported", __func__,
+ workbuf->version_major, workbuf->version_minor);
+ ret = -EINVAL;
+ goto exit;
+ }
+
+exit:
+ memunmap(workbuf);
+ return ret;
+}
+
+static int vboot_probe(struct coreboot_device *dev)
+{
+ int ret;
+
+ vboot_kobj = kobject_create_and_add("vboot", firmware_kobj);
+ if (!vboot_kobj)
+ return -ENOMEM;
+
+ ret = get_workbuf_size(dev, &workbuf_info.size);
+ if (ret)
+ goto exit;
+
+ workbuf_info.buf = memremap(dev->cbmem_ref.cbmem_addr,
+ workbuf_info.size, MEMREMAP_WB);
+
+ sysfs_bin_attr_init(&workbuf_info.bin_attr);
+ workbuf_info.bin_attr.attr.name = "workbuf";
+ workbuf_info.bin_attr.attr.mode = 0666;
+ workbuf_info.bin_attr.size = workbuf_info.size;
+ workbuf_info.bin_attr.read = workbuf_read;
+ workbuf_info.bin_attr.write = workbuf_write;
+
+ ret = sysfs_create_bin_file(vboot_kobj, &workbuf_info.bin_attr);
+
+exit:
+ if (ret) {
+ kobject_put(vboot_kobj);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void vboot_remove(struct coreboot_device *dev)
+{
+ sysfs_remove_bin_file(vboot_kobj, &workbuf_info.bin_attr);
+ kobject_put(vboot_kobj);
+ memunmap(&workbuf_info.buf);
+}
+
+static struct coreboot_driver vboot_driver = {
+ .probe = vboot_probe,
+ .remove = vboot_remove,
+ .drv = {
+ .name = "vboot",
+ },
+ .tag = LB_TAG_VBOOT_WORKBUF,
+};
+module_coreboot_driver(vboot_driver);
+
+MODULE_AUTHOR("Google, Inc.");
+MODULE_LICENSE("GPL");
--
2.37.1.359.gd136c6c3e2-goog


2022-07-27 01:05:31

by Julius Werner

[permalink] [raw]
Subject: Re: [PATCH] firmware: google: Implement vboot workbuf in sysfs

Sorry, this wasn't quite what I meant. I think we should definitely
not hardcode any details about the vboot data structure layout here.
It's already enough of a pain to keep crossystem in sync with
different data structure versions, we absolutely don't want to have to
keep updating that in the kernel as well.

I assume that the reason you went that route seems to have been mostly
that the lb_cbmem_ref coreboot table entry has no size field, so you
had to infer the size from within the data structure. Thankfully, we
don't need to use lb_cbmem_ref for this, that's somewhat of a legacy
entry type that we're trying to phase out where it's no longer needed
for backwards-compatibility anyway (and in fact I think we should be
okay to remove the vboot entry there nowadays). We also have
lb_cbmem_entry (see
https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/imd_cbmem.c#222)
that exports info about every area in CBMEM, with address, size and
CBMEM ID. The vboot workbuffer is CBMEM ID 0x78007343.

I think we should just add a general driver for lb_cbmem_entry tags
here, that uses the "id" as (part of) the device name and contains
node to read/write the raw bytes of the buffer. Then crossystem can
easily find the right one for vboot, and the infrastructure may also
come in handy for other uses (or debugging) in the future.

2022-07-27 01:58:13

by Stephen Boyd

[permalink] [raw]
Subject: Re: [PATCH] firmware: google: Implement vboot workbuf in sysfs

Quoting Julius Werner (2022-07-26 17:26:41)
> Sorry, this wasn't quite what I meant. I think we should definitely
> not hardcode any details about the vboot data structure layout here.
> It's already enough of a pain to keep crossystem in sync with
> different data structure versions, we absolutely don't want to have to
> keep updating that in the kernel as well.
>
> I assume that the reason you went that route seems to have been mostly
> that the lb_cbmem_ref coreboot table entry has no size field, so you
> had to infer the size from within the data structure. Thankfully, we
> don't need to use lb_cbmem_ref for this, that's somewhat of a legacy
> entry type that we're trying to phase out where it's no longer needed
> for backwards-compatibility anyway (and in fact I think we should be
> okay to remove the vboot entry there nowadays). We also have
> lb_cbmem_entry (see
> https://review.coreboot.org/plugins/gitiles/coreboot/+/refs/heads/master/src/lib/imd_cbmem.c#222)
> that exports info about every area in CBMEM, with address, size and
> CBMEM ID. The vboot workbuffer is CBMEM ID 0x78007343.
>
> I think we should just add a general driver for lb_cbmem_entry tags
> here, that uses the "id" as (part of) the device name and contains
> node to read/write the raw bytes of the buffer. Then crossystem can
> easily find the right one for vboot, and the infrastructure may also
> come in handy for other uses (or debugging) in the future.

If there's nothing to really "drive" then a driver is overkill. Would
exposing some raw bytes in /sys/firmware/coreboot be sufficient here,
possibly under some sort of /sys/firmware/coreboot/cbmem_entries/ path?

I honestly have no idea what vboot workbuffer is though so maybe I'm
just talking nonsense. If this ends up in sysfs please document the
files in Documentation/ABI/ and include it in the patch so we know what
is being exposed in the kernel ABI.

2022-07-27 19:37:07

by Jack Rosenthal

[permalink] [raw]
Subject: Re: [PATCH] firmware: google: Implement vboot workbuf in sysfs

Thanks for the feedback. I will re-work this to be a more generic cbmem
sysfs interface.

Jack

2022-07-29 19:34:27

by Jack Rosenthal

[permalink] [raw]
Subject: [PATCH v3] firmware: google: Implement cbmem in sysfs driver

cbmem entries can be read from coreboot table
0x31 (LB_TAG_CBMEM_ENTRY). This commit exports access to cbmem
entries in sysfs under /sys/firmware/coreboot/cbmem-*.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Julius Werner <[email protected]>
Tested-by: Jack Rosenthal <[email protected]>
Signed-off-by: Jack Rosenthal <[email protected]>
---
.../ABI/testing/sysfs-firmware-coreboot | 16 +
drivers/firmware/google/Kconfig | 8 +
drivers/firmware/google/Makefile | 3 +
drivers/firmware/google/cbmem.c | 408 ++++++++++++++++++
drivers/firmware/google/coreboot_table.h | 11 +
5 files changed, 446 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..c66ef96647e9
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
@@ -0,0 +1,16 @@
+What: /sys/firmware/coreboot/
+Date: July 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ Coreboot-based BIOS firmware provides a variety of information
+ in CBMEM. Each CBMEM entry can be found via Coreboot tables.
+ For each CBMEM entry, the following are exposed:
+
+ address: A hexidecimal value of the memory address the data for
+ the entry begins at.
+ size: The size of the data stored.
+ id: The id corresponding to the entry. A list of ids known to
+ coreboot can be found in the coreboot tree at
+ src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h
+ alias: A common name for the id of the entry.
+ mem: A file exposing the raw memory for the entry.
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..bf8316d1cb31 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.
+
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..71c5de17e243
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,408 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google Inc.
+ */
+
+#include <linux/capability.h>
+#include <linux/ctype.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
+
+/*
+ * This list should be kept in sync with
+ * src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h from coreboot.
+ */
+static const struct {
+ const char *alias;
+ u32 id;
+} cbmem_aliases[] = {
+ { "acpi", 0x41435049 },
+ { "acpi-bert", 0x42455254 },
+ { "acpi-cnvs", 0x434e5653 },
+ { "acpi-gnvs", 0x474e5653 },
+ { "acpi-hest", 0x48455354 },
+ { "acpi-ucsi", 0x55435349 },
+ { "after-car", 0xc4787a93 },
+ { "agesa-runtime", 0x41474553 },
+ { "agesa-mtrr", 0xf08b4b9d },
+ { "amdmct-meminfo", 0x494d454e },
+ { "car-globals", 0xcac4e6a3 },
+ { "cbtable", 0x43425442 },
+ { "cbtable-fwd", 0x43425443 },
+ { "cb-early-dram", 0x4544524d },
+ { "console", 0x434f4e53 },
+ { "cpu-crashlog", 0x4350555f },
+ { "coverage", 0x47434f56 },
+ { "cse-update", 0x43534555 },
+ { "ehci-debug", 0xe4c1deb9 },
+ { "elog", 0x454c4f47 },
+ { "freespace", 0x46524545 },
+ { "fsp-reserved-memory", 0x46535052 },
+ { "fsp-runtime", 0x52505346 },
+ { "gdt", 0x4c474454 },
+ { "hob-pointer", 0x484f4221 },
+ { "igd-opregion", 0x4f444749 },
+ { "imd-root", 0xff4017ff },
+ { "imd-small", 0x53a11439 },
+ { "mdata-hash", 0x6873484d },
+ { "meminfo", 0x494d454d },
+ { "mma-data", 0x4d4d4144 },
+ { "mmc-status", 0x4d4d4353 },
+ { "mptable", 0x534d5054 },
+ { "mrcdata", 0x4d524344 },
+ { "pmc-crashlog", 0x504d435f },
+ { "var-mrcdata", 0x4d524345 },
+ { "mtc", 0xcb31d31c },
+ { "none", 0x00000000 },
+ { "pirq", 0x49525154 },
+ { "power-state", 0x50535454 },
+ { "ram-oops", 0x05430095 },
+ { "ramstage", 0x9a357a9e },
+ { "ramstage-cache", 0x9a3ca54e },
+ { "refcode", 0x04efc0de },
+ { "refcode-cache", 0x4efc0de5 },
+ { "resume", 0x5245534d },
+ { "resume-scratch", 0x52455343 },
+ { "romstage-info", 0x47545352 },
+ { "romstage-ram-stack", 0x90357ac4 },
+ { "root", 0xff4007ff },
+ { "smbios", 0x534d4254 },
+ { "smm-save-space", 0x07e9acee },
+ { "stagex-meta", 0x57a9e000 },
+ { "stagex-cache", 0x57a9e100 },
+ { "stagex-mem", 0x57a9e200 },
+ { "storage-data", 0x53746f72 },
+ { "tcpa-log", 0x54435041 },
+ { "tcpa-tcg-log", 0x54445041 },
+ { "timestamp", 0x54494d45 },
+ { "tpm2-tcg-log", 0x54504d32 },
+ { "tpm-ppi", 0x54505049 },
+ { "vboot-handoff", 0x780074f0 },
+ { "vboot-sel-reg", 0x780074f1 },
+ { "vboot-workbuf", 0x78007343 },
+ { "vpd", 0x56504420 },
+ { "wifi-calibration", 0x57494649 },
+ { "ec-hostevent", 0x63ccbbc3 },
+ { "ext-vbt", 0x69866684 },
+ { "vga-rom0", 0x524f4d30 },
+ { "vga-rom1", 0x524f4d31 },
+ { "vga-rom2", 0x524f4d32 },
+ { "vga-rom3", 0x524f4d33 },
+ { "fmap", 0x464d4150 },
+ { "cbfs-ro-mcache", 0x524d5346 },
+ { "cbfs-rw-mcache", 0x574d5346 },
+ { "fsp-logo", 0x4c4f474f },
+ { "smm-combuffer", 0x53534d32 },
+ { "type-c-info", 0x54595045 },
+ { "mem-chip-info", 0x5048434d },
+};
+
+static const char *get_alias(u32 cbmem_id)
+{
+ for (int i = 0; i < ARRAY_SIZE(cbmem_aliases); i++) {
+ if (cbmem_id == cbmem_aliases[i].id)
+ return cbmem_aliases[i].alias;
+ }
+
+ return NULL;
+}
+
+static struct kobject *coreboot_kobj;
+
+struct cbmem_entry;
+struct cbmem_entry_attr {
+ struct kobj_attribute kobj_attr;
+ struct cbmem_entry *entry;
+};
+
+struct cbmem_entry {
+ char *kobj_name;
+ 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;
+ const char *alias;
+ struct cbmem_entry_attr alias_name_file;
+ char *alias_link_name;
+};
+
+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 (!capable(CAP_SYS_ADMIN))
+ return -EPERM;
+ 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_alias_name_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, "%s\n", entry_attr->entry->alias);
+}
+
+static int cbmem_entry_setup_alias(struct cbmem_entry *entry)
+{
+ int ret;
+
+ entry->alias = get_alias(entry->dev->cbmem_entry.id);
+ if (!entry->alias)
+ return 0;
+
+ sysfs_attr_init(&entry->alias_name_file.kobj_attr.attr);
+ entry->alias_name_file.kobj_attr.attr.name = "alias";
+ entry->alias_name_file.kobj_attr.attr.mode = 0444;
+ entry->alias_name_file.kobj_attr.show = cbmem_entry_alias_name_show;
+ entry->alias_name_file.entry = entry;
+ ret = sysfs_create_file(entry->kobj,
+ &entry->alias_name_file.kobj_attr.attr);
+ if (ret)
+ return ret;
+
+ entry->alias_link_name =
+ kasprintf(GFP_KERNEL, "cbmem-%s", entry->alias);
+ if (!entry->alias_link_name) {
+ ret = -ENOMEM;
+ goto free_alias_name_file;
+ }
+
+ ret = sysfs_create_link(coreboot_kobj, entry->kobj,
+ entry->alias_link_name);
+ if (ret)
+ goto free_alias_link_name;
+
+ return 0;
+
+free_alias_link_name:
+ kfree(entry->alias_link_name);
+free_alias_name_file:
+ sysfs_remove_file(entry->kobj, &entry->alias_name_file.kobj_attr.attr);
+
+ return ret;
+}
+
+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%x\n",
+ entry_attr->entry->dev->cbmem_entry.id);
+}
+
+static int cbmem_entry_setup(struct cbmem_entry *entry)
+{
+ int ret;
+
+ entry->mem_file_buf = memremap(entry->dev->cbmem_entry.address,
+ entry->dev->cbmem_entry.entry_size,
+ MEMREMAP_WB);
+ if (!entry->mem_file_buf)
+ return -ENOMEM;
+
+ entry->kobj_name =
+ kasprintf(GFP_KERNEL, "cbmem-%08x", entry->dev->cbmem_entry.id);
+ if (!entry->kobj_name) {
+ ret = -ENOMEM;
+ goto memunmap;
+ }
+
+ entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
+ if (!entry->kobj) {
+ ret = -ENOMEM;
+ goto free_name;
+ }
+
+ sysfs_bin_attr_init(&entry->mem_file);
+ entry->mem_file.attr.name = "mem";
+ entry->mem_file.attr.mode = 0664;
+ 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;
+
+ ret = cbmem_entry_setup_alias(entry);
+ if (ret)
+ goto free_id_file;
+
+ return 0;
+
+free_id_file:
+ sysfs_remove_file(entry->kobj, &entry->id_file.kobj_attr.attr);
+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);
+free_name:
+ kfree(entry->kobj_name);
+memunmap:
+ memunmap(entry->mem_file_buf);
+ return ret;
+}
+
+static int cbmem_entry_probe(struct coreboot_device *dev)
+{
+ int ret;
+ struct cbmem_entry *entry;
+
+ entry = kzalloc(sizeof(*entry), GFP_KERNEL);
+ if (!entry)
+ return -ENOMEM;
+
+ dev_set_drvdata(&dev->dev, entry);
+ entry->dev = dev;
+ ret = cbmem_entry_setup(entry);
+ if (ret) {
+ kfree(entry);
+ return ret;
+ }
+
+ return 0;
+}
+
+static void cbmem_entry_remove(struct coreboot_device *dev)
+{
+ struct cbmem_entry *entry = dev_get_drvdata(&dev->dev);
+
+ sysfs_remove_file(entry->kobj, &entry->alias_name_file.kobj_attr.attr);
+ sysfs_remove_bin_file(entry->kobj, &entry->mem_file);
+ if (entry->alias)
+ sysfs_remove_link(entry->kobj, entry->alias_link_name);
+ kfree(entry->alias_link_name);
+ memunmap(entry->mem_file_buf);
+ kobject_put(entry->kobj);
+ kfree(entry->kobj_name);
+ kfree(entry);
+}
+
+static struct coreboot_driver cbmem_entry_driver = {
+ .probe = cbmem_entry_probe,
+ .remove = cbmem_entry_remove,
+ .drv = {
+ .name = "cbmem",
+ },
+ .tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+ int ret;
+
+ coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
+ if (!coreboot_kobj)
+ return -ENOMEM;
+
+ ret = coreboot_driver_register(&cbmem_entry_driver);
+ if (ret) {
+ kobject_put(coreboot_kobj);
+ return ret;
+ }
+
+ return 0;
+}
+module_init(cbmem_init);
+
+static void __exit cbmem_exit(void)
+{
+ kobject_put(coreboot_kobj);
+ coreboot_driver_unregister(&cbmem_entry_driver);
+}
+module_exit(cbmem_exit);
+
+MODULE_AUTHOR("Google, Inc.");
+MODULE_LICENSE("GPL");
diff --git a/drivers/firmware/google/coreboot_table.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..6c03a8852d1b 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,6 +75,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.1.455.g008518b4e5-goog

2022-07-30 00:33:36

by Julius Werner

[permalink] [raw]
Subject: Re: [PATCH v3] firmware: google: Implement cbmem in sysfs driver

Thanks, I think this looks great in general!

> + * Copyright 2022 Google Inc.

LLC

> +/*
> + * This list should be kept in sync with
> + * src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h from coreboot.
> + */
> +static const struct {
> + const char *alias;
> + u32 id;
> +} cbmem_aliases[] = {
> + { "acpi", 0x41435049 },
> + { "acpi-bert", 0x42455254 },
> + { "acpi-cnvs", 0x434e5653 },

I don't think we really want to keep this in the kernel? New entries
get added here all the time, I think it would really be way more
effort than benefit to keep this in sync. It also doesn't really mesh
with the idea that this is a thin interface in the kernel that doesn't
do any heavyweight interpretation. I'd say keep out the alias stuff
and just have the numeric nodes only.

> +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 (!capable(CAP_SYS_ADMIN))
> + return -EPERM;

Not sure what the current standard kernel policy on these things is,
but we already have file permission bits to control this so generally
I'd leave it at that and let userspace decide how to manage access.
(Forcing everything that could possibly be security-sensitive to be
"root only" just means more things are forced to run as root, which is
also not a good thing.)

> +static int cbmem_entry_setup(struct cbmem_entry *entry)
> +{
> + int ret;
> +
> + entry->mem_file_buf = memremap(entry->dev->cbmem_entry.address,
> + entry->dev->cbmem_entry.entry_size,
> + MEMREMAP_WB);
> + if (!entry->mem_file_buf)
> + return -ENOMEM;

Can this use devm_memremap() for automatic cleanup?

> +MODULE_AUTHOR("Google, Inc.");

LLC

2022-07-30 03:22:38

by Jack Rosenthal

[permalink] [raw]
Subject: [PATCH v4] firmware: google: Implement cbmem in sysfs driver

cbmem entries can be read from coreboot table
0x31 (LB_TAG_CBMEM_ENTRY). This commit exports access to cbmem
entries in sysfs under /sys/firmware/coreboot/cbmem-*.

Link: https://issuetracker.google.com/239604743
Cc: Stephen Boyd <[email protected]>
Cc: Guenter Roeck <[email protected]>
Cc: Julius Werner <[email protected]>
Tested-by: Jack Rosenthal <[email protected]>
Signed-off-by: Jack Rosenthal <[email protected]>
---
.../ABI/testing/sysfs-firmware-coreboot | 15 ++
drivers/firmware/google/Kconfig | 8 +
drivers/firmware/google/Makefile | 3 +
drivers/firmware/google/cbmem.c | 232 ++++++++++++++++++
drivers/firmware/google/coreboot_table.h | 11 +
5 files changed, 269 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..b91477ee6198
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-firmware-coreboot
@@ -0,0 +1,15 @@
+What: /sys/firmware/coreboot/
+Date: July 2022
+Contact: Jack Rosenthal <[email protected]>
+Description:
+ Coreboot-based BIOS firmware provides a variety of information
+ in CBMEM. Each CBMEM entry can be found via Coreboot tables.
+ For each CBMEM entry, the following are exposed:
+
+ address: A hexidecimal value of the memory address the data for
+ the entry begins at.
+ size: The size of the data stored.
+ id: The id corresponding to the entry. A list of ids known to
+ coreboot can be found in the coreboot tree at
+ src/commonlib/bsd/include/commonlib/bsd/cbmem_id.h
+ mem: A file exposing the raw memory for the entry.
diff --git a/drivers/firmware/google/Kconfig b/drivers/firmware/google/Kconfig
index 983e07dc022e..bf8316d1cb31 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.
+
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..9646a8047742
--- /dev/null
+++ b/drivers/firmware/google/cbmem.c
@@ -0,0 +1,232 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * cbmem.c
+ *
+ * Driver for exporting cbmem entries in sysfs.
+ *
+ * Copyright 2022 Google LLC
+ */
+
+#include <linux/ctype.h>
+#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 *coreboot_kobj;
+
+struct cbmem_entry;
+struct cbmem_entry_attr {
+ struct kobj_attribute kobj_attr;
+ struct cbmem_entry *entry;
+};
+
+struct cbmem_entry {
+ char *kobj_name;
+ 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%x\n",
+ entry_attr->entry->dev->cbmem_entry.id);
+}
+
+static int cbmem_entry_setup(struct cbmem_entry *entry)
+{
+ int ret;
+
+ 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;
+
+ entry->kobj_name = devm_kasprintf(&entry->dev->dev, GFP_KERNEL,
+ "cbmem-%08x",
+ entry->dev->cbmem_entry.id);
+ if (!entry->kobj_name)
+ return -ENOMEM;
+
+ entry->kobj = kobject_create_and_add(entry->kobj_name, coreboot_kobj);
+ if (!entry->kobj)
+ return -ENOMEM;
+
+ sysfs_bin_attr_init(&entry->mem_file);
+ entry->mem_file.attr.name = "mem";
+ entry->mem_file.attr.mode = 0664;
+ 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 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->dev = dev;
+ return cbmem_entry_setup(entry);
+}
+
+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",
+ },
+ .tag = LB_TAG_CBMEM_ENTRY,
+};
+
+static int __init cbmem_init(void)
+{
+ int ret;
+
+ coreboot_kobj = kobject_create_and_add("coreboot", firmware_kobj);
+ if (!coreboot_kobj)
+ return -ENOMEM;
+
+ ret = coreboot_driver_register(&cbmem_entry_driver);
+ if (ret) {
+ kobject_put(coreboot_kobj);
+ return ret;
+ }
+
+ return 0;
+}
+module_init(cbmem_init);
+
+static void __exit cbmem_exit(void)
+{
+ kobject_put(coreboot_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.h b/drivers/firmware/google/coreboot_table.h
index beb778674acd..6c03a8852d1b 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,6 +75,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.1.455.g008518b4e5-goog