efistub/tpm: Use ACPI reclaim memory for event log to avoid corruption

PATCH In review Ard Biesheuvel 7 participants 20 messages Sep 12, 2024 – Oct 31, 2024

Subsystems (2)

Stable branch EFI

lore

Share & tools

lore

Ard Biesheuvel

author

2024-09-12 15:52 UTC #0 lore raw

PATCH 1/1 efistub/tpm: Use ACPI reclaim memory for event log to avoid corruption

From: Ard Biesheuvel <[email protected]>

The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.

The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.

Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.

Cc <[email protected]>

Reported-by Breno Leitao <[email protected]>

Tested-by Usama Arif <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 drivers/firmware/efi/libstub/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/firmware/efi/libstub/tpm.c +1 -1

index df3182f2e63a..1fd6823248ab 100644
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
 	}
 
 	/* Allocate space for the logs and copy them. */
-	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
 			     sizeof(*log_tbl) + log_size, (void **)&log_tbl);
 
 	if (status != EFI_SUCCESS) {

Signature · 1 line

2.46.0.662.g92d0881bb0-goog

Ilias Apalodimas

reviewer Reviewed-by on v1 1/1

2024-09-13 06:28 UTC #1 lore raw

Quoted 6 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.

an EFI*

Quoted 34 lines

The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.
Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 drivers/firmware/efi/libstub/tpm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/libstub/tpm.c +1 -2
index df3182f2e63a..1fd6823248ab 100644
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
        }
 
        /* Allocate space for the logs and copy them. */
-       status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+       status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
                             sizeof(*log_tbl) + log_size, (void **)&log_tbl);
 
        if (status != EFI_SUCCESS) {
--

Quoted 3 lines

2.46.0.662.g92d0881bb0-goog

Reviewed-by Ilias Apalodimas <[email protected]>

Breno Leitao

Tested-by on v1 1/1

2024-09-13 10:00 UTC #2 lore raw

In reply to Ard Biesheuvel

Hello Ard,

Thanks for the fix!

Quoted 17 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.
The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.

Quoted 4 lines

Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

Tested-by Breno Leitao <[email protected]>

For stable, having the fixes will help stable folks to decide where to pick. Based on investigation, this was introduced by 33b6d03469b22, so, we might want to have the following line:

Fixes 33b6d03469b22 ("efi: call get_event_log before ExitBootServices")

Thanks again!
--breno

Jiri Slaby

2024-10-24 16:20 UTC #3 lore raw

In reply to Ard Biesheuvel

Quoted 30 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.
The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.
Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
  drivers/firmware/efi/libstub/tpm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/libstub/tpm.c +0 -0
index df3182f2e63a..1fd6823248ab 100644

drivers/firmware/efi/libstub/tpm.c static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca · line 100

Quoted 6 lines

drivers/firmware/efi/libstub/tpm.c +1 -1

@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
  	}
 
  	/* Allocate space for the logs and copy them. */
-	status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+	status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
  			     sizeof(*log_tbl) + log_size, (void **)&log_tbl);

Hi,

this, for some reason, corrupts system configuration table. On good boots, memattr points to 0x77535018, on bad boots (this commit applied), it points to 0x77526018.

And the good content at 0x77526018:
tab=0x77526018 size=16+45*48=0x0000000000000880

bad content at 0x77535018:
tab=0x77535018 size=16+2*1705353216=0x00000000cb4b4010

This happens only on cold boots. Subsequent boots (having the commit or not) are all fine.

Any ideas?

DMI: Dell Inc. Latitude 7290/09386V, BIOS 1.39.0 07/04/2024

This was reported downstream at:
https://bugzilla.suse.com/show_bug.cgi?id=1231465

thanks,

Signature · 2 lines

js
suse labs

Jiri Slaby

2024-10-25 05:07 UTC #4 lore raw

Quoted 45 lines · Jiri Slaby

Quoted 30 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.
The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.
Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/libstub/tpm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/libstub/tpm.c +0 -0
index df3182f2e63a..1fd6823248ab 100644
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version, 
Quoted 6 lines
efi_physical_addr_t log_loca
      }
      /* Allocate space for the logs and copy them. */
-    status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, +    status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY, sizeof(*log_tbl) + log_size, (void **)&log_tbl);
Hi,
this, for some reason, corrupts system configuration table. On good boots, memattr points to 0x77535018, on bad boots (this commit applied), it points to 0x77526018.
And the good content at 0x77526018:
tab=0x77526018 size=16+45*48=0x0000000000000880

Quoted 6 lines

bad content at 0x77535018:
tab=0x77535018 size=16+2*1705353216=0x00000000cb4b4010
This happens only on cold boots. Subsequent boots (having the commit or not) are all fine.
Any ideas?

====
EFI_ACPI_RECLAIM_MEMORY

This memory is to be preserved by the UEFI OS loader and OS until ACPI
is enabled. Once ACPI is enabled, the memory in this range is available for general use.
====

BTW doesn't the above mean it is released by the time TPM actually reads it?

Isn't the proper fix to actually memblock_reserve() that TPM portion. The same as memattr in efi_memattr_init()?

Quoted 6 lines

DMI: Dell Inc. Latitude 7290/09386V, BIOS 1.39.0 07/04/2024
This was reported downstream at:
https://bugzilla.suse.com/show_bug.cgi?id=1231465
thanks,

Signature · 2 lines

js
suse labs

Jiri Slaby

2024-10-25 05:09 UTC #5 lore raw

Quoted 56 lines · Jiri Slaby

Quoted 45 lines · Jiri Slaby
Quoted 30 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.
The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.
Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/libstub/tpm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/libstub/tpm.c +0 -0
index df3182f2e63a..1fd6823248ab 100644
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version, 
Quoted 6 lines
efi_physical_addr_t log_loca
      }
      /* Allocate space for the logs and copy them. */
