Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

PATCH Applied Ard Biesheuvel 4 participants 7 messages Feb 6, 2021 – Feb 8, 2021

Subsystems (2)

ACPI ACPI component architecture (acpica)

lore

Share & tools

lore

Ard Biesheuvel

author

2021-02-06 08:51 UTC #0 lore raw

PATCH 1/1 Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.

The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.

Cc Robert Moore <[email protected]>

Cc Erik Kaneda <[email protected]>

Cc "Rafael J. Wysocki" <[email protected]>

Cc Len Brown <[email protected]>

Reported-by Shawn Guo <[email protected]>

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

---
 drivers/acpi/acpica/nsrepair2.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

drivers/acpi/acpica/nsrepair2.c +13 -4

index d2c8d8279e7a..24c197d91f29 100644
@@ -495,8 +495,9 @@ acpi_ns_repair_HID(struct acpi_evaluate_info *info,
 		   union acpi_operand_object **return_object_ptr)
 {
 	union acpi_operand_object *return_object = *return_object_ptr;
-	char *dest;
+	union acpi_operand_object *new_string;
 	char *source;
+	char *dest;
 
 	ACPI_FUNCTION_NAME(ns_repair_HID);

@@ -517,6 +518,13 @@ acpi_ns_repair_HID(struct acpi_evaluate_info *info,
 		return_ACPI_STATUS(AE_OK);
 	}
 
+	/* It is simplest to always create a new string object */
+
+	new_string = acpi_ut_create_string_object(return_object->string.length);
+	if (!new_string) {
+		return_ACPI_STATUS(AE_NO_MEMORY);
+	}
+
 	/*
 	 * Remove a leading asterisk if present. For some unknown reason, there
 	 * are many machines in the field that contains IDs like this.

@@ -526,7 +534,7 @@ acpi_ns_repair_HID(struct acpi_evaluate_info *info,
 	source = return_object->string.pointer;
 	if (*source == '*') {
 		source++;
-		return_object->string.length--;
+		new_string->string.length--;
 
 		ACPI_DEBUG_PRINT((ACPI_DB_REPAIR,
 				  "%s: Removed invalid leading asterisk\n",

@@ -541,11 +549,12 @@ acpi_ns_repair_HID(struct acpi_evaluate_info *info,
 	 * "NNNN####" where N is an uppercase letter or decimal digit, and
 	 * # is a hex digit.
 	 */
-	for (dest = return_object->string.pointer; *source; dest++, source++) {
+	for (dest = new_string->string.pointer; *source; dest++, source++) {
 		*dest = (char)toupper((int)*source);
 	}
-	return_object->string.pointer[return_object->string.length] = 0;
 
+	acpi_ut_remove_reference(return_object);
+	*return_object_ptr = new_string;
 	return_ACPI_STATUS(AE_OK);
 }

Signature · 1 line

2.30.0

Shawn Guo

Tested-by on v1 1/1

2021-02-06 10:49 UTC #1 lore raw

Quoted 7 lines · Ard Biesheuvel

This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.

Quoted 6 lines

Cc Robert Moore <[email protected]>
Cc Erik Kaneda <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Reported-by Shawn Guo <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

Tested-by Shawn Guo <[email protected]>

Thanks for fixing the regression, Ard!

Shawn

Rafael J. Wysocki

maintainer Applied

2021-02-08 13:00 UTC #2 lore raw

Quoted 9 lines · Shawn Guo

Quoted 7 lines · Ard Biesheuvel
This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.

Quoted 8 lines

Quoted 6 lines
Cc Robert Moore <robert.moore(a)intel.com>
Cc Erik Kaneda <erik.kaneda(a)intel.com>
Cc "Rafael J. Wysocki" <rafael.j.wysocki(a)intel.com>
Cc Len Brown <lenb(a)kernel.org>
Reported-by Shawn Guo <shawn.guo(a)linaro.org>
Signed-off-by Ard Biesheuvel <ardb(a)kernel.org>
Tested-by Shawn Guo <shawn.guo(a)linaro.org>

Applied, thanks!

Erik, the upstream will need to sync up with this revert.

Kaneda, Erik

2021-02-08 19:08 UTC #3 lore raw

Quoted 16 lines

-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Monday, February 8, 2021 5:01 AM
To: Shawn Guo <[email protected]>; Ard Biesheuvel <[email protected]>; Kaneda, Erik <[email protected]>
Cc: Linux ARM <[email protected]>; ACPI Devel Maling List <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

Quoted 5 lines

Quoted 4 lines · Shawn Guo
Quoted 2 lines · Ard Biesheuvel
This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.

Hi Bob, Ard and Rafael,

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.

Since this code runs on basically every _HID and _CID invocation, I would have expected this kind of revert to come in for kernels that do not use initrd override... So it sounds like there is a difference between how pages are mapped for initrd table overrides and tables exposed through the XSDT for ARM.. I think it would be easier for us to make these fixes in the future if we could all come to a consensus on whether if we should assume that these pages are writable or not.

Should we assume that all ACPI tables are non-writable and read only regardless of initrd override and architecture?

Quoted 8 lines

Quoted 8 lines
Quoted 7 lines
Cc Robert Moore <[email protected]>
Cc Erik Kaneda <[email protected]>
Cc "Rafael J. Wysocki" <[email protected]>
Cc Len Brown <[email protected]>
Reported-by Shawn Guo <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

Quoted 5 lines

Quoted 1 line
Tested-by Shawn Guo <[email protected]>
Applied, thanks!
Erik, the upstream will need to sync up with this revert.

Ok sounds good.

Erik
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

author

2021-02-08 19:15 UTC #4 lore raw

Quoted 25 lines · Erik

Quoted 17 lines
-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Monday, February 8, 2021 5:01 AM
To: Shawn Guo <[email protected]>; Ard Biesheuvel <[email protected]>; Kaneda, Erik <[email protected]>
Cc: Linux ARM <[email protected]>; ACPI Devel Maling List <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"
Quoted 5 lines
Quoted 4 lines · Shawn Guo
Quoted 2 lines · Ard Biesheuvel
This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
Hi Bob, Ard and Rafael,

Quoted 9 lines

Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.
Since this code runs on basically every _HID and _CID invocation, I would have expected this kind of revert to come in for kernels that do not use initrd override... So it sounds like there is a difference between how pages are mapped for initrd table overrides and tables exposed through the XSDT for ARM.. I think it would be easier for us to make these fixes in the future if we could all come to a consensus on whether if we should assume that these pages are writable or not.
Should we assume that all ACPI tables are non-writable and read only regardless of initrd override and architecture?

ACPI tables are measured into the TPM on measured boot systems, and checksummed, so I don't think we should ever modify them in place.

But if we need code like this, it should be conditional at the very least, i.e., it should only rewrite _HIDs and _CIDs if they are incorrect to begin with.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Kaneda, Erik

2021-02-08 19:31 UTC #5 lore raw

Quoted 58 lines

-----Original Message-----
From: Ard Biesheuvel <[email protected]>
Sent: Monday, February 8, 2021 11:14 AM
To: Kaneda, Erik <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>; Shawn Guo <[email protected]>; Linux ARM <linux-arm- [email protected]>; ACPI Devel Maling List <linux- [email protected]>; Linux Kernel Mailing List <linux- [email protected]>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"
Quoted 15 lines · Erik
Quoted 15 lines
-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Monday, February 8, 2021 5:01 AM
To: Shawn Guo <[email protected]>; Ard Biesheuvel <[email protected]>; Kaneda, Erik <[email protected]>
Cc: Linux ARM <[email protected]>; ACPI Devel Maling List <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by
using
Quoted 3 lines
Quoted 3 lines
existing buffer"
On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <[email protected]>
Quoted 5 lines · wrote:
Quoted 4 lines
Quoted 4 lines · Ard Biesheuvel
Quoted 2 lines
This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
Quoted 8 lines
Hi Bob, Ard and Rafael,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.
Since this code runs on basically every _HID and _CID invocation, I would
have expected this kind of revert to come in for kernels that do not use initrd override... So it sounds like there is a difference between how pages are mapped for initrd table overrides and tables exposed through the XSDT for ARM.. I think it would be easier for us to make these fixes in the future if we could all come to a consensus on whether if we should assume that these pages are writable or not.
Quoted 1 line

Quoted 6 lines

Quoted 1 line
Should we assume that all ACPI tables are non-writable and read only
regardless of initrd override and architecture?
Quoted 1 line
ACPI tables are measured into the TPM on measured boot systems, and checksummed, so I don't think we should ever modify them in place.

I'm not knowledgeable on TPM but I'm curious - what happens when the TPM detects that these ACPI tables are modified?

Quoted 4 lines

But if we need code like this, it should be conditional at the very least, i.e., it should only rewrite _HIDs and _CIDs if they are incorrect to begin with.

I agree that this would be a more efficient approach

Erik
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Ard Biesheuvel

author

2021-02-08 19:48 UTC #6 lore raw

Quoted 59 lines · Erik

Quoted 59 lines
-----Original Message-----
From: Ard Biesheuvel <[email protected]>
Sent: Monday, February 8, 2021 11:14 AM
To: Kaneda, Erik <[email protected]>
Cc: Rafael J. Wysocki <[email protected]>; Shawn Guo <[email protected]>; Linux ARM <linux-arm- [email protected]>; ACPI Devel Maling List <linux- [email protected]>; Linux Kernel Mailing List <linux- [email protected]>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"
Quoted 15 lines · Erik
Quoted 15 lines
-----Original Message-----
From: Rafael J. Wysocki <[email protected]>
Sent: Monday, February 8, 2021 5:01 AM
To: Shawn Guo <[email protected]>; Ard Biesheuvel <[email protected]>; Kaneda, Erik <[email protected]>
Cc: Linux ARM <[email protected]>; ACPI Devel Maling List <[email protected]>; Linux Kernel Mailing List <linux- [email protected]>; open list:ACPI COMPONENT ARCHITECTURE (ACPICA) <[email protected]>; Wysocki, Rafael J <[email protected]>; Len Brown <[email protected]>; Moore, Robert <[email protected]>
Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by
using
Quoted 3 lines
Quoted 3 lines
existing buffer"
On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <[email protected]>
Quoted 5 lines · wrote:
Quoted 4 lines
Quoted 4 lines · Ard Biesheuvel
Quoted 2 lines
This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
Quoted 8 lines
Hi Bob, Ard and Rafael,
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
The 'exisitng buffer' in this case is the firmware provided table, and we should not modify that in place. This fixes a crash on arm64 with initrd table overrides, in which case the DSDT is not mapped with read/write permissions.
Since this code runs on basically every _HID and _CID invocation, I would
have expected this kind of revert to come in for kernels that do not use initrd override... So it sounds like there is a difference between how pages are mapped for initrd table overrides and tables exposed through the XSDT for ARM.. I think it would be easier for us to make these fixes in the future if we could all come to a consensus on whether if we should assume that these pages are writable or not.
Quoted 1 line

Quoted 9 lines

Quoted 6 lines
Quoted 1 line
Should we assume that all ACPI tables are non-writable and read only
regardless of initrd override and architecture?
Quoted 1 line
ACPI tables are measured into the TPM on measured boot systems, and checksummed, so I don't think we should ever modify them in place.
I'm not knowledgeable on TPM but I'm curious - what happens when the TPM detects that these ACPI tables are modified?

That depends on the policy implemented in user space. The TPM itself does not detect this change, but these ACPI tables will no longer match their SHA hashes in the TPM event log, and this is something we should really avoid.

Quoted 7 lines

Quoted 4 lines
But if we need code like this, it should be conditional at the very least, i.e., it should only rewrite _HIDs and _CIDs if they are incorrect to begin with.
I agree that this would be a more efficient approach

Indeed.

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel