| Message ID | 20211203035815.27433-2-masahisa.kojima@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | fix TCG2 error handling | expand |
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 <masahisa.kojima@linaro.org> > --- > 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, &efi_guid_tcg2_protocol, &handler); > + 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, &efi_guid_tcg2_protocol, > + (void *)&efi_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(&dev); > + 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, &efi_guid_tcg2_protocol, > (void *)&efi_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 installed the PCRs won't be extended. So > - * someone later in the boot flow will notice that and take the > - * necessary actions. > - * - The TPM sandbox is limited and we won't be able to run any efi > - * related tests with TCG2 enabled > - */ > - return EFI_SUCCESS; > + return ret; > } > -- > 2.17.1 > Cheers /Ilias
On Mon, 6 Dec 2021 at 23:08, Ilias Apalodimas <ilias.apalodimas@linaro.org> 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 <masahisa.kojima@linaro.org> > > --- > > 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, &efi_guid_tcg2_protocol, &handler); > > + 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, &efi_guid_tcg2_protocol, > > + (void *)&efi_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(&dev); > > + 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) > > } > > > > ret = efi_init_event_log(); > > - if (ret != EFI_SUCCESS) > > + if (ret != EFI_SUCCESS) { > > + tcg2_uninit(); > > goto fail; > > + } > > > > ret = efi_add_protocol(efi_root, &efi_guid_tcg2_protocol, > > (void *)&efi_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 installed the PCRs won't be extended. So > > - * someone later in the boot flow will notice that and take the > > - * necessary actions. > > - * - The TPM sandbox is limited and we won't be able to run any efi > > - * related tests with TCG2 enabled > > - */ > > - return EFI_SUCCESS; > > + return ret; > > } > > -- > > 2.17.1 > > > > Cheers > /Ilias
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, &efi_guid_tcg2_protocol, &handler); + 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, &efi_guid_tcg2_protocol, + (void *)&efi_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(&dev); + 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, &efi_guid_tcg2_protocol, (void *)&efi_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 installed the PCRs won't be extended. So - * someone later in the boot flow will notice that and take the - * necessary actions. - * - The TPM sandbox is limited and we won't be able to run any efi - * related tests with TCG2 enabled - */ - return EFI_SUCCESS; + return ret; }
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 <masahisa.kojima@linaro.org> --- 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(-)