-    status = efi_bs_call(allocate_pool, EFI_LOADER_DATA, +    status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY, sizeof(*log_tbl) + log_size, (void **)&log_tbl);
Hi,
this, for some reason, corrupts system configuration table. On good boots, memattr points to 0x77535018, on bad boots (this commit applied), it points to 0x77526018.
And the good content at 0x77526018:
tab=0x77526018 size=16+45*48=0x0000000000000880
Quoted 6 lines
bad content at 0x77535018:
tab=0x77535018 size=16+2*1705353216=0x00000000cb4b4010
This happens only on cold boots. Subsequent boots (having the commit or not) are all fine.
Any ideas?
====
EFI_ACPI_RECLAIM_MEMORY

Quoted 7 lines

This memory is to be preserved by the UEFI OS loader and OS until ACPI
is enabled. Once ACPI is enabled, the memory in this range is available for general use.
====
BTW doesn't the above mean it is released by the time TPM actually reads it?
Isn't the proper fix to actually memblock_reserve() that TPM portion. The same as memattr in efi_memattr_init()?

And this is actually done in efi_tpm_eventlog_init().

Quoted 6 lines

Quoted 6 lines
DMI: Dell Inc. Latitude 7290/09386V, BIOS 1.39.0 07/04/2024
This was reported downstream at:
https://bugzilla.suse.com/show_bug.cgi?id=1231465
thanks,

Signature · 2 lines

js
suse labs

Ard Biesheuvel

maintainer

2024-10-25 07:31 UTC #6 lore raw

Quoted 67 lines · Jiri Slaby

Quoted 65 lines · Jiri Slaby
Quoted 48 lines · Jiri Slaby
Quoted 34 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.
The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.
Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
  drivers/firmware/efi/libstub/tpm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/ renamed from drivers/firmware/efi/libstub/tpm.c +0 -0
efi/libstub/tpm.c
index df3182f2e63a..1fd6823248ab 100644
drivers/firmware/efi/libstub/tpm.c +0 -0
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version,
efi_physical_addr_t log_loca
      }
      /* Allocate space for the logs and copy them. */
drivers/firmware/efi/libstub/tpm.c static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca · line 100
Quoted 3 lines
drivers/firmware/efi/libstub/tpm.c +1 -1
-    status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+    status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
                   sizeof(*log_tbl) + log_size, (void **)&log_tbl);
Hi,
this, for some reason, corrupts system configuration table. On good boots, memattr points to 0x77535018, on bad boots (this commit applied), it points to 0x77526018.
And the good content at 0x77526018:
tab=0x77526018 size=16+45*48=0x0000000000000880
Quoted 7 lines
bad content at 0x77535018:
tab=0x77535018 size=16+2*1705353216=0x00000000cb4b4010
This happens only on cold boots. Subsequent boots (having the commit or not) are all fine.
Any ideas?
====
EFI_ACPI_RECLAIM_MEMORY
This memory is to be preserved by the UEFI OS loader and OS until ACPI is enabled. Once ACPI is enabled, the memory in this range is available for general use.
====

Quoted 8 lines

Quoted 5 lines
BTW doesn't the above mean it is released by the time TPM actually reads it?
Isn't the proper fix to actually memblock_reserve() that TPM portion. The same as memattr in efi_memattr_init()?
And this is actually done in efi_tpm_eventlog_init().

EFI_ACPI_RECLAIM_MEMORY may be reclaimed by the OS, but we never actually do that in Linux.

To me, it seems like the use of EFI_ACPI_RECLAIM_MEMORY in this case simply tickles a bug in the firmware that causes it to corrupt the memory attributes table. The fact that cold boot behaves differently is a strong indicator here.

I didn't see the results of the memory attribute table dumps on the bugzilla thread, but dumping this table from EFI is not very useful because it will get regenerated/updated at ExitBootServices() time. Unfortunately, that also takes away the console so capturing the state of that table before the EFI stub boots the kernel is not an easy thing to do.

Is the memattr table completely corrupted? It also has a version field, and only versions 1 and 2 are defined so we might use that to detect corruption.

Gregory Price

2024-10-30 16:32 UTC #7 lore raw

Quoted 37 lines · Ard Biesheuvel

Quoted 12 lines · Jiri Slaby
Quoted 10 lines · Jiri Slaby
On 24. 10. 24, 18:20, Jiri Slaby wrote:
====
EFI_ACPI_RECLAIM_MEMORY
This memory is to be preserved by the UEFI OS loader and OS until ACPI is enabled. Once ACPI is enabled, the memory in this range is available for general use.
====
Quoted 8 lines
Quoted 5 lines
BTW doesn't the above mean it is released by the time TPM actually reads it?
Isn't the proper fix to actually memblock_reserve() that TPM portion. The same as memattr in efi_memattr_init()?
And this is actually done in efi_tpm_eventlog_init().
EFI_ACPI_RECLAIM_MEMORY may be reclaimed by the OS, but we never actually do that in Linux.
To me, it seems like the use of EFI_ACPI_RECLAIM_MEMORY in this case simply tickles a bug in the firmware that causes it to corrupt the memory attributes table. The fact that cold boot behaves differently is a strong indicator here.
I didn't see the results of the memory attribute table dumps on the bugzilla thread, but dumping this table from EFI is not very useful because it will get regenerated/updated at ExitBootServices() time. Unfortunately, that also takes away the console so capturing the state of that table before the EFI stub boots the kernel is not an easy thing to do.

Quoted 3 lines

Is the memattr table completely corrupted? It also has a version field, and only versions 1 and 2 are defined so we might use that to detect corruption.

When we initially identified the TPM log corruption issue, I had a gut feeling we were about to discover a lot more corruption along the same lines. It feels like e820 should have significantly more ACPI entries marked to avoid kexec from touching it - instead of just 1 or 2.

Hopefully I'm wrong, I'll take a look at the raw memory attributes on a few systems and see if there's a disagreement between UEFI and e820.

