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

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

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;
  }