diff mbox series

firmware: google: Implement vboot workbuf in sysfs

Message ID 20220726235115.1184532-1-jrosenth@chromium.org (mailing list archive)
State Superseded
Headers show
Series firmware: google: Implement vboot workbuf in sysfs | expand

Commit Message

Jack Rosenthal July 26, 2022, 11:51 p.m. UTC
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 <swboyd@chromium.org>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Julius Werner <jwerner@chromium.org>
Tested-by: Jack Rosenthal <jrosenth@chromium.org>
Signed-off-by: Jack Rosenthal <jrosenth@chromium.org>
---
 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

Comments

Julius Werner July 27, 2022, 12:26 a.m. UTC | #1
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.
Stephen Boyd July 27, 2022, 1:40 a.m. UTC | #2
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.
Jack Rosenthal July 27, 2022, 7:34 p.m. UTC | #3
Thanks for the feedback.  I will re-work this to be a more generic cbmem
sysfs interface.

Jack
diff mbox series

Patch

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");