Not looking forward to a thrilling game of whack-a-mole :[

~Gregory

Jiri Slaby

2024-10-31 07:55 UTC #8 lore raw

In reply to Ard Biesheuvel

Quoted 12 lines · Ard Biesheuvel

To me, it seems like the use of EFI_ACPI_RECLAIM_MEMORY in this case simply tickles a bug in the firmware that causes it to corrupt the memory attributes table. The fact that cold boot behaves differently is a strong indicator here.
I didn't see the results of the memory attribute table dumps on the bugzilla thread, but dumping this table from EFI is not very useful because it will get regenerated/updated at ExitBootServices() time. Unfortunately, that also takes away the console so capturing the state of that table before the EFI stub boots the kernel is not an easy thing to do.

Quoted 3 lines

Is the memattr table completely corrupted? It also has a version field, and only versions 1 and 2 are defined so we might use that to detect corruption.

So from a today test:
https://bugzilla.suse.com/attachment.cgi?id=878296

Quoted 1 line

efi: memattr: efi_memattr_init: tab=0x7752f018 ver=1 size=16+2*1705287680=0x00000000cb494010

version is NOT corrupted :).

thanks,

Signature · 2 lines

js
suse labs

Ard Biesheuvel

maintainer

2024-10-31 09:04 UTC #9 lore raw

Quoted 18 lines · Jiri Slaby

Quoted 12 lines · Ard Biesheuvel
To me, it seems like the use of EFI_ACPI_RECLAIM_MEMORY in this case simply tickles a bug in the firmware that causes it to corrupt the memory attributes table. The fact that cold boot behaves differently is a strong indicator here.
I didn't see the results of the memory attribute table dumps on the bugzilla thread, but dumping this table from EFI is not very useful because it will get regenerated/updated at ExitBootServices() time. Unfortunately, that also takes away the console so capturing the state of that table before the EFI stub boots the kernel is not an easy thing to do.
Quoted 3 lines
Is the memattr table completely corrupted? It also has a version field, and only versions 1 and 2 are defined so we might use that to detect corruption.

Quoted 8 lines

So from a today test:
https://bugzilla.suse.com/attachment.cgi?id=878296
Quoted 1 line
efi: memattr: efi_memattr_init: tab=0x7752f018 ver=1
size=16+2*1705287680=0x00000000cb494010
version is NOT corrupted :).

OK, so the struct looks like this

typedef struct {
        u32 version;
        u32 num_entries;
        u32 desc_size;
        u32 flags;
        efi_memory_desc_t entry[];
} efi_memory_attributes_table_t;

and in the correct case, num_entries == 45 and desc_size == 48.

It is quite easy to sanity check this structure: desc_size should be equal to the desc_size in the memory map, and num_entries can never exceed 2x the number of entries in the EFI memory map.

I'll go and implement something that performs the check right after ExitBootServices(), and just drops the table if it is bogus (it isn't that important anyway)

Usama Arif

2024-10-25 13:27 UTC #10 lore raw

In reply to Jiri Slaby

Quoted 60 lines · Jiri Slaby

Quoted 59 lines · Jiri Slaby
Quoted 44 lines · Jiri Slaby
Quoted 30 lines · Ard Biesheuvel
From: Ard Biesheuvel <[email protected]>
The TPM event log table is a Linux specific construct, where the data produced by the GetEventLog() boot service is cached in memory, and passed on to the OS using a EFI configuration table.
The use of EFI_LOADER_DATA here results in the region being left unreserved in the E820 memory map constructed by the EFI stub, and this is the memory description that is passed on to the incoming kernel by kexec, which is therefore unaware that the region should be reserved.
Even though the utility of the TPM2 event log after a kexec is questionable, any corruption might send the parsing code off into the weeds and crash the kernel. So let's use EFI_ACPI_RECLAIM_MEMORY instead, which is always treated as reserved by the E820 conversion logic.
Cc <[email protected]>
Reported-by Breno Leitao <[email protected]>
Tested-by Usama Arif <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
drivers/firmware/efi/libstub/tpm.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
drivers/firmware/efi/libstub/tpm.c +0 -0
index df3182f2e63a..1fd6823248ab 100644
drivers/firmware/efi/libstub/tpm.c static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca · line 99
Quoted 5 lines
drivers/firmware/efi/libstub/tpm.c +1 -1
@@ -96,7 +96,7 @@ static void efi_retrieve_tcg2_eventlog(int version, efi_physical_addr_t log_loca
       }
       /* Allocate space for the logs and copy them. */
