Re: [PATCH v3 1/5] efi_loader: add secure boot variable measurement

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

Kojima-san,

On Fri, Aug 06, 2021 at 04:02:11PM +0900, Masahisa Kojima wrote:
> TCG PC Client PFP spec requires to measure the secure
> boot policy before validating the UEFI image.
> This commit adds the secure boot variable measurement
> of "SecureBoot", "PK", "KEK", "db", "dbx", "dbt", and "dbr".
> 
> Note that this implementation assumes that secure boot
> variables are pre-configured and not be set/updated in runtime.
> 
> Signed-off-by: Masahisa Kojima <masahisa.kojima@linaro.org>
> ---
> Changes in v3:
> - add "dbt" and "dbr" measurement
> - accept empty variable measurement for "SecureBoot", "PK",
>   "KEK", "db" and "dbx" as TCG2 spec requires
> - fix comment format
> 
> Changes in v2:
> - missing null check for getting variable data
> - some minor fix for readability
> 
>  include/efi_tcg2.h        |  20 +++++
>  lib/efi_loader/efi_tcg2.c | 165 ++++++++++++++++++++++++++++++++++++++
>  2 files changed, 185 insertions(+)
> 
> diff --git a/include/efi_tcg2.h b/include/efi_tcg2.h
> index bcfb98168a..497ba3ce94 100644
> --- a/include/efi_tcg2.h
> +++ b/include/efi_tcg2.h
> @@ -142,6 +142,26 @@ struct efi_tcg2_final_events_table {
>  	struct tcg_pcr_event2 event[];
>  };
>  
> +/**
> + * struct tdUEFI_VARIABLE_DATA - event log structure of UEFI variable
> + * @variable_name:		The vendorGUID parameter in the
> + *				GetVariable() API.
> + * @unicode_name_length:	The length in CHAR16 of the Unicode name of
> + *				the variable.
> + * @variable_data_length:	The size of the variable data.
> + * @unicode_name:		The CHAR16 unicode name of the variable
> + *				without NULL-terminator.
> + * @variable_data:		The data parameter of the efi variable
> + *				in the GetVariable() API.
> + */
> +struct efi_tcg2_uefi_variable_data {
> +	efi_guid_t variable_name;
> +	u64 unicode_name_length;
> +	u64 variable_data_length;
> +	u16 unicode_name[1];
> +	u8 variable_data[1];
> +};
> +
>  struct efi_tcg2_protocol {
>  	efi_status_t (EFIAPI * get_capability)(struct efi_tcg2_protocol *this,
>  					       struct efi_tcg2_boot_service_capability *capability);
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 1319a8b378..a2e9587cd0 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -78,6 +78,19 @@ static const struct digest_info hash_algo_list[] = {
>  	},
>  };
>  
> +struct variable_info {
> +	u16		*name;
> +	const efi_guid_t	*guid;
> +};
> +
> +static struct variable_info secure_variables[] = {
> +	{L"SecureBoot", &efi_global_variable_guid},
> +	{L"PK", &efi_global_variable_guid},
> +	{L"KEK", &efi_global_variable_guid},
> +	{L"db", &efi_guid_image_security_database},
> +	{L"dbx", &efi_guid_image_security_database},
> +};
> +
>  #define MAX_HASH_COUNT ARRAY_SIZE(hash_algo_list)
>  
>  /**
> @@ -1264,6 +1277,39 @@ free_pool:
>  	return ret;
>  }
>  
> +/**
> + * tcg2_measure_event() - common function to add event log and extend PCR
> + *
> + * @dev:		TPM device
> + * @pcr_index:		PCR index
> + * @event_type:		type of event added
> + * @size:		event size
> + * @event:		event data
> + *
> + * Return:	status code
> + */
> +static efi_status_t EFIAPI
> +tcg2_measure_event(struct udevice *dev, u32 pcr_index, u32 event_type,
> +		   u32 size, u8 event[])
> +{
> +	struct tpml_digest_values digest_list;
> +	efi_status_t ret;
> +
> +	ret = tcg2_create_digest(event, size, &digest_list);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> +	if (ret != EFI_SUCCESS)
> +		goto out;
> +
> +	ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
> +				    size, event);
> +
> +out:
> +	return ret;
> +}
> +
>  /**
>   * efi_append_scrtm_version - Append an S-CRTM EV_S_CRTM_VERSION event on the
>   *			      eventlog and extend the PCRs
> @@ -1294,6 +1340,118 @@ out:
>  	return ret;
>  }
>  
> +/**
> + * tcg2_measure_variable() - add variable event log and extend PCR
> + *
> + * @dev:		TPM device
> + * @pcr_index:		PCR index
> + * @event_type:		type of event added
> + * @var_name:		variable name
> + * @guid:		guid
> + * @data_size:		variable data size
> + * @data:		variable data
> + *
> + * Return:	status code
> + */
> +static efi_status_t tcg2_measure_variable(struct udevice *dev, u32 pcr_index,
> +					  u32 event_type, u16 *var_name,
> +					  const efi_guid_t *guid,
> +					  efi_uintn_t data_size, u8 *data)
> +{
> +	u32 event_size;
> +	efi_status_t ret;
> +	struct efi_tcg2_uefi_variable_data *event;
> +
> +	event_size = sizeof(event->variable_name) +
> +		     sizeof(event->unicode_name_length) +
> +		     sizeof(event->variable_data_length) +
> +		     (u16_strlen(var_name) * sizeof(u16)) + data_size;
> +	event = malloc(event_size);
> +	if (!event)
> +		return EFI_OUT_OF_RESOURCES;
> +
> +	guidcpy(&event->variable_name, guid);
> +	event->unicode_name_length = u16_strlen(var_name);
> +	event->variable_data_length = data_size;
> +	memcpy(event->unicode_name, var_name,
> +	       (event->unicode_name_length * sizeof(u16)));
> +	if (data) {
> +		memcpy((u16 *)event->unicode_name + event->unicode_name_length,
> +		       data, data_size);
> +	}
> +	ret = tcg2_measure_event(dev, pcr_index, event_type, event_size,
> +				 (u8 *)event);
> +	free(event);
> +	return ret;
> +}
> +
> +/**
> + * tcg2_measure_secure_boot_variable() - measure secure boot variables
> + *
> + * @dev:	TPM device
> + *
> + * Return:	status code
> + */
> +static efi_status_t tcg2_measure_secure_boot_variable(struct udevice *dev)
> +{
> +	u8 *data;
> +	efi_uintn_t data_size;
> +	u32 count, i;
> +	efi_status_t ret;
> +
> +	count = ARRAY_SIZE(secure_variables);
> +	for (i = 0; i < count; i++) {
> +		/*
> +		 * According to the TCG2 PC Client PFP spec, "SecureBoot",
> +		 * "PK", "KEK", "db" and "dbx" variables must be measured
> +		 * even if they are empty.
> +		 */
> +		data = efi_get_var(secure_variables[i].name,
> +				   secure_variables[i].guid,
> +				   &data_size);
> +
> +		ret = tcg2_measure_variable(dev, 7,
> +					    EV_EFI_VARIABLE_DRIVER_CONFIG,
> +					    secure_variables[i].name,
> +					    secure_variables[i].guid,
> +					    data_size, data);
> +		free(data);
> +		if (ret != EFI_SUCCESS)
> +			goto error;
> +	}
> +
> +	/*
> +	 * TCG2 PC Client PFP spec says "dbt" and "dbr" are
> +	 * measured if present and not empty.
> +	 */

The current implementation of secure boot doesn't support neither
dbt or dbr. Do we have to measure them now?
I think that EFI_OS_INDICATIONS_TIMESTAMP_REVOCATION bit in
osIndicationSupported should always be checked first.

> +	data = efi_get_var(L"dbt",
> +			   &efi_global_variable_guid,
> +			   &data_size);
> +	if (data) {
> +		ret = tcg2_measure_variable(dev, 7,
> +					    EV_EFI_VARIABLE_DRIVER_CONFIG,
> +					    L"dbt",
> +					    &efi_global_variable_guid,

=> efi_guid_image_security_database

> +					    data_size, data);
> +		free(data);
> +	}
> +
> +	data = efi_get_var(L"dbr",
> +			   &efi_global_variable_guid,
> +			   &data_size);
> +	if (data) {
> +		ret = tcg2_measure_variable(dev, 7,
> +					    EV_EFI_VARIABLE_DRIVER_CONFIG,
> +					    L"dbr",
> +					    &efi_global_variable_guid,

ditto for dbr

-Takahiro Akashi

> +					    data_size, data);
> +		free(data);
> +	}
> +
> +error:
> +	return ret;
> +}
> +
>  /**
>   * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
>   *
> @@ -1328,6 +1486,13 @@ efi_status_t efi_tcg2_register(void)
>  		tcg2_uninit();
>  		goto fail;
>  	}
> +
> +	ret = tcg2_measure_secure_boot_variable(dev);
> +	if (ret != EFI_SUCCESS) {
> +		tcg2_uninit();
> +		goto fail;
> +	}
> +
>  	return ret;
>  
>  fail:
> -- 
> 2.17.1
>