efi: reserve memory for tpm_log only if TPM final events table is valid

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

Posted by Usama Arif 1 year, 3 months ago

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;
@@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void)
 		goto out;
 	}
 
+	tbl_size = sizeof(*log_tbl) + log_tbl->size;
+	if (tbl_size < 0) {
+		pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n");
+		ret = -EINVAL;
+		goto out;
+	}
+	if (memblock_reserve(efi.tpm_log, tbl_size)) {
+		pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n",
+		       efi.tpm_log, tbl_size);
+		ret = -ENOMEM;
+		goto out;
+	}
+
 	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
 
 	if (!final_tbl) {
-- 
2.43.5

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

Posted by Gregory Price 1 year, 3 months ago

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.

> @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void)
>  		goto out;
>  	}
>  
> +	tbl_size = sizeof(*log_tbl) + log_tbl->size;
> +	if (tbl_size < 0) {
> +		pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n");
> +		ret = -EINVAL;
> +		goto out;
> +	}
> +	if (memblock_reserve(efi.tpm_log, tbl_size)) {
> +		pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n",
> +		       efi.tpm_log, tbl_size);
> +		ret = -ENOMEM;
> +		goto out;
> +	}
> +
>  	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
>  
>  	if (!final_tbl) {
> -- 
> 2.43.5
>

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

Posted by Usama Arif 1 year, 3 months ago


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?


> 
>> @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void)
>>  		goto out;
>>  	}
>>  
>> +	tbl_size = sizeof(*log_tbl) + log_tbl->size;
>> +	if (tbl_size < 0) {
>> +		pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n");
>> +		ret = -EINVAL;
>> +		goto out;
>> +	}
>> +	if (memblock_reserve(efi.tpm_log, tbl_size)) {
>> +		pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n",
>> +		       efi.tpm_log, tbl_size);
>> +		ret = -ENOMEM;
>> +		goto out;
>> +	}
>> +
>>  	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
>>  
>>  	if (!final_tbl) {
>> -- 
>> 2.43.5
>>

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

Posted by Gregory Price 1 year, 3 months ago

On Fri, Aug 30, 2024 at 10:49:46PM +0100, 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?
> 
> 

if format is tcg 1_2 then log_tbl is used, which is reflected in libstub
drivers/firmware/efi/libstub/tpm.c

        efi_guid_t tpm2_guid = EFI_TCG2_PROTOCOL_GUID;
        int version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_2;

        status = efi_bs_call(locate_protocol, &tpm2_guid, NULL, (void **)&tpm2);
        if (status == EFI_SUCCESS) {
                status = efi_call_proto(tpm2, get_event_log, version, &log_location,
                                        &log_last_entry, &truncated);

                if (status != EFI_SUCCESS || !log_location) {
                        version = EFI_TCG2_EVENT_LOG_FORMAT_TCG_1_2;
                        status = efi_call_proto(tpm2, get_event_log, version,
                                                &log_location, &log_last_entry,
                                                &truncated);
			^^^^^^^^ log still present in TCG 1_2 ^^^^^^^^^^^
                } else {
                        final_events_table =
                                get_efi_config_table(EFI_TCG2_FINAL_EVENTS_TABLE_GUID);
                }
        } else {

~Gregory

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

Posted by Usama Arif 1 year, 3 months ago


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!

> 
>>
>>> @@ -70,6 +67,19 @@ int __init efi_tpm_eventlog_init(void)
>>>  		goto out;
>>>  	}
>>>  
>>> +	tbl_size = sizeof(*log_tbl) + log_tbl->size;
>>> +	if (tbl_size < 0) {
>>> +		pr_err(FW_BUG "Failed to parse event in TPM Final Events Log\n");
>>> +		ret = -EINVAL;
>>> +		goto out;
>>> +	}
>>> +	if (memblock_reserve(efi.tpm_log, tbl_size)) {
>>> +		pr_err("TPM Event Log memblock reserve fails 0x%lx - %x\n",
>>> +		       efi.tpm_log, tbl_size);
>>> +		ret = -ENOMEM;
>>> +		goto out;
>>> +	}
>>> +
>>>  	final_tbl = early_memremap(efi.tpm_final_log, sizeof(*final_tbl));
>>>  
>>>  	if (!final_tbl) {
>>> -- 
>>> 2.43.5
>>>
>

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

Posted by Gregory Price 1 year, 3 months ago

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

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

Posted by Usama Arif 1 year, 3 months ago


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!