[PATCH v7 05/17] efi_loader: variable: add secure boot state transition

AKASHI Takahiro takahiro.akashi at linaro.org
Fri Apr 17 00:53:06 CEST 2020


Heinrich,

On Thu, Apr 16, 2020 at 04:20:35PM +0200, Heinrich Schuchardt wrote:
> On 14.04.20 04:51, AKASHI Takahiro wrote:
> > UEFI specification defines several global variables which are related to
> > the current secure boot state. In this commit, those values will be
> > maintained according to operations. Currently, AuditMode and DeployedMode
> > are defined but not implemented.
> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi at linaro.org>
> > ---
> >  lib/efi_loader/efi_variable.c | 231 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 228 insertions(+), 3 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_variable.c b/lib/efi_loader/efi_variable.c
> > index adb78470f2d6..fd5c41f830b1 100644
> > --- a/lib/efi_loader/efi_variable.c
> > +++ b/lib/efi_loader/efi_variable.c
> > @@ -16,8 +16,16 @@
> >  #include <u-boot/crc.h>
> >  #include "../lib/crypto/pkcs7_parser.h"
> >
> > +enum efi_secure_mode {
> > +	EFI_MODE_SETUP,
> > +	EFI_MODE_USER,
> > +	EFI_MODE_AUDIT,
> > +	EFI_MODE_DEPLOYED,
> > +};
> > +
> >  const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >  static bool efi_secure_boot;
> > +static int efi_secure_mode;
> >
> >  #define READ_ONLY BIT(31)
> >
> > @@ -160,6 +168,210 @@ static const char *parse_attr(const char *str, u32 *attrp, u64 *timep)
> >  	return str;
> >  }
> >
> > +static efi_status_t efi_set_variable_internal(u16 *variable_name,
> > +					      const efi_guid_t *vendor,
> > +					      u32 attributes,
> > +					      efi_uintn_t data_size,
> > +					      const void *data,
> > +					      bool ro_check);
> > +
> > +/**
> > + * efi_transfer_secure_state - handle a secure boot state transition
> > + * @mode:	new state
> > + *
> > + * Depending on @mode, secure boot related variables are updated.
> > + * Those variables are *read-only* for users, efi_set_variable_internal()
> > + * is called here.
> > + *
> > + * Return:	EFI_SUCCESS on success, status code (negative) on error
> > + */
> > +static efi_status_t efi_transfer_secure_state(enum efi_secure_mode mode)
> > +{
> > +	u32 attributes;
> > +	u8 val;
> > +	efi_status_t ret;
> > +
> > +	debug("Secure state from %d to %d\n", efi_secure_mode, mode);
> 
> Do you mean "Switching secure state from %d to %d\n"?
> 
> > +
> > +	attributes = EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > +		     EFI_VARIABLE_RUNTIME_ACCESS;
> > +	if (mode == EFI_MODE_DEPLOYED) {
> 
> Please, use switch.
> 
> > +		val = 1;
> > +		ret = efi_set_variable_internal(L"SecureBoot",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"SetupMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"AuditMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 1;
> > +		ret = efi_set_variable_internal(L"DeployedMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +
> 
> You are repeating this code block four times. Please extract a function,
> eg. efi_set_secure_state(). Than this reduces to
> 
> switch (mode) {
> case EFI_MODE_DEPLOYED:
> 	ret = efi_set_secure_stat(1, 0, 0, 1);
> 	break;
> case EFI_MODE_AUDIT:
> 	ret = efi_set_variable_internal(L"PK",
> 					&efi_global_variable_guid,
> 					attributes,
> 					0, NULL, false);
> 	if (ret == EFI_SUCCESS)
> 		ret = efi_set_secure_stat(0, 1, 1, 0);
> 	break;
> case EFI_MODE_USER:
> 	ret = efi_set_secure_stat(1, 0, 0, 0);
> 	break;
> ...

Okay, but please let me know when you finish to review
*all* the patches.
I have not changed the code in this commit for a long time
(probably since v1) and don't want to repeatedly submit the whole set
which is only partially reviewed.

Thanks,
-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > +		efi_secure_boot = true;
> > +	} else if (mode == EFI_MODE_AUDIT) {
> > +		ret = efi_set_variable_internal(L"PK",
> > +						&efi_global_variable_guid,
> > +						attributes,
> > +						0, NULL,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"SecureBoot",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 1;
> > +		ret = efi_set_variable_internal(L"SetupMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 1;
> > +		ret = efi_set_variable_internal(L"AuditMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"DeployedMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +
> > +		efi_secure_boot = true;
> > +	} else if (mode == EFI_MODE_USER) {
> > +		val = 1;
> > +		ret = efi_set_variable_internal(L"SecureBoot",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"SetupMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"AuditMode",
> > +						&efi_global_variable_guid,
> > +						attributes,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"DeployedMode",
> > +						&efi_global_variable_guid,
> > +						attributes,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +
> > +		efi_secure_boot = true;
> > +	} else if (mode == EFI_MODE_SETUP) {
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"SecureBoot",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 1;
> > +		ret = efi_set_variable_internal(L"SetupMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"AuditMode",
> > +						&efi_global_variable_guid,
> > +						attributes,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +		val = 0;
> > +		ret = efi_set_variable_internal(L"DeployedMode",
> > +						&efi_global_variable_guid,
> > +						attributes | READ_ONLY,
> > +						sizeof(val), &val,
> > +						false);
> > +		if (ret != EFI_SUCCESS)
> > +			goto err;
> > +	} else {
> > +		return EFI_INVALID_PARAMETER;
> > +	}
> > +
> > +	return EFI_SUCCESS;
> > +
> > +err:
> > +	/* TODO: What action should be taken here? */
> > +	printf("ERROR: Secure state transition failed\n");
> > +	return ret;
> > +}
> > +
> > +/**
> > + * efi_init_secure_state - initialize secure boot state
> > + *
> > + * Return:	EFI_SUCCESS on success, status code (negative) on error
> > + */
> > +static efi_status_t efi_init_secure_state(void)
> > +{
> > +	efi_uintn_t size = 0;
> > +	efi_status_t ret;
> > +
> > +	ret = EFI_CALL(efi_get_variable(L"PK", &efi_global_variable_guid,
> > +					NULL, &size, NULL));
> > +	if (ret == EFI_BUFFER_TOO_SMALL && IS_ENABLED(CONFIG_EFI_SECURE_BOOT))
> > +		ret = efi_transfer_secure_state(EFI_MODE_USER);
> > +	else
> > +		ret = efi_transfer_secure_state(EFI_MODE_SETUP);
> > +
> > +	return ret;
> > +}
> > +
> >  /**
> >   * efi_secure_boot_enabled - return if secure boot is enabled or not
> >   *
> > @@ -910,10 +1122,19 @@ efi_status_t EFIAPI efi_set_variable_common(u16 *variable_name,
> >  	EFI_PRINT("setting: %s=%s\n", native_name, val);
> >
> >  out:
> > -	if (env_set(native_name, val))
> > +	if (env_set(native_name, val)) {
> >  		ret = EFI_DEVICE_ERROR;
> > -	else
> > +	} else {
> > +		if ((u16_strcmp(variable_name, L"PK") == 0 &&
> > +		     guidcmp(vendor, &efi_global_variable_guid) == 0)) {
> > +			ret = efi_transfer_secure_state(
> > +					(delete ? EFI_MODE_SETUP :
> > +						  EFI_MODE_USER));
> > +			if (ret != EFI_SUCCESS)
> > +				goto err;
> > +		}
> >  		ret = EFI_SUCCESS;
> > +	}
> >
> >  err:
> >  	free(native_name);
> > @@ -1098,5 +1319,9 @@ void efi_variables_boot_exit_notify(void)
> >   */
> >  efi_status_t efi_init_variables(void)
> >  {
> > -	return EFI_SUCCESS;
> > +	efi_status_t ret;
> > +
> > +	ret = efi_init_secure_state();
> > +
> > +	return ret;
> >  }
> >
> 


More information about the U-Boot mailing list