-    status = efi_bs_call(allocate_pool, EFI_LOADER_DATA,
+    status = efi_bs_call(allocate_pool, EFI_ACPI_RECLAIM_MEMORY,
                    sizeof(*log_tbl) + log_size, (void **)&log_tbl);
Hi,
this, for some reason, corrupts system configuration table. On good boots, memattr points to 0x77535018, on bad boots (this commit applied), it points to 0x77526018.
And the good content at 0x77526018:
tab=0x77526018 size=16+45*48=0x0000000000000880
Quoted 6 lines
bad content at 0x77535018:
tab=0x77535018 size=16+2*1705353216=0x00000000cb4b4010
This happens only on cold boots. Subsequent boots (having the commit or not) are all fine.
Any ideas?
====
EFI_ACPI_RECLAIM_MEMORY
This memory is to be preserved by the UEFI OS loader and OS until ACPI
is enabled. Once ACPI is enabled, the memory in this range is available for general use.
====

Quoted 13 lines

Quoted 3 lines
BTW doesn't the above mean it is released by the time TPM actually reads it?
Isn't the proper fix to actually memblock_reserve() that TPM portion. The same as memattr in efi_memattr_init()?
And this is actually done in efi_tpm_eventlog_init().
Quoted 6 lines
Quoted 6 lines
DMI: Dell Inc. Latitude 7290/09386V, BIOS 1.39.0 07/04/2024
This was reported downstream at:
https://bugzilla.suse.com/show_bug.cgi?id=1231465
thanks,

Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.

Thanks,
Usama

Jiri Slaby

2024-10-30 05:25 UTC #11 lore raw

Quoted 2 lines · Usama Arif

Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.

It's shared in the aforementioned bug [1] already.

6.11.2 dmesg (bad run):
https://bugzilla.suse.com/attachment.cgi?id=877874

6.12-rc2 dmesg (good run):
https://bugzilla.suse.com/attachment.cgi?id=877887

FWIW from https://bugzilla.suse.com/attachment.cgi?id=878051:
good TPMEventLog=0x682aa018
bad TPMEventLog=0x65a6b018

[1] https://bugzilla.suse.com/show_bug.cgi?id=1231465

wdiff of e820:
wdiff -n bad good |colordiff
BIOS-e820: [mem 0x0000000000000000-0x0000000000057fff] usable
BIOS-e820: [mem 0x0000000000058000-0x0000000000058fff] reserved
BIOS-e820: [mem 0x0000000000059000-0x000000000009efff] usable
BIOS-e820: [mem 0x000000000009f000-0x00000000000fffff] reserved
BIOS-e820: [mem [-0x0000000000100000-0x0000000065a6efff]-] {+0x0000000000100000-0x00000000682abfff]+} usable
BIOS-e820: [mem [-0x0000000065a6f000-0x0000000065a7dfff]-] {+0x00000000682ac000-0x00000000682bafff]+} ACPI data
BIOS-e820: [mem [-0x0000000065a7e000-0x000000006a5acfff]-] {+0x00000000682bb000-0x000000006a5acfff]+} usable
BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
BIOS-e820: [mem 0x000000006a5ae000-0x000000006a5aefff] reserved
BIOS-e820: [mem 0x000000006a5af000-0x0000000079e83fff] usable
BIOS-e820: [mem 0x0000000079e84000-0x000000007a246fff] reserved
BIOS-e820: [mem 0x000000007a247000-0x000000007a28efff] ACPI data
BIOS-e820: [mem 0x000000007a28f000-0x000000007abf0fff] ACPI NVS
BIOS-e820: [mem 0x000000007abf1000-0x000000007b5fefff] reserved
BIOS-e820: [mem 0x000000007b5ff000-0x000000007b5fffff] usable
BIOS-e820: [mem 0x000000007b600000-0x000000007f7fffff] reserved
BIOS-e820: [mem 0x00000000f0000000-0x00000000f7ffffff] reserved
BIOS-e820: [mem 0x00000000fe000000-0x00000000fe010fff] reserved
BIOS-e820: [mem 0x00000000fec00000-0x00000000fec00fff] reserved
BIOS-e820: [mem 0x00000000fee00000-0x00000000fee00fff] reserved
BIOS-e820: [mem 0x00000000ff000000-0x00000000ffffffff] reserved
BIOS-e820: [mem 0x0000000100000000-0x000000087e7fffff] usable
NX (Execute Disable) protection: active
APIC: Static calls initialized
e820: update [mem [-0x65a5e018-0x65a6e457]-] {+0x6829b018-0x682ab457]+} usable ==> usable
extended physical RAM map:
reserve setup_data: [mem 0x0000000000000000-0x0000000000057fff] usable
reserve setup_data: [mem 0x0000000000058000-0x0000000000058fff] reserved
reserve setup_data: [mem 0x0000000000059000-0x000000000009efff] usable
reserve setup_data: [mem 0x000000000009f000-0x00000000000fffff] reserved
reserve setup_data: [mem [-0x0000000000100000-0x0000000065a5e017]-] {+0x0000000000100000-0x000000006829b017]+} usable
reserve setup_data: [mem [-0x0000000065a5e018-0x0000000065a6e457]-] {+0x000000006829b018-0x00000000682ab457]+} usable
reserve setup_data: [mem [-0x0000000065a6e458-0x0000000065a6efff]-] {+0x00000000682ab458-0x00000000682abfff]+} usable
reserve setup_data: [mem [-0x0000000065a6f000-0x0000000065a7dfff]-] {+0x00000000682ac000-0x00000000682bafff]+} ACPI data
reserve setup_data: [mem [-0x0000000065a7e000-0x000000006a5acfff]-] {+0x00000000682bb000-0x000000006a5acfff]+} usable
reserve setup_data: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
reserve setup_data: [mem 0x000000006a5ae000-0x000000006a5aefff] reserved
reserve setup_data: [mem 0x000000006a5af000-0x0000000079e83fff] usable
reserve setup_data: [mem 0x0000000079e84000-0x000000007a246fff] reserved
reserve setup_data: [mem 0x000000007a247000-0x000000007a28efff] ACPI data
reserve setup_data: [mem 0x000000007a28f000-0x000000007abf0fff] ACPI NVS
reserve setup_data: [mem 0x000000007abf1000-0x000000007b5fefff] reserved
reserve setup_data: [mem 0x000000007b5ff000-0x000000007b5fffff] usable
reserve setup_data: [mem 0x000000007b600000-0x000000007f7fffff] reserved
reserve setup_data: [mem 0x00000000f0000000-0x00000000f7ffffff] reserved
reserve setup_data: [mem 0x00000000fe000000-0x00000000fe010fff] reserved
reserve setup_data: [mem 0x00000000fec00000-0x00000000fec00fff] reserved
reserve setup_data: [mem 0x00000000fee00000-0x00000000fee00fff] reserved
reserve setup_data: [mem 0x00000000ff000000-0x00000000ffffffff] reserved
reserve setup_data: [mem 0x0000000100000000-0x000000087e7fffff] usable
efi: EFI v2.6 by American Megatrends
efi: ACPI=0x7a255000 ACPI 2.0=0x7a255000 SMBIOS=0x7b140000 SMBIOS 3.0=0x7b13f000 TPMFinalLog=0x7a892000 ESRT=0x7b0deb18 [-MEMATTR=0x77535018-] {+MEMATTR=0x77526018+} MOKvar=0x7b13e000 RNG=0x7a254018 [-TPMEventLog=0x65a6f018-] {+TPMEventLog=0x682ac018+}

thanks,

Signature · 2 lines

js
suse labs

Usama Arif

2024-10-30 17:13 UTC #12 lore raw

