[PATCH 2/7] efi_loader: fix the return values on efi_tcg

Ilias Apalodimas ilias.apalodimas at linaro.org
Sat Jun 22 20:02:51 CEST 2024


On Sat, 22 Jun 2024 at 21:01, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
>
>
>
> Am 22. Juni 2024 18:09:40 MESZ schrieb Ilias Apalodimas <ilias.apalodimas at linaro.org>:
> >Hi Heinrich,
> >
> >[...]
> >
> >> >       rc = tpm2_submit_command(dev, input_param_block,
> >> >                                output_param_block, &resp_buf_size);
> >> > @@ -714,19 +721,20 @@ efi_tcg2_get_active_pcr_banks(struct efi_tcg2_protocol *this,
> >> >                             u32 *active_pcr_banks)
> >> >   {
> >> >       struct udevice *dev;
> >> > -     efi_status_t ret;
> >> > +     efi_status_t ret = EFI_INVALID_PARAMETER;
> >> >
> >> >       EFI_ENTRY("%p, %p", this, active_pcr_banks);
> >> >
> >> > -     if (!this || !active_pcr_banks) {
> >> > -             ret = EFI_INVALID_PARAMETER;
> >> > +     if (!this || !active_pcr_banks)
> >> >               goto out;
> >> > -     }
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >>
> >> EFI_INVALID_PARAMETER does not convey the problem type.
> >> Should we return EFI_DEVICE_ERROR here?
> >>
> >> The authors of the specification only foresaw one or more of the
> >> parameters being incorrect (EFI_INVALID_PARAMETER).
> >
> >I completely agree that the result is misleading. However, I'd prefer
> >sticking to the spec for now and maybe add a comment?
> >
> >
> >>
> >> > +             goto out;
> >> > +
> >> > +     if (tcg2_get_active_pcr_banks(dev, active_pcr_banks))
> >>
> >> EFI_DEVICE_ERROR?
> >
> >Same here
> >
> >Thanks for the qucik review!
> >/Ilias
>
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

Thanks Heinrich.
FWIW I added a comment with the misleading result

>
>
> >>
> >> Best regards
> >>
> >> Heinrich
> >>
> >> >               goto out;
> >> >
> >> > -     ret = tcg2_get_active_pcr_banks(dev, active_pcr_banks);
> >> > +     ret = EFI_SUCCESS;
> >> >
> >> >   out:
> >> >       return EFI_EXIT(ret);
> >> > @@ -852,14 +860,15 @@ static efi_status_t 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;
> >> > +     efi_status_t ret = EFI_DEVICE_ERROR;
> >> > +     int rc;
> >> >
> >> > -     ret = tcg2_create_digest(dev, event, size, &digest_list);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     rc = tcg2_create_digest(dev, event, size, &digest_list);
> >> > +     if (rc)
> >> >               goto out;
> >> >
> >> > -     ret = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     rc = tcg2_pcr_extend(dev, pcr_index, &digest_list);
> >> > +     if (rc)
> >> >               goto out;
> >> >
> >> >       ret = tcg2_agile_log_append(pcr_index, event_type, &digest_list,
> >> > @@ -901,10 +910,10 @@ static efi_status_t efi_init_event_log(void)
> >> >       struct tcg2_event_log elog;
> >> >       struct udevice *dev;
> >> >       efi_status_t ret;
> >> > +     int rc;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > -             return ret;
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >> > +             return EFI_DEVICE_ERROR;
> >> >
> >> >       ret = efi_allocate_pool(EFI_BOOT_SERVICES_DATA, TPM2_EVENT_LOG_SIZE,
> >> >                               (void **)&event_log.buffer);
> >> > @@ -933,9 +942,11 @@ static efi_status_t efi_init_event_log(void)
> >> >        */
> >> >       elog.log = event_log.buffer;
> >> >       elog.log_size = TPM2_EVENT_LOG_SIZE;
> >> > -     ret = tcg2_log_prepare_buffer(dev, &elog, false);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     rc = tcg2_log_prepare_buffer(dev, &elog, false);
> >> > +     if (rc) {
> >> > +             ret = (rc == -ENOBUFS) ? EFI_BUFFER_TOO_SMALL : EFI_DEVICE_ERROR;
> >> >               goto free_pool;
> >> > +     }
> >> >
> >> >       event_log.pos = elog.log_position;
> >> >
> >> > @@ -1306,8 +1317,7 @@ efi_status_t efi_tcg2_measure_dtb(void *dtb)
> >> >       if (!is_tcg2_protocol_installed())
> >> >               return EFI_SUCCESS;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >> >               return EFI_SECURITY_VIOLATION;
> >> >
> >> >       rsvmap_size = size_of_rsvmap(dtb);
> >> > @@ -1356,8 +1366,7 @@ efi_status_t efi_tcg2_measure_efi_app_invocation(struct efi_loaded_image_obj *ha
> >> >       if (tcg2_efi_app_invoked)
> >> >               return EFI_SUCCESS;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >> >               return EFI_SECURITY_VIOLATION;
> >> >
> >> >       ret = tcg2_measure_boot_variable(dev);
> >> > @@ -1406,9 +1415,8 @@ efi_status_t efi_tcg2_measure_efi_app_exit(void)
> >> >       if (!is_tcg2_protocol_installed())
> >> >               return EFI_SUCCESS;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > -             return ret;
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >> > +             return EFI_SECURITY_VIOLATION;
> >> >
> >> >       ret = measure_event(dev, 4, EV_EFI_ACTION,
> >> >                           strlen(EFI_RETURNING_FROM_EFI_APPLICATION),
> >> > @@ -1437,9 +1445,10 @@ efi_tcg2_notify_exit_boot_services(struct efi_event *event, void *context)
> >> >               goto out;
> >> >       }
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     if (tcg2_platform_get_tpm2(&dev)) {
> >> > +             ret = EFI_SECURITY_VIOLATION;
> >> >               goto out;
> >> > +     }
> >> >
> >> >       ret = measure_event(dev, 5, EV_EFI_ACTION,
> >> >                           strlen(EFI_EXIT_BOOT_SERVICES_INVOCATION),
> >> > @@ -1469,9 +1478,8 @@ efi_status_t efi_tcg2_notify_exit_boot_services_failed(void)
> >> >       if (!is_tcg2_protocol_installed())
> >> >               return EFI_SUCCESS;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > -             goto out;
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >> > +             return EFI_SECURITY_VIOLATION;
> >> >
> >> >       ret = measure_event(dev, 5, EV_EFI_ACTION,
> >> >                           strlen(EFI_EXIT_BOOT_SERVICES_INVOCATION),
> >> > @@ -1551,8 +1559,7 @@ efi_status_t efi_tcg2_do_initial_measurement(void)
> >> >       if (!is_tcg2_protocol_installed())
> >> >               return EFI_SUCCESS;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS)
> >> > +     if (tcg2_platform_get_tpm2(&dev))
> >> >               return EFI_SECURITY_VIOLATION;
> >> >
> >> >       ret = tcg2_measure_secure_boot_variable(dev);
> >> > @@ -1577,8 +1584,7 @@ efi_status_t efi_tcg2_register(void)
> >> >       struct efi_event *event;
> >> >       u32 err;
> >> >
> >> > -     ret = tcg2_platform_get_tpm2(&dev);
> >> > -     if (ret != EFI_SUCCESS) {
> >> > +     if (tcg2_platform_get_tpm2(&dev)) {
> >> >               log_warning("Missing TPMv2 device for EFI_TCG_PROTOCOL\n");
> >> >               return EFI_SUCCESS;
> >> >       }
> >> > @@ -1586,6 +1592,7 @@ efi_status_t efi_tcg2_register(void)
> >> >       /* initialize the TPM as early as possible. */
> >> >       err = tpm_auto_start(dev);
> >> >       if (err) {
> >> > +             ret = EFI_DEVICE_ERROR;
> >> >               log_err("TPM startup failed\n");
> >> >               goto fail;
> >> >       }
> >>


More information about the U-Boot mailing list