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@xxxxxxxxx>
> >>> ---
> >>> 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