Quoted 74 lines · Jiri Slaby

Quoted 2 lines · Usama Arif
Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.
It's shared in the aforementioned bug [1] already.
6.11.2 dmesg (bad run):
https://bugzilla.suse.com/attachment.cgi?id=877874
6.12-rc2 dmesg (good run):
https://bugzilla.suse.com/attachment.cgi?id=877887
FWIW from https://bugzilla.suse.com/attachment.cgi?id=878051:
good TPMEventLog=0x682aa018
bad TPMEventLog=0x65a6b018
[1] https://bugzilla.suse.com/show_bug.cgi?id=1231465
wdiff of e820:
wdiff -n bad good |colordiff
BIOS-e820: [mem 0x0000000000000000-0x0000000000057fff] usable
BIOS-e820: [mem 0x0000000000058000-0x0000000000058fff] reserved
BIOS-e820: [mem 0x0000000000059000-0x000000000009efff] usable
BIOS-e820: [mem 0x000000000009f000-0x00000000000fffff] reserved
BIOS-e820: [mem [-0x0000000000100000-0x0000000065a6efff]-] {+0x0000000000100000-0x00000000682abfff]+} usable
BIOS-e820: [mem [-0x0000000065a6f000-0x0000000065a7dfff]-] {+0x00000000682ac000-0x00000000682bafff]+} ACPI data
BIOS-e820: [mem [-0x0000000065a7e000-0x000000006a5acfff]-] {+0x00000000682bb000-0x000000006a5acfff]+} usable
BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
BIOS-e820: [mem 0x000000006a5ae000-0x000000006a5aefff] reserved
BIOS-e820: [mem 0x000000006a5af000-0x0000000079e83fff] usable
BIOS-e820: [mem 0x0000000079e84000-0x000000007a246fff] reserved
BIOS-e820: [mem 0x000000007a247000-0x000000007a28efff] ACPI data
BIOS-e820: [mem 0x000000007a28f000-0x000000007abf0fff] ACPI NVS
BIOS-e820: [mem 0x000000007abf1000-0x000000007b5fefff] reserved
BIOS-e820: [mem 0x000000007b5ff000-0x000000007b5fffff] usable
BIOS-e820: [mem 0x000000007b600000-0x000000007f7fffff] reserved
BIOS-e820: [mem 0x00000000f0000000-0x00000000f7ffffff] reserved
BIOS-e820: [mem 0x00000000fe000000-0x00000000fe010fff] reserved
BIOS-e820: [mem 0x00000000fec00000-0x00000000fec00fff] reserved
BIOS-e820: [mem 0x00000000fee00000-0x00000000fee00fff] reserved
BIOS-e820: [mem 0x00000000ff000000-0x00000000ffffffff] reserved
BIOS-e820: [mem 0x0000000100000000-0x000000087e7fffff] usable
NX (Execute Disable) protection: active
APIC: Static calls initialized
e820: update [mem [-0x65a5e018-0x65a6e457]-] {+0x6829b018-0x682ab457]+} usable ==> usable
extended physical RAM map:
reserve setup_data: [mem 0x0000000000000000-0x0000000000057fff] usable
reserve setup_data: [mem 0x0000000000058000-0x0000000000058fff] reserved
reserve setup_data: [mem 0x0000000000059000-0x000000000009efff] usable
reserve setup_data: [mem 0x000000000009f000-0x00000000000fffff] reserved
reserve setup_data: [mem [-0x0000000000100000-0x0000000065a5e017]-] {+0x0000000000100000-0x000000006829b017]+} usable
reserve setup_data: [mem [-0x0000000065a5e018-0x0000000065a6e457]-] {+0x000000006829b018-0x00000000682ab457]+} usable
reserve setup_data: [mem [-0x0000000065a6e458-0x0000000065a6efff]-] {+0x00000000682ab458-0x00000000682abfff]+} usable
reserve setup_data: [mem [-0x0000000065a6f000-0x0000000065a7dfff]-] {+0x00000000682ac000-0x00000000682bafff]+} ACPI data
reserve setup_data: [mem [-0x0000000065a7e000-0x000000006a5acfff]-] {+0x00000000682bb000-0x000000006a5acfff]+} usable
reserve setup_data: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
reserve setup_data: [mem 0x000000006a5ae000-0x000000006a5aefff] reserved
reserve setup_data: [mem 0x000000006a5af000-0x0000000079e83fff] usable
reserve setup_data: [mem 0x0000000079e84000-0x000000007a246fff] reserved
reserve setup_data: [mem 0x000000007a247000-0x000000007a28efff] ACPI data
reserve setup_data: [mem 0x000000007a28f000-0x000000007abf0fff] ACPI NVS
reserve setup_data: [mem 0x000000007abf1000-0x000000007b5fefff] reserved
reserve setup_data: [mem 0x000000007b5ff000-0x000000007b5fffff] usable
reserve setup_data: [mem 0x000000007b600000-0x000000007f7fffff] reserved
reserve setup_data: [mem 0x00000000f0000000-0x00000000f7ffffff] reserved
reserve setup_data: [mem 0x00000000fe000000-0x00000000fe010fff] reserved
reserve setup_data: [mem 0x00000000fec00000-0x00000000fec00fff] reserved
reserve setup_data: [mem 0x00000000fee00000-0x00000000fee00fff] reserved
reserve setup_data: [mem 0x00000000ff000000-0x00000000ffffffff] reserved
reserve setup_data: [mem 0x0000000100000000-0x000000087e7fffff] usable
efi: EFI v2.6 by American Megatrends
efi: ACPI=0x7a255000 ACPI 2.0=0x7a255000 SMBIOS=0x7b140000 SMBIOS 3.0=0x7b13f000 TPMFinalLog=0x7a892000 ESRT=0x7b0deb18 [-MEMATTR=0x77535018-] {+MEMATTR=0x77526018+} MOKvar=0x7b13e000 RNG=0x7a254018 [-TPMEventLog=0x65a6f018-] {+TPMEventLog=0x682ac018+}

