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.aka...@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;
> > }
> >
>
