Re: [PATCH v4 3/6] bootm: Support boot measurement

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Eddie,

On Wed, Jan 25, 2023 at 11:18:07AM -0600, Eddie James wrote:
> Add a configuration option to measure the boot through the bootm
> function. Add the measurement state to the booti and bootz paths
> as well.
>
> Signed-off-by: Eddie James <eajames@linux.ibm.com>
> ---
>  boot/Kconfig    | 23 ++++++++++++++++
>  boot/bootm.c    | 70 +++++++++++++++++++++++++++++++++++++++++++++++++
>  cmd/booti.c     |  1 +
>  cmd/bootm.c     |  2 ++
>  cmd/bootz.c     |  1 +
>  include/bootm.h |  2 ++
>  include/image.h |  1 +
>  7 files changed, 100 insertions(+)
>
> diff --git a/boot/Kconfig b/boot/Kconfig
> index fdcfbae7b2..831b9e954c 100644
> --- a/boot/Kconfig
> +++ b/boot/Kconfig
> @@ -601,6 +601,29 @@ config LEGACY_IMAGE_FORMAT
>  	  loaded. If a board needs the legacy image format support in this
>  	  case, enable it here.
>
> +config MEASURED_BOOT
> +	bool "Measure boot images and configuration to TPM and event log"
> +	depends on HASH && TPM_V2
> +	help
> +	  This option enables measurement of the boot process. Measurement
> +	  involves creating cryptographic hashes of the binary images that
> +	  are booting and storing them in the TPM. In addition, a log of
> +	  these hashes is stored in memory for the OS to verify the booted
> +	  images and configuration. Enable this if the OS has configured
> +	  some memory area for the event log and you intend to use some
> +	  attestation tools on your system.
> +
> +if MEASURED_BOOT
> +	config MEASURE_DEVICETREE
> +	bool "Measure the devicetree image"
> +	default y if MEASURED_BOOT
> +	help
> +	  On some platforms, the devicetree is not static as it may contain
> +	  random MAC addresses or other such data that changes each boot.
> +	  Therefore, it should not be measured into the TPM. In that case,
> +	  disable the measurement here.
> +endif # MEASURED_BOOT
> +
>  config SUPPORT_RAW_INITRD
>  	bool "Enable raw initrd images"
>  	help
> diff --git a/boot/bootm.c b/boot/bootm.c
> index 15fce8ad95..c8423f2c60 100644
> --- a/boot/bootm.c
> +++ b/boot/bootm.c
> @@ -22,6 +22,7 @@
>  #include <asm/global_data.h>
>  #include <asm/io.h>
>  #include <linux/sizes.h>
> +#include <tpm-v2.h>
>  #if defined(CONFIG_CMD_USB)
>  #include <usb.h>
>  #endif
> @@ -659,6 +660,72 @@ int bootm_process_cmdline_env(int flags)
>  	return 0;
>  }
>
> +int bootm_measure(struct bootm_headers *images)
> +{
> +	int ret = 0;
> +
> +	/* Skip measurement if EFI is going to do it */
> +	if (images->os.os == IH_OS_EFI &&
> +	    IS_ENABLED(CONFIG_EFI_TCG2_PROTOCOL) &&
> +	    IS_ENABLED(CONFIG_BOOTM_EFI))
> +		return ret;
> +
> +	if (IS_ENABLED(CONFIG_MEASURED_BOOT)) {
> +		struct tcg2_event_log elog;
> +		struct udevice *dev;
> +		void *initrd_buf;
> +		void *image_buf;
> +		const char *s;
> +		u32 rd_len;
> +
> +		ret = tcg2_measurement_init(&dev, &elog);
> +		if (ret)
> +			return ret;
> +
> +		image_buf = map_sysmem(images->os.image_start,
> +				       images->os.image_len);
> +		ret = tcg2_measure_data(dev, &elog, 8, images->os.image_len,
> +					image_buf, EV_COMPACT_HASH,
> +					strlen("linux") + 1, (u8 *)"linux");
> +		if (ret)
> +			goto unmap_image;
> +
> +		rd_len = images->rd_end - images->rd_start;
> +		initrd_buf = map_sysmem(images->rd_start, rd_len);
> +		ret = tcg2_measure_data(dev, &elog, 8, rd_len, initrd_buf,
> +					EV_COMPACT_HASH, strlen("initrd") + 1,
> +					(u8 *)"initrd");

The kernel, if loaded with EFI,  measures the initrd data into PCR9.
Although it's not important can we please use the same PCR here?

> +		if (ret)
> +			goto unmap_initrd;
> +
> +		if (IS_ENABLED(CONFIG_MEASURE_DEVICETREE)) {
> +			ret = tcg2_measure_data(dev, &elog, 9, images->ft_len,
> +						(u8 *)images->ft_addr,
> +						EV_TABLE_OF_DEVICES,
> +						strlen("dts") + 1,
> +						(u8 *)"dts");

The PC client spec describes how ACPI data should be measured.  Although it
has no wording on the DTB, I think it should be treated in a similar
fashion.  This [0] was trying to do the same thing for EFI.  Any reason we
can't use PCR0 here?

> +			if (ret)
> +				goto unmap_initrd;
> +		}
> +
> +		s = env_get("bootargs");
> +		if (!s)
> +			s = "";
> +		ret = tcg2_measure_data(dev, &elog, 1, strlen(s) + 1, (u8 *)s,
> +					EV_PLATFORM_CONFIG_FLAGS,
> +					strlen(s) + 1, (u8 *)s);
> +
> +unmap_initrd:
> +		unmap_sysmem(initrd_buf);

 [0] https://lore.kernel.org/u-boot/20221207151110.529106-1-etienne.carriere@linaro.org/

 Regards
 /Ilias