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@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
Yes, makes sense. Thanks!
