Re: [PATCH v7 3/3] efi_loader: Extend PCR's for firmware measurements

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

Hi Ruchika,

On Fri, 26 Nov 2021 at 13:53, Ruchika Gupta <ruchika.gupta@linaro.org> wrote:
>
> Firmwares before U-Boot may be capable of doing tpm measurements
> and passing them to U-Boot in the form of eventlog. However there
> may be scenarios where the firmwares don't have TPM driver and
> are not capable of extending the measurements in the PCRs.
> Based on TCG spec, if previous firnware has extended PCR's, PCR0
> would not be 0. So, read the PCR0 to determine if the PCR's need
> to be extended as eventlog is parsed or not.
>
> Signed-off-by: Ruchika Gupta <ruchika.gupta@linaro.org>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Tested-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
> v7:
> Addressed Heinrick's comments - Added missing parameter in function header
>
> v6: Changed TPM2_DIGEST_LEN to TPM2_SHA512_DIGEST_SIZE
>
> v5 : No change
>
> v4 : No change
>
> v3 :
> Rebase changes on top of changes made in first patch series
>
> v2 :
> Removed check for PCR0 in eventlog
>
>  lib/efi_loader/efi_tcg2.c | 76 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 76 insertions(+)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 5ded57fd29..d247179fbf 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -199,6 +199,44 @@ static efi_status_t tcg2_pcr_extend(struct udevice *dev, u32 pcr_index,
>         return EFI_SUCCESS;
>  }
>
> +/* tcg2_pcr_read - Read PCRs for a TPM2 device for a given tpml_digest_values
> + *
> + * @dev:               device
> + * @pcr_index:         PCR index
> + * @digest_list:       list of digest algorithms to extend
> + *
> + * @Return: status code
> + */
> +static efi_status_t tcg2_pcr_read(struct udevice *dev, u32 pcr_index,
> +                                 struct tpml_digest_values *digest_list)
> +{
> +       struct tpm_chip_priv *priv;
> +       unsigned int updates, pcr_select_min;
> +       u32 rc;
> +       size_t i;
> +
> +       priv = dev_get_uclass_priv(dev);
> +       if (!priv)
> +               return EFI_DEVICE_ERROR;
> +
> +       pcr_select_min = priv->pcr_select_min;
> +
> +       for (i = 0; i < digest_list->count; i++) {
> +               u16 hash_alg = digest_list->digests[i].hash_alg;
> +               u8 *digest = (u8 *)&digest_list->digests[i].digest;
> +
> +               rc = tpm2_pcr_read(dev, pcr_index, pcr_select_min,
> +                                  hash_alg, digest, alg_to_len(hash_alg),
> +                                  &updates);
> +               if (rc) {
> +                       EFI_PRINT("Failed to read PCR\n");
> +                       return EFI_DEVICE_ERROR;
> +               }
> +       }
> +
> +       return EFI_SUCCESS;
> +}
> +
>  /* put_event - Append an agile event to an eventlog
>   *
>   * @pcr_index:         PCR index
> @@ -1461,6 +1499,8 @@ static efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
>         u32 pcr, pos;
>         u64 base;
>         u32 sz;
> +       bool extend_pcr = false;
> +       int i;
>
>         ret = platform_get_eventlog(dev, &base, &sz);
>         if (ret != EFI_SUCCESS)
> @@ -1482,6 +1522,26 @@ static efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
>                 return ret;
>         }
>
> +       ret = tcg2_pcr_read(dev, 0, &digest_list);
> +       if (ret) {
> +               log_err("Error reading PCR 0\n");
> +               return ret;
> +       }
> +
> +       /*
> +        * If PCR0 is 0, previous firmware didn't have the capability
> +        * to extend the PCR. In this scenario, extend the PCR as
> +        * the eventlog is parsed.
> +        */
> +       for (i = 0; i < digest_list.count; i++) {
> +               u8 buffer[TPM2_SHA512_DIGEST_SIZE] =  { 0 };

I missed that since it doesn't show on this diff but on [1/3].
However there's a prexisting variable in the begging called 'void
*buffer', so this needs a different name

> +               u16 hash_alg = digest_list.digests[i].hash_alg;
> +
> +               if (!memcmp((u8 *)&digest_list.digests[i].digest, buffer,
> +                           alg_to_len(hash_alg)))
> +                       extend_pcr = true;
> +       }
> +
>         while (pos < sz) {
>                 ret = tcg2_parse_event(dev, buffer, sz, &pos, &digest_list,
>                                        &pcr);
> @@ -1489,6 +1549,22 @@ static efi_status_t tcg2_get_fw_eventlog(struct udevice *dev, void *log_buffer,
>                         log_err("Error parsing event\n");
>                         return ret;
>                 }
> +               if (extend_pcr) {
> +                       ret = tcg2_pcr_extend(dev, pcr, &digest_list);
> +                       if (ret != EFI_SUCCESS) {
> +                               log_err("Error in extending PCR\n");
> +                               return ret;
> +                       }
> +
> +                       /* Clear the digest for next event */
> +                       for (i = 0; i < digest_list.count; i++) {
> +                               u16 hash_alg = digest_list.digests[i].hash_alg;
> +                               u8 *digest =
> +                                  (u8 *)&digest_list.digests[i].digest;
> +
> +                               memset(digest, 0, alg_to_len(hash_alg));
> +                       }
> +               }
>         }
>
>         memcpy(log_buffer, buffer, sz);
> --
> 2.25.1
>

Cheers
/Ilias