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

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

[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.

Cc: Robert Moore <robert.moore@intel.com>
Cc: Erik Kaneda <erik.kaneda@intel.com>
Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Cc: Len Brown <lenb@kernel.org>
Reported-by: Shawn Guo <shawn.guo@linaro.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 drivers/acpi/acpica/nsrepair2.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/acpica/nsrepair2.c b/drivers/acpi/acpica/nsrepair2.c
index d2c8d8279e7a..24c197d91f29 100644
--- a/drivers/acpi/acpica/nsrepair2.c
+++ b/drivers/acpi/acpica/nsrepair2.c
@@ -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);
 }
 
-- 
2.30.0

Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

[Shawn Guo]

On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> 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 <robert.moore@intel.com>
> Cc: Erik Kaneda <erik.kaneda@intel.com>
> Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Reported-by: Shawn Guo <shawn.guo@linaro.org>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Tested-by: Shawn Guo <shawn.guo@linaro.org>

Thanks for fixing the regression, Ard!

Shawn

Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

[Rafael J. Wysocki]

On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <shawn.guo@linaro.org> wrote:
>
> On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > 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 <robert.moore@intel.com>
> > Cc: Erik Kaneda <erik.kaneda@intel.com>
> > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > Cc: Len Brown <lenb@kernel.org>
> > Reported-by: Shawn Guo <shawn.guo@linaro.org>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> Tested-by: Shawn Guo <shawn.guo@linaro.org>

Applied, thanks!

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

RE: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

[Kaneda, Erik]

> -----Original Message-----
> From: Rafael J. Wysocki <rafael@kernel.org>
> Sent: Monday, February 8, 2021 5:01 AM
> To: Shawn Guo <shawn.guo@linaro.org>; Ard Biesheuvel
> <ardb@kernel.org>; Kaneda, Erik <erik.kaneda@intel.com>
> Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
> List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA) <devel@acpica.org>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Moore,
> Robert <robert.moore@intel.com>
> Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using
> existing buffer"
> 
> On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> >
> > On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > > This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
> > >

Hi Bob, Ard and Rafael,

> > > 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?

> > >
> > > Cc: Robert Moore <robert.moore@intel.com>
> > > Cc: Erik Kaneda <erik.kaneda@intel.com>
> > > Cc: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> > > Cc: Len Brown <lenb@kernel.org>
> > > Reported-by: Shawn Guo <shawn.guo@linaro.org>
> > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> >
> > Tested-by: Shawn Guo <shawn.guo@linaro.org>
> 
> Applied, thanks!
> 
> Erik, the upstream will need to sync up with this revert.

Ok sounds good.

Erik

Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

[Ard Biesheuvel]

On Mon, 8 Feb 2021 at 20:07, Kaneda, Erik <erik.kaneda@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Rafael J. Wysocki <rafael@kernel.org>
> > Sent: Monday, February 8, 2021 5:01 AM
> > To: Shawn Guo <shawn.guo@linaro.org>; Ard Biesheuvel
> > <ardb@kernel.org>; Kaneda, Erik <erik.kaneda@intel.com>
> > Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
> > List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> > (ACPICA) <devel@acpica.org>; Wysocki, Rafael J
> > <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Moore,
> > Robert <robert.moore@intel.com>
> > Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using
> > existing buffer"
> >
> > On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <shawn.guo@linaro.org> wrote:
> > >
> > > On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > > > This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
> > > >
>
> Hi Bob, Ard and Rafael,
>
> > > > 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.

RE: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

[Kaneda, Erik]