Quoted 1 line

thanks,

Thanks for sharing this.

This looks a bit weird for me.

The issue this patch was trying to fix was TPMEventLog being overwritten during kexec. We are using efi libstub.
Without this patch we would see
BIOS-e820: [mem 0x0000000000100000-0x0000000064763fff] usable TPMEventLog=0x5ed47018
i.e. TPMEventLog was usable memory and therefore was prone to corruption during kexec.

With this patch
BIOS-e820: [mem 0x00000000a8c01000-0x00000000a8cebfff] ACPI data TPMEventLog=0xa8ca8018
i.e. TPMEventLog is reserved as ACPI data, hence cant be corrupted during kexec.

In your case, from the logs you shared, good run without the patch:

[    0.000000] [      T0] BIOS-e820: [mem 0x0000000065a6f000-0x0000000065a7dfff] ACPI data
[    0.000000] [      T0] BIOS-e820: [mem 0x0000000065a7e000-0x000000006a5acfff] usable
[    0.000000] [      T0] BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
TPMEventLog=0x65a6f018 
bad run with the patch:
[    0.000000] [      T0] BIOS-e820: [mem 0x00000000682ac000-0x00000000682bafff] ACPI data
[    0.000000] [      T0] BIOS-e820: [mem 0x00000000682bb000-0x000000006a5acfff] usable
[    0.000000] [      T0] BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS

TPMEventLog=0x682ac018
Both with and without the fix, the TPMEventLog is part of ACPI data.

It means your firmware has already marked that area as ACPI data. Are you using efi/libstub?

Thanks,
Usama

Gregory Price

2024-10-30 18:02 UTC #13 lore raw

Quoted 21 lines · Usama Arif

Quoted 18 lines · Jiri Slaby
Quoted 2 lines · Usama Arif
Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.
It's shared in the aforementioned bug [1] already.
6.11.2 dmesg (bad run):
https://bugzilla.suse.com/attachment.cgi?id=877874
6.12-rc2 dmesg (good run):
https://bugzilla.suse.com/attachment.cgi?id=877887
FWIW from https://bugzilla.suse.com/attachment.cgi?id=878051:
good TPMEventLog=0x682aa018
bad TPMEventLog=0x65a6b018
[1] https://bugzilla.suse.com/show_bug.cgi?id=1231465

... snip ...

Quoted 32 lines

Quoted 5 lines
efi: EFI v2.6 by American Megatrends
efi: ACPI=0x7a255000 ACPI 2.0=0x7a255000 SMBIOS=0x7b140000 SMBIOS 3.0=0x7b13f000 TPMFinalLog=0x7a892000 ESRT=0x7b0deb18 [-MEMATTR=0x77535018-] {+MEMATTR=0x77526018+} MOKvar=0x7b13e000 RNG=0x7a254018 [-TPMEventLog=0x65a6f018-] {+TPMEventLog=0x682ac018+}
thanks,
Thanks for sharing this.
This looks a bit weird for me.
The issue this patch was trying to fix was TPMEventLog being overwritten during kexec. We are using efi libstub.
Without this patch we would see
BIOS-e820: [mem 0x0000000000100000-0x0000000064763fff] usable TPMEventLog=0x5ed47018
i.e. TPMEventLog was usable memory and therefore was prone to corruption during kexec.
With this patch
BIOS-e820: [mem 0x00000000a8c01000-0x00000000a8cebfff] ACPI data TPMEventLog=0xa8ca8018
i.e. TPMEventLog is reserved as ACPI data, hence cant be corrupted during kexec.
In your case, from the logs you shared, good run without the patch:
[    0.000000] [      T0] BIOS-e820: [mem 0x0000000065a6f000-0x0000000065a7dfff] ACPI data
[    0.000000] [      T0] BIOS-e820: [mem 0x0000000065a7e000-0x000000006a5acfff] usable
[    0.000000] [      T0] BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
TPMEventLog=0x65a6f018 
bad run with the patch:
[    0.000000] [      T0] BIOS-e820: [mem 0x00000000682ac000-0x00000000682bafff] ACPI data
[    0.000000] [      T0] BIOS-e820: [mem 0x00000000682bb000-0x000000006a5acfff] usable
[    0.000000] [      T0] BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS

Quoted 3 lines

TPMEventLog=0x682ac018
Both with and without the fix, the TPMEventLog is part of ACPI data.

Just wondering - why would the TPM log move a total of ~40MB between COLD boots.

I would expect this location to be relatively fixed (give or take a small amount of memory) - especially since it's so early in boot.

Quoted 8 lines

It means your firmware has already marked that area as ACPI data. Are you using efi/libstub?
Thanks,
Usama

Usama Arif

2024-10-30 18:24 UTC #14 lore raw

Quoted 59 lines · Gregory Price

