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

["Jarkko Sakkinen" <jarkko@kernel.org>]

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