> -----Original Message-----
> From: Ard Biesheuvel <ardb@kernel.org>
> Sent: Monday, February 8, 2021 11:14 AM
> To: Kaneda, Erik <erik.kaneda@intel.com>
> Cc: Rafael J. Wysocki <rafael@kernel.org>; Shawn Guo
> <shawn.guo@linaro.org>; Linux ARM <linux-arm-
> kernel@lists.infradead.org>; ACPI Devel Maling List <linux-
> acpi@vger.kernel.org>; Linux Kernel Mailing List <linux-
> kernel@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> (ACPICA) <devel@acpica.org>; Wysocki, Rafael J
> <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Moore,
> Robert <robert.moore@intel.com>
> Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using
> existing buffer"
> 
> On Mon, 8 Feb 2021 at 20:07, Kaneda, Erik <erik.kaneda@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: Rafael J. Wysocki <rafael@kernel.org>
> > > Sent: Monday, February 8, 2021 5:01 AM
> > > To: Shawn Guo <shawn.guo@linaro.org>; Ard Biesheuvel
> > > <ardb@kernel.org>; Kaneda, Erik <erik.kaneda@intel.com>
> > > Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
> > > List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > > kernel@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> > > (ACPICA) <devel@acpica.org>; Wysocki, Rafael J
> > > <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Moore,
> > > Robert <robert.moore@intel.com>
> > > Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by
> using
> > > existing buffer"
> > >
> > > On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <shawn.guo@linaro.org>
> wrote:
> > > >
> > > > On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > > > > This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
> > > > >
> >
> > Hi Bob, Ard and Rafael,
> >
> > > > > 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.

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

> 
> 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

Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using existing buffer"

[Ard Biesheuvel]

On Mon, 8 Feb 2021 at 20:30, Kaneda, Erik <erik.kaneda@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: Ard Biesheuvel <ardb@kernel.org>
> > Sent: Monday, February 8, 2021 11:14 AM
> > To: Kaneda, Erik <erik.kaneda@intel.com>
> > Cc: Rafael J. Wysocki <rafael@kernel.org>; Shawn Guo
> > <shawn.guo@linaro.org>; Linux ARM <linux-arm-
> > kernel@lists.infradead.org>; ACPI Devel Maling List <linux-
> > acpi@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > kernel@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> > (ACPICA) <devel@acpica.org>; Wysocki, Rafael J
> > <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Moore,
> > Robert <robert.moore@intel.com>
> > Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by using
> > existing buffer"
> >
> > On Mon, 8 Feb 2021 at 20:07, Kaneda, Erik <erik.kaneda@intel.com> wrote:
> > >
> > >
> > >
> > > > -----Original Message-----
> > > > From: Rafael J. Wysocki <rafael@kernel.org>
> > > > Sent: Monday, February 8, 2021 5:01 AM
> > > > To: Shawn Guo <shawn.guo@linaro.org>; Ard Biesheuvel
> > > > <ardb@kernel.org>; Kaneda, Erik <erik.kaneda@intel.com>
> > > > Cc: Linux ARM <linux-arm-kernel@lists.infradead.org>; ACPI Devel Maling
> > > > List <linux-acpi@vger.kernel.org>; Linux Kernel Mailing List <linux-
> > > > kernel@vger.kernel.org>; open list:ACPI COMPONENT ARCHITECTURE
> > > > (ACPICA) <devel@acpica.org>; Wysocki, Rafael J
> > > > <rafael.j.wysocki@intel.com>; Len Brown <lenb@kernel.org>; Moore,
> > > > Robert <robert.moore@intel.com>
> > > > Subject: Re: [PATCH] Revert "ACPICA: Interpreter: fix memory leak by
> > using
> > > > existing buffer"
> > > >
> > > > On Sat, Feb 6, 2021 at 11:49 AM Shawn Guo <shawn.guo@linaro.org>
> > wrote:
> > > > >
> > > > > On Sat, Feb 06, 2021 at 09:49:37AM +0100, Ard Biesheuvel wrote:
> > > > > > This reverts commit 32cf1a12cad43358e47dac8014379c2f33dfbed4.
> > > > > >
> > >
> > > Hi Bob, Ard and Rafael,
> > >
> > > > > > 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.
>
> 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.

> >
> > 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.