Linux ACPI
 help / color / mirror / Atom feed
* [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core
@ 2026-05-30  9:40 Marco Scardovi
  2026-05-30  9:40 ` [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler Marco Scardovi
                   ` (2 more replies)
  0 siblings, 3 replies; 11+ messages in thread
From: Marco Scardovi @ 2026-05-30  9:40 UTC (permalink / raw)
  To: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski
  Cc: linux-gpio, linux-acpi, linux-kernel, Marco Scardovi

Hi all,

While reviewing drivers/gpio/gpiolib-acpi-core.c in linux-next,
I noticed two bounds-checking issues in the ACPI GPIO handling paths.

The first issue is in acpi_gpio_adr_space_handler(), where the
64-bit ACPI OperationRegion address is truncated to u16 before
validation against pin_table_length. This can cause out-of-range
addresses to wrap around and access unintended GPIO entries.

Depending on platform firmware configuration, this could potentially
affect GPIO lines associated with sensitive hardware controls.

The second issue is in acpi_gpio_package_count(), where malformed
_DSD packages can cause pointer advancement past the package end
during element parsing, potentially resulting in out-of-bounds reads.

This series fixes both issues by:

- Validating the full address range before truncation
- Making the length calculation overflow-safe
- Validating remaining package elements before pointer advancement

The fixes are intentionally minimal and preserve the existing
behavior of capping ranges that extend past the end of the pin table.

Patch 1 also converts the related loop variables to unsigned types
for consistency with the updated arithmetic.

Thanks,
Marco

Marco Scardovi (2):
gpiolib: acpi: prevent address truncation in OperationRegion handler
gpiolib: acpi: fix out-of-bounds pointer arithmetic in
acpi_gpio_package_count

drivers/gpio/gpiolib-acpi-core.c | 19 +++++++++++++++----
1 file changed, 15 insertions(+), 4 deletions(-)

--
2.54.0


^ permalink raw reply	[flat|nested] 11+ messages in thread

* [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler
  2026-05-30  9:40 [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Marco Scardovi
@ 2026-05-30  9:40 ` Marco Scardovi
  2026-06-01  5:02   ` Mika Westerberg
  2026-05-30  9:40 ` [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count Marco Scardovi
  2026-06-02  7:52 ` [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Andy Shevchenko
  2 siblings, 1 reply; 11+ messages in thread
From: Marco Scardovi @ 2026-05-30  9:40 UTC (permalink / raw)
  To: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski
  Cc: linux-gpio, linux-acpi, linux-kernel, Marco Scardovi

The ACPI address space handler for GPIO OperationRegions receives the
pin offset as a 64-bit acpi_physical_address. However, the handler
truncates this address to a u16 pin_index before validating it.

If an ACPI table attempts to access a pin offset greater than 65535,
the truncation wraps the index around. This may result in accesses to
unintended GPIO pins.

Fix this by adding an explicit check to verify that the 64-bit address
is less than agpio->pin_table_length before assigning it to the u16
pin_index, returning AE_BAD_PARAMETER if it is out of bounds.
Additionally, make the length calculation overflow-safe and change the
types of length and loop counter to unsigned.

Signed-off-by: Marco Scardovi <scardracs@disroot.org>
---
 drivers/gpio/gpiolib-acpi-core.c | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index eb8a40cfb7a9..049e4cbc14ed 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -1087,10 +1087,10 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 	struct gpio_chip *chip = achip->chip;
 	struct acpi_resource_gpio *agpio;
 	struct acpi_resource *ares;
-	u16 pin_index = address;
+	unsigned int length;
 	acpi_status status;
-	int length;
-	int i;
+	unsigned int i;
+	u16 pin_index;
 
 	status = acpi_buffer_to_resource(achip->conn_info.connection,
 					 achip->conn_info.length, &ares);
@@ -1110,7 +1110,16 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		return AE_BAD_PARAMETER;
 	}
 
-	length = min(agpio->pin_table_length, pin_index + bits);
+	if (address >= agpio->pin_table_length) {
+		ACPI_FREE(ares);
+		return AE_BAD_PARAMETER;
+	}
+
+	pin_index = address;
+	if (bits > agpio->pin_table_length - pin_index)
+		length = agpio->pin_table_length;
+	else
+		length = pin_index + bits;
 	for (i = pin_index; i < length; ++i) {
 		unsigned int pin = agpio->pin_table[i];
 		struct acpi_gpio_connection *conn;
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count
  2026-05-30  9:40 [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Marco Scardovi
  2026-05-30  9:40 ` [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler Marco Scardovi
@ 2026-05-30  9:40 ` Marco Scardovi
  2026-06-01  5:17   ` Mika Westerberg
  2026-06-02  7:52 ` [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Andy Shevchenko
  2 siblings, 1 reply; 11+ messages in thread
From: Marco Scardovi @ 2026-05-30  9:40 UTC (permalink / raw)
  To: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski
  Cc: linux-gpio, linux-acpi, linux-kernel, Marco Scardovi

When counting GPIOs in an ACPI package, encountering a reference or
string causes the element pointer to be advanced by 3 (element += 3)
and then by 1 (element++).

If a malformed ACPI package contains fewer than 4 remaining elements
when a reference or string is processed, this pointer arithmetic
advances the element pointer past the end of the package elements
array. This results in undefined behavior and can cause out-of-bounds
reads.

Fix this by ensuring at least 4 elements remain in the package before
advancing the element pointer, returning -EPROTO if the package
structure is invalid.

Signed-off-by: Marco Scardovi <scardracs@disroot.org>
---
 drivers/gpio/gpiolib-acpi-core.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index 049e4cbc14ed..494dcd166aef 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -1310,6 +1310,8 @@ static int acpi_gpio_package_count(const union acpi_object *obj)
 		switch (element->type) {
 		case ACPI_TYPE_LOCAL_REFERENCE:
 		case ACPI_TYPE_STRING:
+			if (end - element < 4)
+				return -EPROTO;
 			element += 3;
 			fallthrough;
 		case ACPI_TYPE_INTEGER:
-- 
2.54.0


^ permalink raw reply related	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler
  2026-05-30  9:40 ` [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler Marco Scardovi
@ 2026-06-01  5:02   ` Mika Westerberg
  2026-06-01  6:31     ` Marco Scardovi
  0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2026-06-01  5:02 UTC (permalink / raw)
  To: Marco Scardovi
  Cc: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel

Hi,

On Sat, May 30, 2026 at 11:40:11AM +0200, Marco Scardovi wrote:
> The ACPI address space handler for GPIO OperationRegions receives the
> pin offset as a 64-bit acpi_physical_address. However, the handler
> truncates this address to a u16 pin_index before validating it.
> 
> If an ACPI table attempts to access a pin offset greater than 65535,
> the truncation wraps the index around. This may result in accesses to
> unintended GPIO pins.

If you look at the ACPI spec:

https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#connection-descriptors

the pin number is 2 bytes and 0xffff is defined as no connection. So the
firmware cannot really think that it can access GPIO outside of that range.

> Fix this by adding an explicit check to verify that the 64-bit address
> is less than agpio->pin_table_length before assigning it to the u16
> pin_index, returning AE_BAD_PARAMETER if it is out of bounds.
> Additionally, make the length calculation overflow-safe and change the
> types of length and loop counter to unsigned.
> 
> Signed-off-by: Marco Scardovi <scardracs@disroot.org>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 17 +++++++++++++----
>  1 file changed, 13 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index eb8a40cfb7a9..049e4cbc14ed 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -1087,10 +1087,10 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  	struct gpio_chip *chip = achip->chip;
>  	struct acpi_resource_gpio *agpio;
>  	struct acpi_resource *ares;
> -	u16 pin_index = address;
> +	unsigned int length;
>  	acpi_status status;
> -	int length;
> -	int i;
> +	unsigned int i;
> +	u16 pin_index;
>  
>  	status = acpi_buffer_to_resource(achip->conn_info.connection,
>  					 achip->conn_info.length, &ares);
> @@ -1110,7 +1110,16 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		return AE_BAD_PARAMETER;
>  	}
>  
> -	length = min(agpio->pin_table_length, pin_index + bits);
> +	if (address >= agpio->pin_table_length) {
> +		ACPI_FREE(ares);
> +		return AE_BAD_PARAMETER;
> +	}
> +
> +	pin_index = address;
> +	if (bits > agpio->pin_table_length - pin_index)
> +		length = agpio->pin_table_length;
> +	else
> +		length = pin_index + bits;
>  	for (i = pin_index; i < length; ++i) {
>  		unsigned int pin = agpio->pin_table[i];
>  		struct acpi_gpio_connection *conn;
> -- 
> 2.54.0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count
  2026-05-30  9:40 ` [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count Marco Scardovi
@ 2026-06-01  5:17   ` Mika Westerberg
  2026-06-01  6:31     ` Marco Scardovi
  0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2026-06-01  5:17 UTC (permalink / raw)
  To: Marco Scardovi
  Cc: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel

On Sat, May 30, 2026 at 11:40:12AM +0200, Marco Scardovi wrote:
> When counting GPIOs in an ACPI package, encountering a reference or
> string causes the element pointer to be advanced by 3 (element += 3)
> and then by 1 (element++).
> 
> If a malformed ACPI package contains fewer than 4 remaining elements
> when a reference or string is processed, this pointer arithmetic
> advances the element pointer past the end of the package elements
> array. This results in undefined behavior and can cause out-of-bounds
> reads.

How can it cause out-of-bounds reads? We increase "element" but the next
iteration checks that it is still inside "end" and it's never dereferenced.
Maybe I'm missing something?

> Fix this by ensuring at least 4 elements remain in the package before
> advancing the element pointer, returning -EPROTO if the package
> structure is invalid.
> 
> Signed-off-by: Marco Scardovi <scardracs@disroot.org>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index 049e4cbc14ed..494dcd166aef 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -1310,6 +1310,8 @@ static int acpi_gpio_package_count(const union acpi_object *obj)
>  		switch (element->type) {
>  		case ACPI_TYPE_LOCAL_REFERENCE:
>  		case ACPI_TYPE_STRING:
> +			if (end - element < 4)
> +				return -EPROTO;
>  			element += 3;
>  			fallthrough;
>  		case ACPI_TYPE_INTEGER:
> -- 
> 2.54.0

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler
  2026-06-01  5:02   ` Mika Westerberg
@ 2026-06-01  6:31     ` Marco Scardovi
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Scardovi @ 2026-06-01  6:31 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel

In data lunedì 1 giugno 2026 07:02:38 Ora legale dell’Europa centrale, Mika 
Westerberg ha scritto:
> Hi,
> 
> On Sat, May 30, 2026 at 11:40:11AM +0200, Marco Scardovi wrote:
> > The ACPI address space handler for GPIO OperationRegions receives the
> > pin offset as a 64-bit acpi_physical_address. However, the handler
> > truncates this address to a u16 pin_index before validating it.
> > 
> > If an ACPI table attempts to access a pin offset greater than 65535,
> > the truncation wraps the index around. This may result in accesses to
> > unintended GPIO pins.
> 
> If you look at the ACPI spec:
> 
> https://uefi.org/specs/ACPI/6.5/06_Device_Configuration.html#connection-desc
> riptors
> 
> the pin number is 2 bytes and 0xffff is defined as no connection. So the
> firmware cannot really think that it can access GPIO outside of that range.
> 
Hi Mika,

You are right that GPIO connection descriptors define pin indices as 16-bit
values and that 0xffff is reserved as "no connection".

My concern is that `address` is provided by ACPICA as a 64-bit
`acpi_physical_address`, while the GPIO driver immediately narrows it:

```
u16 pin_index = address;
```

At this point, the value is converted to the target type before any
validation against the GPIO resource is performed in the driver.

The intention of the change is to ensure that the GPIO layer validates the
value against its 16-bit domain before narrowing it, so that validation is
performed on the original value received from ACPICA rather than on a
truncated representation.

This keeps the type boundary explicit at the GPIO driver level, where the
semantic meaning of the value (GPIO pin index) is actually enforced.
>
> > Fix this by adding an explicit check to verify that the 64-bit address
> > is less than agpio->pin_table_length before assigning it to the u16
> > pin_index, returning AE_BAD_PARAMETER if it is out of bounds.
> > Additionally, make the length calculation overflow-safe and change the
> > types of length and loop counter to unsigned.
> > 
> > Signed-off-by: Marco Scardovi <scardracs@disroot.org>
> > ---
> > 
> >  drivers/gpio/gpiolib-acpi-core.c | 17 +++++++++++++----
> >  1 file changed, 13 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-acpi-core.c
> > b/drivers/gpio/gpiolib-acpi-core.c index eb8a40cfb7a9..049e4cbc14ed
> > 100644
> > --- a/drivers/gpio/gpiolib-acpi-core.c
> > +++ b/drivers/gpio/gpiolib-acpi-core.c
> > @@ -1087,10 +1087,10 @@ acpi_gpio_adr_space_handler(u32 function,
> > acpi_physical_address address,> 
> >  	struct gpio_chip *chip = achip->chip;
> >  	struct acpi_resource_gpio *agpio;
> >  	struct acpi_resource *ares;
> > 
> > -	u16 pin_index = address;
> > +	unsigned int length;
> > 
> >  	acpi_status status;
> > 
> > -	int length;
> > -	int i;
> > +	unsigned int i;
> > +	u16 pin_index;
> > 
> >  	status = acpi_buffer_to_resource(achip->conn_info.connection,
> >  	
> >  					 achip-
>conn_info.length, &ares);
> > 
> > @@ -1110,7 +1110,16 @@ acpi_gpio_adr_space_handler(u32 function,
> > acpi_physical_address address,> 
> >  		return AE_BAD_PARAMETER;
> >  	
> >  	}
> > 
> > -	length = min(agpio->pin_table_length, pin_index + bits);
> > +	if (address >= agpio->pin_table_length) {
> > +		ACPI_FREE(ares);
> > +		return AE_BAD_PARAMETER;
> > +	}
> > +
> > +	pin_index = address;
> > +	if (bits > agpio->pin_table_length - pin_index)
> > +		length = agpio->pin_table_length;
> > +	else
> > +		length = pin_index + bits;
> > 
> >  	for (i = pin_index; i < length; ++i) {
> >  	
> >  		unsigned int pin = agpio->pin_table[i];
> >  		struct acpi_gpio_connection *conn;





^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count
  2026-06-01  5:17   ` Mika Westerberg
@ 2026-06-01  6:31     ` Marco Scardovi
  2026-06-01  7:17       ` Mika Westerberg
  0 siblings, 1 reply; 11+ messages in thread
From: Marco Scardovi @ 2026-06-01  6:31 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel

In data lunedì 1 giugno 2026 07:17:35 Ora legale dell’Europa centrale, Mika 
Westerberg ha scritto:
> On Sat, May 30, 2026 at 11:40:12AM +0200, Marco Scardovi wrote:
> > When counting GPIOs in an ACPI package, encountering a reference or
> > string causes the element pointer to be advanced by 3 (element += 3)
> > and then by 1 (element++).
> > 
> > If a malformed ACPI package contains fewer than 4 remaining elements
> > when a reference or string is processed, this pointer arithmetic
> > advances the element pointer past the end of the package elements
> > array. This results in undefined behavior and can cause out-of-bounds
> > reads.
> 
> How can it cause out-of-bounds reads? We increase "element" but the next
> iteration checks that it is still inside "end" and it's never dereferenced.
> Maybe I'm missing something?
>
Hi Mika,

I agree that `element` is not dereferenced after the loop exits.

My main concern is the parser logic rather than the pointer arithmetic
itself.

A GPIO connection is defined to consist of 4 package elements
(a reference/string followed by 3 integers), but the loop condition only
checks whether at least one element remains:

```
element < end
```

As a result, a malformed package containing fewer than 4 remaining elements
can still be processed as if it were a complete GPIO entry. This can lead
to a GPIO connection being accounted for even though the descriptor is
structurally incomplete.

Such truncated descriptors should be rejected with `-EPROTO` rather than
being accepted as valid input.

Ensuring sufficient remaining elements before entering the loop also
guarantees that pointer arithmetic stays within the defined bounds of the
package, but the primary issue is the acceptance of incomplete GPIO entries.
>
> > Fix this by ensuring at least 4 elements remain in the package before
> > advancing the element pointer, returning -EPROTO if the package
> > structure is invalid.
> > 
> > Signed-off-by: Marco Scardovi <scardracs@disroot.org>
> > ---
> > 
> >  drivers/gpio/gpiolib-acpi-core.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/gpio/gpiolib-acpi-core.c
> > b/drivers/gpio/gpiolib-acpi-core.c index 049e4cbc14ed..494dcd166aef
> > 100644
> > --- a/drivers/gpio/gpiolib-acpi-core.c
> > +++ b/drivers/gpio/gpiolib-acpi-core.c
> > @@ -1310,6 +1310,8 @@ static int acpi_gpio_package_count(const union
> > acpi_object *obj)> 
> >  		switch (element->type) {
> >  		case ACPI_TYPE_LOCAL_REFERENCE:
> > 
> >  		case ACPI_TYPE_STRING:
> > +			if (end - element < 4)
> > +				return -EPROTO;
> > 
> >  			element += 3;
> >  			fallthrough;
> >  		
> >  		case ACPI_TYPE_INTEGER:





^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count
  2026-06-01  6:31     ` Marco Scardovi
@ 2026-06-01  7:17       ` Mika Westerberg
  2026-06-01  7:53         ` Marco Scardovi
  0 siblings, 1 reply; 11+ messages in thread
From: Mika Westerberg @ 2026-06-01  7:17 UTC (permalink / raw)
  To: Marco Scardovi
  Cc: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel

Hi,

On Mon, Jun 01, 2026 at 08:31:16AM +0200, Marco Scardovi wrote:
> In data lunedì 1 giugno 2026 07:17:35 Ora legale dell’Europa centrale, Mika 
> Westerberg ha scritto:
> > On Sat, May 30, 2026 at 11:40:12AM +0200, Marco Scardovi wrote:
> > > When counting GPIOs in an ACPI package, encountering a reference or
> > > string causes the element pointer to be advanced by 3 (element += 3)
> > > and then by 1 (element++).
> > > 
> > > If a malformed ACPI package contains fewer than 4 remaining elements
> > > when a reference or string is processed, this pointer arithmetic
> > > advances the element pointer past the end of the package elements
> > > array. This results in undefined behavior and can cause out-of-bounds
> > > reads.
> > 
> > How can it cause out-of-bounds reads? We increase "element" but the next
> > iteration checks that it is still inside "end" and it's never dereferenced.
> > Maybe I'm missing something?
> >
> Hi Mika,
> 
> I agree that `element` is not dereferenced after the loop exits.
> 
> My main concern is the parser logic rather than the pointer arithmetic
> itself.
> 
> A GPIO connection is defined to consist of 4 package elements
> (a reference/string followed by 3 integers), but the loop condition only
> checks whether at least one element remains:
> 
> ```
> element < end
> ```
> 
> As a result, a malformed package containing fewer than 4 remaining elements
> can still be processed as if it were a complete GPIO entry. This can lead
> to a GPIO connection being accounted for even though the descriptor is
> structurally incomplete.

Does it take into account that GPIOs are optional in some cases so this is
totally valid:

  Package () {
      "cs-gpios",
      Package () {
         ^GPIO, 19, 0, 0,
         ^GPIO, 20, 0, 0,
         0,
         ^GPIO, 21, 0, 0,
      }
  }

I'm worried that this breaks things rather than improves. If your intent is
to "harden" against malicios ACPI tables then there are much worse things
than this that can be done (e.g we run a full bytecode interpreter inside
the kernel with not much restrictions and all that bytecode comes from the
ACPI tables).

Have you verified this change against any system that actually calls this
function?

> Such truncated descriptors should be rejected with `-EPROTO` rather than
> being accepted as valid input.
> 
> Ensuring sufficient remaining elements before entering the loop also
> guarantees that pointer arithmetic stays within the defined bounds of the
> package, but the primary issue is the acceptance of incomplete GPIO entries.
> >
> > > Fix this by ensuring at least 4 elements remain in the package before
> > > advancing the element pointer, returning -EPROTO if the package
> > > structure is invalid.
> > > 
> > > Signed-off-by: Marco Scardovi <scardracs@disroot.org>
> > > ---
> > > 
> > >  drivers/gpio/gpiolib-acpi-core.c | 2 ++
> > >  1 file changed, 2 insertions(+)
> > > 
> > > diff --git a/drivers/gpio/gpiolib-acpi-core.c
> > > b/drivers/gpio/gpiolib-acpi-core.c index 049e4cbc14ed..494dcd166aef
> > > 100644
> > > --- a/drivers/gpio/gpiolib-acpi-core.c
> > > +++ b/drivers/gpio/gpiolib-acpi-core.c
> > > @@ -1310,6 +1310,8 @@ static int acpi_gpio_package_count(const union
> > > acpi_object *obj)> 
> > >  		switch (element->type) {
> > >  		case ACPI_TYPE_LOCAL_REFERENCE:
> > > 
> > >  		case ACPI_TYPE_STRING:
> > > +			if (end - element < 4)
> > > +				return -EPROTO;
> > > 
> > >  			element += 3;
> > >  			fallthrough;
> > >  		
> > >  		case ACPI_TYPE_INTEGER:
> 
> 
> 

^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count
  2026-06-01  7:17       ` Mika Westerberg
@ 2026-06-01  7:53         ` Marco Scardovi
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Scardovi @ 2026-06-01  7:53 UTC (permalink / raw)
  To: Mika Westerberg
  Cc: Mika Westerberg, Andy Shevchenko, Linus Walleij,
	Bartosz Golaszewski, linux-gpio, linux-acpi, linux-kernel

In data lunedì 1 giugno 2026 09:17:03 Ora legale dell’Europa centrale, Mika 
Westerberg ha scritto:
> Hi,
> 
> On Mon, Jun 01, 2026 at 08:31:16AM +0200, Marco Scardovi wrote:
> > In data lunedì 1 giugno 2026 07:17:35 Ora legale dell’Europa centrale,
> > Mika
> > 
> > Westerberg ha scritto:
> > > On Sat, May 30, 2026 at 11:40:12AM +0200, Marco Scardovi wrote:
> > > > When counting GPIOs in an ACPI package, encountering a reference or
> > > > string causes the element pointer to be advanced by 3 (element += 3)
> > > > and then by 1 (element++).
> > > > 
> > > > If a malformed ACPI package contains fewer than 4 remaining elements
> > > > when a reference or string is processed, this pointer arithmetic
> > > > advances the element pointer past the end of the package elements
> > > > array. This results in undefined behavior and can cause out-of-bounds
> > > > reads.
> > > 
> > > How can it cause out-of-bounds reads? We increase "element" but the next
> > > iteration checks that it is still inside "end" and it's never
> > > dereferenced.
> > > Maybe I'm missing something?
> > 
> > Hi Mika,
> > 
> > I agree that `element` is not dereferenced after the loop exits.
> > 
> > My main concern is the parser logic rather than the pointer arithmetic
> > itself.
> > 
> > A GPIO connection is defined to consist of 4 package elements
> > (a reference/string followed by 3 integers), but the loop condition only
> > checks whether at least one element remains:
> > 
> > ```
> > element < end
> > ```
> > 
> > As a result, a malformed package containing fewer than 4 remaining
> > elements
> > can still be processed as if it were a complete GPIO entry. This can lead
> > to a GPIO connection being accounted for even though the descriptor is
> > structurally incomplete.
> 
> Does it take into account that GPIOs are optional in some cases so this is
> totally valid:
> 
>   Package () {
>       "cs-gpios",
>       Package () {
>          ^GPIO, 19, 0, 0,
>          ^GPIO, 20, 0, 0,
>          0,
>          ^GPIO, 21, 0, 0,
>       }
>   }
> 
> I'm worried that this breaks things rather than improves. If your intent is
> to "harden" against malicios ACPI tables then there are much worse things
> than this that can be done (e.g we run a full bytecode interpreter inside
> the kernel with not much restrictions and all that bytecode comes from the
> ACPI tables).
> 
> Have you verified this change against any system that actually calls this
> function?
>
Hi Mika,

thanks for the detailed explanation — I think your point is
absolutely correct.

Looking again at the code and considering the valid ACPI cases you
mentioned (including optional GPIO entries and non-strict packages),
I realize my patch was too aggressive and based on an incorrect
assumption about the parsing invariants.

I was incorrectly assuming that each GPIO descriptor must always
consist of a strict 4-element sequence, which led me to interpret
partial or irregular packages as invalid.

I also tested the change only with virtme-ng, as I do not have
access to real hardware, but I agree this is not sufficient to
validate the behavior in real systems.

The check I proposed would likely reject valid real-world cases and
effectively change the semantics of the parser rather than fixing a
real issue.

Given this, I agree the change is not appropriate and I will drop the
patch.

Sorry for the noise, and thanks for the clarification.
Marco
> 
> > Such truncated descriptors should be rejected with `-EPROTO` rather than
> > being accepted as valid input.
> > 
> > Ensuring sufficient remaining elements before entering the loop also
> > guarantees that pointer arithmetic stays within the defined bounds of the
> > package, but the primary issue is the acceptance of incomplete GPIO
> > entries.> 
> > > > Fix this by ensuring at least 4 elements remain in the package before
> > > > advancing the element pointer, returning -EPROTO if the package
> > > > structure is invalid.
> > > > 
> > > > Signed-off-by: Marco Scardovi <scardracs@disroot.org>
> > > > ---
> > > > 
> > > >  drivers/gpio/gpiolib-acpi-core.c | 2 ++
> > > >  1 file changed, 2 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpio/gpiolib-acpi-core.c
> > > > b/drivers/gpio/gpiolib-acpi-core.c index 049e4cbc14ed..494dcd166aef
> > > > 100644
> > > > --- a/drivers/gpio/gpiolib-acpi-core.c
> > > > +++ b/drivers/gpio/gpiolib-acpi-core.c
> > > > @@ -1310,6 +1310,8 @@ static int acpi_gpio_package_count(const union
> > > > acpi_object *obj)>
> > > > 
> > > >  		switch (element->type) {
> > > >  		case ACPI_TYPE_LOCAL_REFERENCE:
> > > > 
> > > >  		case ACPI_TYPE_STRING:
> > > > +			if (end - element < 4)
> > > > +				return -EPROTO;
> > > > 
> > > >  			element += 3;
> > > >  			fallthrough;
> > > >  		
> > > >  		case ACPI_TYPE_INTEGER:





^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core
  2026-05-30  9:40 [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Marco Scardovi
  2026-05-30  9:40 ` [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler Marco Scardovi
  2026-05-30  9:40 ` [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count Marco Scardovi
@ 2026-06-02  7:52 ` Andy Shevchenko
  2026-06-02  7:59   ` Marco Scardovi
  2 siblings, 1 reply; 11+ messages in thread
From: Andy Shevchenko @ 2026-06-02  7:52 UTC (permalink / raw)
  To: Marco Scardovi
  Cc: Mika Westerberg, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-acpi, linux-kernel

On Sat, May 30, 2026 at 11:40:10AM +0200, Marco Scardovi wrote:

> While reviewing drivers/gpio/gpiolib-acpi-core.c in linux-next,
> I noticed two bounds-checking issues in the ACPI GPIO handling paths.
> 
> The first issue is in acpi_gpio_adr_space_handler(), where the
> 64-bit ACPI OperationRegion address is truncated to u16 before
> validation against pin_table_length. This can cause out-of-range
> addresses to wrap around and access unintended GPIO entries.
> 
> Depending on platform firmware configuration, this could potentially
> affect GPIO lines associated with sensitive hardware controls.
> 
> The second issue is in acpi_gpio_package_count(), where malformed
> _DSD packages can cause pointer advancement past the package end
> during element parsing, potentially resulting in out-of-bounds reads.
> 
> This series fixes both issues by:
> 
> - Validating the full address range before truncation
> - Making the length calculation overflow-safe
> - Validating remaining package elements before pointer advancement
> 
> The fixes are intentionally minimal and preserve the existing
> behavior of capping ranges that extend past the end of the pin table.
> 
> Patch 1 also converts the related loop variables to unsigned types
> for consistency with the updated arithmetic.

You got me lost. There was v3 of something, what is this?!

-- 
With Best Regards,
Andy Shevchenko



^ permalink raw reply	[flat|nested] 11+ messages in thread

* Re: [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core
  2026-06-02  7:52 ` [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Andy Shevchenko
@ 2026-06-02  7:59   ` Marco Scardovi
  0 siblings, 0 replies; 11+ messages in thread
From: Marco Scardovi @ 2026-06-02  7:59 UTC (permalink / raw)
  To: Andy Shevchenko
  Cc: Mika Westerberg, Linus Walleij, Bartosz Golaszewski, linux-gpio,
	linux-acpi, linux-kernel

In data martedì 2 giugno 2026 09:52:46 Ora legale dell’Europa centrale, Andy 
Shevchenko ha scritto:
> On Sat, May 30, 2026 at 11:40:10AM +0200, Marco Scardovi wrote:
> > While reviewing drivers/gpio/gpiolib-acpi-core.c in linux-next,
> > I noticed two bounds-checking issues in the ACPI GPIO handling paths.
> > 
> > The first issue is in acpi_gpio_adr_space_handler(), where the
> > 64-bit ACPI OperationRegion address is truncated to u16 before
> > validation against pin_table_length. This can cause out-of-range
> > addresses to wrap around and access unintended GPIO entries.
> > 
> > Depending on platform firmware configuration, this could potentially
> > affect GPIO lines associated with sensitive hardware controls.
> > 
> > The second issue is in acpi_gpio_package_count(), where malformed
> > _DSD packages can cause pointer advancement past the package end
> > during element parsing, potentially resulting in out-of-bounds reads.
> > 
> > This series fixes both issues by:
> > 
> > - Validating the full address range before truncation
> > - Making the length calculation overflow-safe
> > - Validating remaining package elements before pointer advancement
> > 
> > The fixes are intentionally minimal and preserve the existing
> > behavior of capping ranges that extend past the end of the pin table.
> > 
> > Patch 1 also converts the related loop variables to unsigned types
> > for consistency with the updated arithmetic.
> 
> You got me lost. There was v3 of something, what is this?!

Hi Andy,

yes I messed up the patches on my side and made way too noise for my own, and 
yours, peace of mind. Please don't consider this thread anymore.




^ permalink raw reply	[flat|nested] 11+ messages in thread

end of thread, other threads:[~2026-06-02  7:59 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-05-30  9:40 [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Marco Scardovi
2026-05-30  9:40 ` [PATCH 1/2] gpiolib: acpi: prevent address truncation in OperationRegion handler Marco Scardovi
2026-06-01  5:02   ` Mika Westerberg
2026-06-01  6:31     ` Marco Scardovi
2026-05-30  9:40 ` [PATCH 2/2] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count Marco Scardovi
2026-06-01  5:17   ` Mika Westerberg
2026-06-01  6:31     ` Marco Scardovi
2026-06-01  7:17       ` Mika Westerberg
2026-06-01  7:53         ` Marco Scardovi
2026-06-02  7:52 ` [PATCH 0/2] gpiolib: acpi: fix bounds-checking bugs in GPIO ACPI core Andy Shevchenko
2026-06-02  7:59   ` Marco Scardovi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox