Re: [PATCH] efi: expose TPM event log to userspace via sysfs

Jarkko Sakkinen
2024-04-26T11:48:44Z

On Mon Apr 22, 2024 at 2:27 PM EEST, Mikko Rapeli wrote:
> Documentation/ABI/testing/sysfs-firmware-efi | 12 ++++++++++++
> drivers/firmware/efi/efi.c | 13 +++++++++++++
> 2 files changed, 25 insertions(+)
>
> diff --git a/Documentation/ABI/testing/sysfs-firmware-efi b/Documentation/ABI/testing/sysfs-firmware-efi
> index 5e4d0b27cdfe..caaff27cc73e 100644
> --- a/Documentation/ABI/testing/sysfs-firmware-efi
> +++ b/Documentation/ABI/testing/sysfs-firmware-efi
> @@ -36,3 +36,15 @@ Description: Displays the content of the Runtime Configuration Interface
> Table version 2 on Dell EMC PowerEdge systems in binary format
> Users: It is used by Dell EMC OpenManage Server Administrator tool to
> populate BIOS setup page.
> +
> +What: /sys/firmware/efi/tpm_log

*Conditional* suggestion: s/tpm_log/tpm_event_log/

I.e. if we want this better to use the most popular name.

> +Date: April 2024
> +Contact: Mikko Rapeli <mikko.rapeli@linaro.org>
> +Description: If EFI firmware supports TPM device and measurements were done
> + then a TPM event log has very likely been generated and provided
> + to the kernel. This serves as indicator for userspace to load
> + TPM drivers and to start related service early in the boot sequence,
> + e.g. initramfs, where full bus probes and device scans are not yet
> + done.
> +Users: systemd will use this interface to support TPM drivers as modules also
> + for early initramfs
> diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c
> index 4fcda50acfa4..94773e8b8806 100644
> --- a/drivers/firmware/efi/efi.c
> +++ b/drivers/firmware/efi/efi.c
> @@ -162,6 +162,13 @@ static ssize_t fw_platform_size_show(struct kobject *kobj,
> return sprintf(buf, "%d\n", efi_enabled(EFI_64BIT) ? 64 : 32);
> }
>
> +static ssize_t tpm_log_show(struct kobject *kobj,
> + struct kobj_attribute *attr, char *buf)
> +{
> + return sprintf(buf, "TPMEventLog=0x%lx", efi.tpm_log);

It is also asymmetric with the log message as we can see here. Why you
want to put that prefix and not just print the number? Why hex and not
just %lu?

> +}
> +static struct kobj_attribute efi_attr_tpm_log = __ATTR_RO_MODE(tpm_log, 0400);
> +
> extern __weak struct kobj_attribute efi_attr_fw_vendor;
> extern __weak struct kobj_attribute efi_attr_runtime;
> extern __weak struct kobj_attribute efi_attr_config_table;
> @@ -459,6 +466,12 @@ static int __init efisubsys_init(void)
> platform_device_register_simple("efi_secret", 0, NULL, 0);
> #endif
>
> + if (efi.tpm_log != EFI_INVALID_TABLE_ADDR) {
> + error = sysfs_create_file(efi_kobj, &efi_attr_tpm_log.attr);
> + if (error)
> + pr_err("sysfs create file failed with error %d.\n", error);
> + }

s/err/warn/

Why "sys create file" and not "sys_create_file"?

> +
> return 0;
>
> err_remove_group:

BR, Jarkko