Quoted 21 lines · Usama Arif
Quoted 18 lines · Jiri Slaby
Quoted 2 lines · Usama Arif
Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.
It's shared in the aforementioned bug [1] already.
6.11.2 dmesg (bad run):
https://bugzilla.suse.com/attachment.cgi?id=877874
6.12-rc2 dmesg (good run):
https://bugzilla.suse.com/attachment.cgi?id=877887
FWIW from https://bugzilla.suse.com/attachment.cgi?id=878051:
good TPMEventLog=0x682aa018
bad TPMEventLog=0x65a6b018
[1] https://bugzilla.suse.com/show_bug.cgi?id=1231465
... snip ...
Quoted 32 lines
Quoted 5 lines
efi: EFI v2.6 by American Megatrends
efi: ACPI=0x7a255000 ACPI 2.0=0x7a255000 SMBIOS=0x7b140000 SMBIOS 3.0=0x7b13f000 TPMFinalLog=0x7a892000 ESRT=0x7b0deb18 [-MEMATTR=0x77535018-] {+MEMATTR=0x77526018+} MOKvar=0x7b13e000 RNG=0x7a254018 [-TPMEventLog=0x65a6f018-] {+TPMEventLog=0x682ac018+}
thanks,
Thanks for sharing this.
This looks a bit weird for me.
The issue this patch was trying to fix was TPMEventLog being overwritten during kexec. We are using efi libstub.
Without this patch we would see
BIOS-e820: [mem 0x0000000000100000-0x0000000064763fff] usable TPMEventLog=0x5ed47018
i.e. TPMEventLog was usable memory and therefore was prone to corruption during kexec.
With this patch
BIOS-e820: [mem 0x00000000a8c01000-0x00000000a8cebfff] ACPI data TPMEventLog=0xa8ca8018
i.e. TPMEventLog is reserved as ACPI data, hence cant be corrupted during kexec.
In your case, from the logs you shared, good run without the patch:
[    0.000000] [      T0] BIOS-e820: [mem 0x0000000065a6f000-0x0000000065a7dfff] ACPI data
[    0.000000] [      T0] BIOS-e820: [mem 0x0000000065a7e000-0x000000006a5acfff] usable
[    0.000000] [      T0] BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
TPMEventLog=0x65a6f018 
bad run with the patch:
[    0.000000] [      T0] BIOS-e820: [mem 0x00000000682ac000-0x00000000682bafff] ACPI data
[    0.000000] [      T0] BIOS-e820: [mem 0x00000000682bb000-0x000000006a5acfff] usable
[    0.000000] [      T0] BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
Quoted 3 lines
TPMEventLog=0x682ac018
Both with and without the fix, the TPMEventLog is part of ACPI data.

Quoted 5 lines

Just wondering - why would the TPM log move a total of ~40MB between COLD boots.
I would expect this location to be relatively fixed (give or take a small amount of memory) - especially since it's so early in boot.

This is a good point. I think if we suspect this patch, it would be good to compare dmesg of kernel cold boot with this patch vs the commit just before this patch, rather than comparing with 6.12.

Thanks

Quoted 8 lines

Quoted 8 lines
It means your firmware has already marked that area as ACPI data. Are you using efi/libstub?
Thanks,
Usama

Jiri Slaby

2024-10-31 08:38 UTC #15 lore raw

Quoted 3 lines · Usama Arif

This is a good point. I think if we suspect this patch, it would be good to compare dmesg of kernel cold boot with this patch vs the commit just before this patch, rather than comparing with 6.12.

Here it goes then:

dmesg for 2e6871a632a99d9 (bad -- patch applied):
https://bugzilla.suse.com/attachment.cgi?id=878297

dmesg for bc2846c7ab029f0 (good -- before the patch, i.e. 2e6871a632a99d9^):
https://bugzilla.suse.com/attachment.cgi?id=878298

(6.11-stable SHAs ^^^)

$ wdiff -n good bad |colordiff
BIOS-e820: [mem 0x0000000000000000-0x0000000000057fff] usable
BIOS-e820: [mem 0x0000000000058000-0x0000000000058fff] reserved
BIOS-e820: [mem 0x0000000000059000-0x000000000009efff] usable
BIOS-e820: [mem 0x000000000009f000-0x00000000000fffff] reserved
BIOS-e820: [mem [-0x0000000000100000-0x000000006a5acfff]-] {+0x0000000000100000-0x0000000066a17fff] usable+}
{+BIOS-e820: [mem 0x0000000066a18000-0x0000000066a27fff] ACPI data+}
{+BIOS-e820: [mem 0x0000000066a28000-0x000000006a5acfff]+} usable
BIOS-e820: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
BIOS-e820: [mem 0x000000006a5ae000-0x000000006a5aefff] reserved
BIOS-e820: [mem 0x000000006a5af000-0x0000000079e83fff] usable
BIOS-e820: [mem 0x0000000079e84000-0x000000007a246fff] reserved
BIOS-e820: [mem 0x000000007a247000-0x000000007a28efff] ACPI data
BIOS-e820: [mem 0x000000007a28f000-0x000000007abf0fff] ACPI NVS
BIOS-e820: [mem 0x000000007abf1000-0x000000007b5fefff] reserved
BIOS-e820: [mem 0x000000007b5ff000-0x000000007b5fffff] usable
BIOS-e820: [mem 0x000000007b600000-0x000000007f7fffff] reserved
BIOS-e820: [mem 0x00000000f0000000-0x00000000f7ffffff] reserved
BIOS-e820: [mem 0x00000000fe000000-0x00000000fe010fff] reserved
BIOS-e820: [mem 0x00000000fec00000-0x00000000fec00fff] reserved
BIOS-e820: [mem 0x00000000fee00000-0x00000000fee00fff] reserved
BIOS-e820: [mem 0x00000000ff000000-0x00000000ffffffff] reserved
BIOS-e820: [mem 0x0000000100000000-0x000000087e7fffff] usable
NX (Execute Disable) protection: active
APIC: Static calls initialized
e820: update [mem 0x66a07018-0x66a17457] usable ==> usable
extended physical RAM map:
reserve setup_data: [mem 0x0000000000000000-0x0000000000057fff] usable
reserve setup_data: [mem 0x0000000000058000-0x0000000000058fff] reserved
reserve setup_data: [mem 0x0000000000059000-0x000000000009efff] usable
reserve setup_data: [mem 0x000000000009f000-0x00000000000fffff] reserved
reserve setup_data: [mem 0x0000000000100000-0x0000000066a07017] usable
reserve setup_data: [mem 0x0000000066a07018-0x0000000066a17457] usable
reserve setup_data: [mem [-0x0000000066a17458-0x000000006a5acfff]-] {+0x0000000066a17458-0x0000000066a17fff] usable+}
{+reserve setup_data: [mem 0x0000000066a18000-0x0000000066a27fff] ACPI data+}
{+reserve setup_data: [mem 0x0000000066a28000-0x000000006a5acfff]+} usable
reserve setup_data: [mem 0x000000006a5ad000-0x000000006a5adfff] ACPI NVS
reserve setup_data: [mem 0x000000006a5ae000-0x000000006a5aefff] reserved
reserve setup_data: [mem 0x000000006a5af000-0x0000000079e83fff] usable
reserve setup_data: [mem 0x0000000079e84000-0x000000007a246fff] reserved
reserve setup_data: [mem 0x000000007a247000-0x000000007a28efff] ACPI data
reserve setup_data: [mem 0x000000007a28f000-0x000000007abf0fff] ACPI NVS
reserve setup_data: [mem 0x000000007abf1000-0x000000007b5fefff] reserved
reserve setup_data: [mem 0x000000007b5ff000-0x000000007b5fffff] usable
reserve setup_data: [mem 0x000000007b600000-0x000000007f7fffff] reserved
reserve setup_data: [mem 0x00000000f0000000-0x00000000f7ffffff] reserved
reserve setup_data: [mem 0x00000000fe000000-0x00000000fe010fff] reserved
reserve setup_data: [mem 0x00000000fec00000-0x00000000fec00fff] reserved
reserve setup_data: [mem 0x00000000fee00000-0x00000000fee00fff] reserved
reserve setup_data: [mem 0x00000000ff000000-0x00000000ffffffff] reserved
reserve setup_data: [mem 0x0000000100000000-0x000000087e7fffff] usable
efi: EFI v2.6 by American Megatrends
efi: ACPI=0x7a255000 ACPI 2.0=0x7a255000 SMBIOS=0x7b140000 SMBIOS 3.0=0x7b13f000 TPMFinalLog=0x7a892000 ESRT=0x7b0deb18 [-MEMATTR=0x77536298-] {+MEMATTR=0x7752f018+} MOKvar=0x7b13e000 RNG=0x7a254018 TPMEventLog=0x66a18018

