[PATCH v2 1/1] efi_loader: error handling in tcg2_hash_pe_image()

[Ilias Apalodimas]

Hi Heinrich,

On Mon, 31 Jul 2023 at 15:11, Heinrich Schuchardt
<heinrich.schuchardt at canonical.com> wrote:
>
> If the hard coded array hash_algo_list[] contains an entry for an
> unsupported algorithm, we should not leak resources new_efi and regs.
>
> We should still extend the log with the digests for the supported
> algorithms and not write any message.
>
> The same holds true of tcg2_create_digest(): just continue in case
> hash_algo_list[] contains an unsupported entry.
>
> Fixes: 163a0d7e2cbd ("efi_loader: add PE/COFF image measurement")
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> ---
> v2:
>         drop the error message
>         fix tcg2_create_digest() too
> ---
>  lib/efi_loader/efi_tcg2.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/efi_loader/efi_tcg2.c b/lib/efi_loader/efi_tcg2.c
> index 49f8a5e77c..7b7926a0d4 100644
> --- a/lib/efi_loader/efi_tcg2.c
> +++ b/lib/efi_loader/efi_tcg2.c
> @@ -706,8 +706,7 @@ static efi_status_t tcg2_create_digest(const u8 *input, u32 length,
>                         sha512_finish(&ctx_512, final);
>                         break;
>                 default:
> -                       EFI_PRINT("Unsupported algorithm %x\n", hash_alg);
> -                       return EFI_INVALID_PARAMETER;
> +                       continue;
>                 }
>                 digest_list->digests[digest_list->count].hash_alg = hash_alg;
>                 memcpy(&digest_list->digests[digest_list->count].digest, final,
> @@ -930,8 +929,7 @@ static efi_status_t tcg2_hash_pe_image(void *efi, u64 efi_size,
>                         hash_calculate("sha512", regs->reg, regs->num, hash);
>                         break;
>                 default:
> -                       EFI_PRINT("Unsupported algorithm %x\n", hash_alg);
> -                       return EFI_INVALID_PARAMETER;
> +                       continue;
>                 }
>                 digest_list->digests[digest_list->count].hash_alg = hash_alg;
>                 memcpy(&digest_list->digests[digest_list->count].digest, hash,
> --
> 2.40.1
>

I am fine with the change.  It's worth noting that this will only
happen if we ever add an invalid algorithm to hash_algo_list[].  But
the algorithms are described in the spec, so  we should never end up
in that case regardless.  Can you pick this up via the EFI tree?

Reviewed-by: Ilias Apalodimas <ilias.apalodimas at linaro.org>