Re: [PATCH] efi: reserve memory for tpm_log only if TPM final events table is valid

[Gregory Price <gourry@gourry.net>]

On Fri, Aug 30, 2024 at 10:52:48PM +0100, Usama Arif wrote:
> 
> 
> On 30/08/2024 17:49, Usama Arif wrote:
> > 
> > 
> > On 30/08/2024 17:42, Gregory Price wrote:
> >> On Fri, Aug 30, 2024 at 10:28:52PM +0100, Usama Arif wrote:
> >>> If efi.tpm_log is corrupted, log_tbl->size can be garbage (and
> >>> negative). This can result in a large memblock reservation, resulting
> >>> in the kernel booting without sufficient memory. Move the memblock
> >>> reservation after log_tbl->version check, and check the value of
> >>> both tbl_size and memblock_reserve.
> >>>
> >>> Signed-off-by: Usama Arif <usamaarif642@gmail.com>
> >>> ---
> >>>  drivers/firmware/efi/tpm.c | 16 +++++++++++++---
> >>>  1 file changed, 13 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/drivers/firmware/efi/tpm.c b/drivers/firmware/efi/tpm.c
> >>> index e8d69bd548f3..cfc6a065f441 100644
> >>> --- a/drivers/firmware/efi/tpm.c
> >>> +++ b/drivers/firmware/efi/tpm.c
> >>> @@ -59,9 +59,6 @@ int __init efi_tpm_eventlog_init(void)
> >>>  		return -ENOMEM;
> >>>  	}
> >>>  
> >>> -	tbl_size = sizeof(*log_tbl) + log_tbl->size;
> >>> -	memblock_reserve(efi.tpm_log, tbl_size);
> >>> -
> >>>  	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
> >>>  		pr_info("TPM Final Events table not present\n");
> >>>  		goto out;
> >>
> >> The final event table is not present in TCG 1_2 format, but the
> >> tpm log still needs to be mapped.  So this change is incorrect for
> >> v1_2.
> > 
> > hmm so we have 
> > 
> > 	if (efi.tpm_final_log == EFI_INVALID_TABLE_ADDR) {
> > 		pr_info("TPM Final Events table not present\n");
> > 		goto out;
> > 	} else if (log_tbl->version != EFI_TCG2_EVENT_LOG_FORMAT_TCG_2) {
> > 		pr_warn(FW_BUG "TPM Final Events table invalid\n");
> > 		goto out;
> > 	}
> > 
> > If the format is TCG 1_2, then log_tbl is not used?
> > 
> 
> Ah its the case that log_tbl->size will be valid, even if its TCG 2.
> Got it, Thanks!

No, not necessarily.  The corruption issue is entirely separate from the
patches here. size can technically be 0xffffffff and treated as valid
because as far as I can see the spec does not define a maximum size.

the tl;dr here is that the above checks on tpm_final_log and log_tbl->version
gate mapping/operating on the final log - but do not have anything to
say about the event log.

There isn't really a good sanity check for whether or not to memblock_reserve
the log.  Possibly you add a LOG_FORMAT_MAX and check if version >= MAX,
but again that should be separate from the change that checks the return
value of the memblock_reserve call.

~Gregory