Re: [PATCH v11] firmware: google: Implement cbmem in sysfs driver

Greg KH
2022-09-30T06:33:01Z

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