thanks,

Signature · 2 lines

js
suse labs

Gregory Price

2024-10-30 18:26 UTC #16 lore raw

In reply to Usama Arif

Quoted 7 lines · Usama Arif

Quoted 4 lines · Jiri Slaby
Quoted 2 lines · Usama Arif
Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.
efi: EFI v2.6 by American Megatrends

Tossing in another observation - the AMI EFI we've been working with has been

EFI v2.8 by American Megatrends
or
EFI v2.9 by American Megatrends

We have not seen this particular behavior (cold boot corruption issues) on top of these version. Might be worth investigating this issue.

you may also want to investigate this patch set:

https://lore.kernel.org/all/[email protected]/

which I believe would have caught your "eat all memory" sign extention issue.

This is queued up for v6.13 i think - but possibly 1/4 deserves a stable mark.

~Gregory

Ard Biesheuvel

maintainer

2024-10-30 19:43 UTC #17 lore raw

Quoted 19 lines · Gregory Price

Quoted 7 lines · Usama Arif
Quoted 4 lines · Jiri Slaby
Quoted 2 lines · Usama Arif
Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.
efi: EFI v2.6 by American Megatrends
Tossing in another observation - the AMI EFI we've been working with has been
EFI v2.8 by American Megatrends
or
EFI v2.9 by American Megatrends
We have not seen this particular behavior (cold boot corruption issues) on top of these version. Might be worth investigating this issue.

Quoted 8 lines

you may also want to investigate this patch set:
https://lore.kernel.org/all/[email protected]/
which I believe would have caught your "eat all memory" sign extention issue.
This is queued up for v6.13 i think - but possibly 1/4 deserves a stable mark.

To me, it does not seem obvious at all that the TPM code is the culprit here. The firmware produces a corrupted memory attributes table now that the EFI stub uses ACPI reclaim memory for the TPM event log, but to me, it smells like a firmware issue, not an issue in the EFI stub. (Pool allocations can trigger page allocations, affecting the layout of the EFI memory map).

So let's keep an open mind here, and not stare ourselves blind on the TPM event log code.

Gregory Price

2024-10-30 20:29 UTC #18 lore raw

Quoted 36 lines · Ard Biesheuvel

Quoted 19 lines · Gregory Price
Quoted 7 lines · Usama Arif
Quoted 4 lines · Jiri Slaby
Quoted 2 lines · Usama Arif
Could you share the e820 map, reserve setup_data and TPMEventLog address with and without the patch? All of these should be just be in the dmesg.
efi: EFI v2.6 by American Megatrends
Tossing in another observation - the AMI EFI we've been working with has been
EFI v2.8 by American Megatrends
or
EFI v2.9 by American Megatrends
We have not seen this particular behavior (cold boot corruption issues) on top of these version. Might be worth investigating this issue.
Quoted 8 lines
you may also want to investigate this patch set:
https://lore.kernel.org/all/[email protected]/
which I believe would have caught your "eat all memory" sign extention issue.
This is queued up for v6.13 i think - but possibly 1/4 deserves a stable mark.
To me, it does not seem obvious at all that the TPM code is the culprit here. The firmware produces a corrupted memory attributes table now that the EFI stub uses ACPI reclaim memory for the TPM event log, but to me, it smells like a firmware issue, not an issue in the EFI stub. (Pool allocations can trigger page allocations, affecting the layout of the EFI memory map).

Quoted 2 lines

So let's keep an open mind here, and not stare ourselves blind on the TPM event log code.

Agreed, just working backward a bit - this definitely feels like something is wrong in the state of Denm... firmware.

~Gregory

Jiri Slaby

2024-10-31 08:19 UTC #19 lore raw

In reply to Gregory Price

Quoted 5 lines · Gregory Price

you may also want to investigate this patch set:
https://lore.kernel.org/all/[email protected]/
which I believe would have caught your "eat all memory" sign extention issue.

But it's memattr what is corrupted, not tpmeventlog.

The reporter is using this for some time and it indeed works:

drivers/firmware/efi/memattr.c +1 -1

@@ -12,7 +12,7 @@
 
 #include <asm/early_ioremap.h>
 
-static int __initdata tbl_size;
+static phys_addr_t __initdata tbl_size;
 unsigned long __ro_after_init efi_mem_attr_table = EFI_INVALID_TABLE_ADDR;
 
 /*

Signature · 2 lines

js
suse labs