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

[Usama Arif <usamaarif642@gmail.com>]

On 30/08/2024 17:58, Gregory Price wrote:
> 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

Yes, makes sense. Thanks!