Re: [PATCH 1/3] efi_loader: efi_tcg2_register returns appropriate error
On Mon, 6 Dec 2021 at 23:08, Ilias Apalodimas
wrote:
>
> On Fri, Dec 03, 2021 at 12:58:13PM +0900, Masahisa Kojima wrote:
> > This commit modify efi_tcg2_register() to return the
> > appropriate error.
> > With this fix, sandbox will not boot because efi_tcg2_register()
> > fails due to some missing feature in GetCapabilities.
> > So disable sandbox if EFI_TCG2_PROTOCOL is enabled.
> >
> > UEFI secure boot variable measurement is not directly related
> > to TCG2 protocol installation, tcg2_measure_secure_boot_variable()
> > is moved to the separate function.
> >
> > Signed-off-by: Masahisa Kojima
> > ---
> > include/efi_loader.h | 2 ++
> > lib/efi_loader/Kconfig | 2 ++
> > lib/efi_loader/efi_setup.c | 2 ++
> > lib/efi_loader/efi_tcg2.c | 65 +++---
> > 4 files changed, 53 insertions(+), 18 deletions(-)
> >
> > diff --git a/include/efi_loader.h b/include/efi_loader.h
> > index 67c40ca57a..f4860e87fc 100644
> > --- a/include/efi_loader.h
> > +++ b/include/efi_loader.h
> > @@ -525,6 +525,8 @@ efi_status_t efi_disk_register(void);
> > efi_status_t efi_rng_register(void);
> > /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> > efi_status_t efi_tcg2_register(void);
> > +/* Called by efi_init_obj_list() to do initial measurement */
> > +efi_status_t efi_tcg2_do_initial_measurement(void);
> > /* measure the pe-coff image, extend PCR and add Event Log */
> > efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
> > struct efi_loaded_image_obj *handle,
> > diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> > index 700dc838dd..24f9a2bb75 100644
> > --- a/lib/efi_loader/Kconfig
> > +++ b/lib/efi_loader/Kconfig
> > @@ -308,6 +308,8 @@ config EFI_TCG2_PROTOCOL
> > bool "EFI_TCG2_PROTOCOL support"
> > default y
> > depends on TPM_V2
> > + # Sandbox TPM currently fails on GetCapabilities needed for TCG2
> > + depends on !SANDBOX
> > select SHA1
> > select SHA256
> > select SHA384
> > diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> > index 1aba71cd96..f58a4afa7f 100644
> > --- a/lib/efi_loader/efi_setup.c
> > +++ b/lib/efi_loader/efi_setup.c
> > @@ -241,6 +241,8 @@ efi_status_t efi_init_obj_list(void)
> > ret = efi_tcg2_register();
> > if (ret != EFI_SUCCESS)
> > goto out;
> > +
> > + efi_tcg2_do_initial_measurement();
> > }
> >
> > /* Secure boot */
> > diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> > index 5f71b188a0..6dbdd35f29 100644
> > --- a/lib/efi_loader/efi_tcg2.c
> > +++ b/lib/efi_loader/efi_tcg2.c
> > @@ -153,6 +153,15 @@ static u16 alg_to_len(u16 hash_alg)
> > return 0;
> > }
> >
> > +static bool is_tcg2_protocol_installed(void)
> > +{
> > + struct efi_handler *handler;
> > + efi_status_t ret;
> > +
> > + ret = efi_search_protocol(efi_root, _guid_tcg2_protocol,
> > );
> > + return ret == EFI_SUCCESS;
> > +}
> > +
> > static u32 tcg_event_final_size(struct tpml_digest_values *digest_list)
> > {
> > u32 len;
> > @@ -1664,6 +1673,14 @@ void tcg2_uninit(void)
> > event_log.buffer = NULL;
> > efi_free_pool(event_log.final_buffer);
> > event_log.final_buffer = NULL;
> > +
> > + if (!is_tcg2_protocol_installed())
> > + return;
> > +
> > + ret = efi_remove_protocol(efi_root, _guid_tcg2_protocol,
> > + (void *)_tcg2_protocol);
> > + if (ret != EFI_SUCCESS)
> > + log_err("Failed to remove EFI TCG2 protocol\n");
> > }
> >
> > /**
> > @@ -2345,12 +2362,37 @@ error:
> > return ret;
> > }
> >
> > +/**
> > + * efi_tcg2_do_initial_measurement() - do initial measurement
> > + *
> > + * Return: status code
> > + */
> > +efi_status_t efi_tcg2_do_initial_measurement(void)
> > +{
> > + efi_status_t ret;
> > + struct udevice *dev;
> > +
> > + if (!is_tcg2_protocol_installed())
> > + return EFI_SUCCESS;
> > +
> > + ret = platform_get_tpm2_device();
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
>
> Would it make more sense to return a security violation here and treat this
> error similarly to patch [3/3]?
Yes, I agree. I also add the similar check for
efi_tcg2_measure_efi_app_invocation().
Thanks,
Masahisa Kojima
>
> > + ret = tcg2_measure_secure_boot_variable(dev);
> > + if (ret != EFI_SUCCESS)
> > + goto out;
> > +
> > +out:
> > + return ret;
> > +}
> > +
> > /**
> > * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
> > *
> > * If a TPM2 device is available, the TPM TCG2 Protocol is registered
> > *
> > - * Return: An error status is only returned if adding the protocol fails.
> > + * Return: status code
> > */
> > efi_status_t efi_tcg2_register(void)
> > {
> > @@ -2373,8 +2415,10 @@ efi_status_t efi_tcg2_register(void)
> >
Re: [PATCH 1/3] efi_loader: efi_tcg2_register returns appropriate error
On Fri, Dec 03, 2021 at 12:58:13PM +0900, Masahisa Kojima wrote:
> This commit modify efi_tcg2_register() to return the
> appropriate error.
> With this fix, sandbox will not boot because efi_tcg2_register()
> fails due to some missing feature in GetCapabilities.
> So disable sandbox if EFI_TCG2_PROTOCOL is enabled.
>
> UEFI secure boot variable measurement is not directly related
> to TCG2 protocol installation, tcg2_measure_secure_boot_variable()
> is moved to the separate function.
>
> Signed-off-by: Masahisa Kojima
> ---
> include/efi_loader.h | 2 ++
> lib/efi_loader/Kconfig | 2 ++
> lib/efi_loader/efi_setup.c | 2 ++
> lib/efi_loader/efi_tcg2.c | 65 +++---
> 4 files changed, 53 insertions(+), 18 deletions(-)
>
> diff --git a/include/efi_loader.h b/include/efi_loader.h
> index 67c40ca57a..f4860e87fc 100644
> --- a/include/efi_loader.h
> +++ b/include/efi_loader.h
> @@ -525,6 +525,8 @@ efi_status_t efi_disk_register(void);
> efi_status_t efi_rng_register(void);
> /* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
> efi_status_t efi_tcg2_register(void);
> +/* Called by efi_init_obj_list() to do initial measurement */
> +efi_status_t efi_tcg2_do_initial_measurement(void);
> /* measure the pe-coff image, extend PCR and add Event Log */
> efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
> struct efi_loaded_image_obj *handle,
> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> index 700dc838dd..24f9a2bb75 100644
> --- a/lib/efi_loader/Kconfig
> +++ b/lib/efi_loader/Kconfig
> @@ -308,6 +308,8 @@ config EFI_TCG2_PROTOCOL
> bool "EFI_TCG2_PROTOCOL support"
> default y
> depends on TPM_V2
> + # Sandbox TPM currently fails on GetCapabilities needed for TCG2
> + depends on !SANDBOX
> select SHA1
> select SHA256
> select SHA384
> diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
> index 1aba71cd96..f58a4afa7f 100644
> --- a/lib/efi_loader/efi_setup.c
> +++ b/lib/efi_loader/efi_setup.c
> @@ -241,6 +241,8 @@ efi_status_t efi_init_obj_list(void)
> ret = efi_tcg2_register();
> if (ret != EFI_SUCCESS)
> goto out;
> +
> + efi_tcg2_do_initial_measurement();
> }
>
> /* Secure boot */
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 5f71b188a0..6dbdd35f29 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -153,6 +153,15 @@ static u16 alg_to_len(u16 hash_alg)
> return 0;
> }
>
> +static bool is_tcg2_protocol_installed(void)
> +{
> + struct efi_handler *handler;
> + efi_status_t ret;
> +
> + ret = efi_search_protocol(efi_root, _guid_tcg2_protocol, );
> + return ret == EFI_SUCCESS;
> +}
> +
> static u32 tcg_event_final_size(struct tpml_digest_values *digest_list)
> {
> u32 len;
> @@ -1664,6 +1673,14 @@ void tcg2_uninit(void)
> event_log.buffer = NULL;
> efi_free_pool(event_log.final_buffer);
> event_log.final_buffer = NULL;
> +
> + if (!is_tcg2_protocol_installed())
> + return;
> +
> + ret = efi_remove_protocol(efi_root, _guid_tcg2_protocol,
> + (void *)_tcg2_protocol);
> + if (ret != EFI_SUCCESS)
> + log_err("Failed to remove EFI TCG2 protocol\n");
> }
>
> /**
> @@ -2345,12 +2362,37 @@ error:
> return ret;
> }
>
> +/**
> + * efi_tcg2_do_initial_measurement() - do initial measurement
> + *
> + * Return: status code
> + */
> +efi_status_t efi_tcg2_do_initial_measurement(void)
> +{
> + efi_status_t ret;
> + struct udevice *dev;
> +
> + if (!is_tcg2_protocol_installed())
> + return EFI_SUCCESS;
> +
> + ret = platform_get_tpm2_device();
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
Would it make more sense to return a security violation here and treat this
error similarly to patch [3/3]?
> + ret = tcg2_measure_secure_boot_variable(dev);
> + if (ret != EFI_SUCCESS)
> + goto out;
> +
> +out:
> + return ret;
> +}
> +
> /**
> * efi_tcg2_register() - register EFI_TCG2_PROTOCOL
> *
> * If a TPM2 device is available, the TPM TCG2 Protocol is registered
> *
> - * Return: An error status is only returned if adding the protocol fails.
> + * Return: status code
> */
> efi_status_t efi_tcg2_register(void)
> {
> @@ -2373,8 +2415,10 @@ efi_status_t efi_tcg2_register(void)
> }
>
> ret = efi_init_event_log();
> - if (ret != EFI_SUCCESS)
> + if (ret != EFI_SUCCESS) {
> + tcg2_uninit();
> goto fail;
> + }
>
> ret = efi_add_protocol(efi_root, _guid_tcg2_protocol,
> (void *)_tcg2_protocol);
> @@ -2391,24 +2435,9 @@ efi_status_t efi_tcg2_register(void)
> goto fail;
> }
>
> - ret =
[PATCH 1/3] efi_loader: efi_tcg2_register returns appropriate error
This commit modify efi_tcg2_register() to return the
appropriate error.
With this fix, sandbox will not boot because efi_tcg2_register()
fails due to some missing feature in GetCapabilities.
So disable sandbox if EFI_TCG2_PROTOCOL is enabled.
UEFI secure boot variable measurement is not directly related
to TCG2 protocol installation, tcg2_measure_secure_boot_variable()
is moved to the separate function.
Signed-off-by: Masahisa Kojima
---
include/efi_loader.h | 2 ++
lib/efi_loader/Kconfig | 2 ++
lib/efi_loader/efi_setup.c | 2 ++
lib/efi_loader/efi_tcg2.c | 65 +++---
4 files changed, 53 insertions(+), 18 deletions(-)
diff --git a/include/efi_loader.h b/include/efi_loader.h
index 67c40ca57a..f4860e87fc 100644
--- a/include/efi_loader.h
+++ b/include/efi_loader.h
@@ -525,6 +525,8 @@ efi_status_t efi_disk_register(void);
efi_status_t efi_rng_register(void);
/* Called by efi_init_obj_list() to install EFI_TCG2_PROTOCOL */
efi_status_t efi_tcg2_register(void);
+/* Called by efi_init_obj_list() to do initial measurement */
+efi_status_t efi_tcg2_do_initial_measurement(void);
/* measure the pe-coff image, extend PCR and add Event Log */
efi_status_t tcg2_measure_pe_image(void *efi, u64 efi_size,
struct efi_loaded_image_obj *handle,
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index 700dc838dd..24f9a2bb75 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -308,6 +308,8 @@ config EFI_TCG2_PROTOCOL
bool "EFI_TCG2_PROTOCOL support"
default y
depends on TPM_V2
+ # Sandbox TPM currently fails on GetCapabilities needed for TCG2
+ depends on !SANDBOX
select SHA1
select SHA256
select SHA384
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index 1aba71cd96..f58a4afa7f 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -241,6 +241,8 @@ efi_status_t efi_init_obj_list(void)
ret = efi_tcg2_register();
if (ret != EFI_SUCCESS)
goto out;
+
+ efi_tcg2_do_initial_measurement();
}
/* Secure boot */
diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
index 5f71b188a0..6dbdd35f29 100644
--- a/lib/efi_loader/efi_tcg2.c
+++ b/lib/efi_loader/efi_tcg2.c
@@ -153,6 +153,15 @@ static u16 alg_to_len(u16 hash_alg)
return 0;
}
+static bool is_tcg2_protocol_installed(void)
+{
+ struct efi_handler *handler;
+ efi_status_t ret;
+
+ ret = efi_search_protocol(efi_root, _guid_tcg2_protocol, );
+ return ret == EFI_SUCCESS;
+}
+
static u32 tcg_event_final_size(struct tpml_digest_values *digest_list)
{
u32 len;
@@ -1664,6 +1673,14 @@ void tcg2_uninit(void)
event_log.buffer = NULL;
efi_free_pool(event_log.final_buffer);
event_log.final_buffer = NULL;
+
+ if (!is_tcg2_protocol_installed())
+ return;
+
+ ret = efi_remove_protocol(efi_root, _guid_tcg2_protocol,
+ (void *)_tcg2_protocol);
+ if (ret != EFI_SUCCESS)
+ log_err("Failed to remove EFI TCG2 protocol\n");
}
/**
@@ -2345,12 +2362,37 @@ error:
return ret;
}
+/**
+ * efi_tcg2_do_initial_measurement() - do initial measurement
+ *
+ * Return: status code
+ */
+efi_status_t efi_tcg2_do_initial_measurement(void)
+{
+ efi_status_t ret;
+ struct udevice *dev;
+
+ if (!is_tcg2_protocol_installed())
+ return EFI_SUCCESS;
+
+ ret = platform_get_tpm2_device();
+ if (ret != EFI_SUCCESS)
+ goto out;
+
+ ret = tcg2_measure_secure_boot_variable(dev);
+ if (ret != EFI_SUCCESS)
+ goto out;
+
+out:
+ return ret;
+}
+
/**
* efi_tcg2_register() - register EFI_TCG2_PROTOCOL
*
* If a TPM2 device is available, the TPM TCG2 Protocol is registered
*
- * Return: An error status is only returned if adding the protocol fails.
+ * Return: status code
*/
efi_status_t efi_tcg2_register(void)
{
@@ -2373,8 +2415,10 @@ efi_status_t efi_tcg2_register(void)
}
ret = efi_init_event_log();
- if (ret != EFI_SUCCESS)
+ if (ret != EFI_SUCCESS) {
+ tcg2_uninit();
goto fail;
+ }
ret = efi_add_protocol(efi_root, _guid_tcg2_protocol,
(void *)_tcg2_protocol);
@@ -2391,24 +2435,9 @@ efi_status_t efi_tcg2_register(void)
goto fail;
}
- ret = tcg2_measure_secure_boot_variable(dev);
- if (ret != EFI_SUCCESS) {
- tcg2_uninit();
- goto fail;
- }
-
return ret;
fail:
log_err("Cannot install EFI_TCG2_PROTOCOL\n");
- /*
-* Return EFI_SUCCESS and don't stop the EFI subsystem.
-* That's done for 2 reasons
-* - If the protocol is not
