Re: [PATCH 4/6] tpm: sanity check the log version before using it

[Ilias Apalodimas]

Hi Gregory,

On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@xxxxxxxxxx> wrote:
>
> If the log version is not sane (0 or >2), don't attempt to use
> the rest of the log values for anything to avoid potential corruption.
>
> Signed-off-by: Gregory Price <gourry@xxxxxxxxxx>
> ---
>  drivers/firmware/efi/tpm.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
>
> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> index 6e03eed0dc6f..9a080887a3e0 100644
> --- a/drivers/firmware/efi/tpm.c
> +++ b/drivers/firmware/efi/tpm.c
> @@ -60,6 +60,15 @@ int __init efi_tpm_eventlog_init(void)
>                 return -ENOMEM;
>         }
>
> +       if (!log_tbl->version ||
> +           log_tbl->version > EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> +               pr_err(FW_BUG "TPM Events table version invalid (%x)\n",
> +                      log_tbl->version);
> +               early_memunmap(log_tbl, sizeof(*log_tbl));
> +               efi.tpm_log = EFI_INVALID_TABLE_ADDR;
> +               return -EINVAL;

I don't think we need this check at all. Did you actually see this happening?
efi_retrieve_eventlog() that runs during the efistub tries to retrieve
the log and the EFI protocol itself explicitly says that the firmware
*must* return EFI_INVALID_PARAMETER if the event log is not in 1.2 or
2.0 format. If the firmware does something wrong, we should report the
FW BUG in that function, instead of installing the config tables Linux
uses internally to handover the log and catching it late.

Thanks
/Ilias



> +       }
> +
>         tbl_size = sizeof(*log_tbl) + log_tbl->size;
>         if (memblock_reserve(efi.tpm_log, tbl_size)) {
>                 pr_err("TPM Event Log memblock reserve fails (0x%lx, 0x%x)\n",
> --
> 2.43.0
>