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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAC_iWjJizjQWucDbrqKGdZTcj7FFxiPN97=p1zwfnPE=sAC6RQ@mail.gmail.com>
Date: Fri, 13 Sep 2024 09:40:30 +0300
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Gregory Price <gourry@...rry.net>
Cc: linux-efi@...r.kernel.org, linux-kernel@...r.kernel.org, ardb@...nel.org, 
	leitao@...ian.org, usamaarif642@...il.com, 
	sathyanarayanan.kuppuswamy@...ux.intel.com
Subject: Re: [PATCH 4/6] tpm: sanity check the log version before using it

Hi Gregory,

On Fri, 6 Sept 2024 at 23:28, Gregory Price <gourry@...rry.net> 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@...rry.net>
> ---
>  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
>