[PATCH v4 0/4] gpiolib: acpi: Consolidated fixes and bounds checking

[PATCH v4 0/4] gpiolib: acpi: Consolidated fixes and bounds checking

[Marco Scardovi]

Hi all,
while reviewing my patches I ended up noticing these two were concurrent of
the same problem so I ended up merging them and opening a new thread to
avoid any confusion.

lore.kernel.org/linux-gpio/20260518075357.112584-1-mscardovi95@gmail.com/
lore.kernel.org/linux-gpio/20260524162708.62949-1-scardracs@disroot.org/

Please mind to check the 1st one before reviewing it as it contains some
of @Andy's comments on it.

This series consolidates various fixes and bounds-checking improvements
for gpiolib-acpi.

- Patch 1: Adds robust bounds checking for GPIO pin resource arrays.
- Patch 2: Fixes a resource leak and concurrent access race in the
  GPIO OperationRegion address space handler.
- Patch 3: Prevents physical address truncation from 64-bit to 16-bit
  in the OperationRegion handler.
- Patch 4: Prevents out-of-bounds pointer arithmetic in
  acpi_gpio_package_count when counting GPIOs.

Changes in v4:
- Merged the address truncation and package count pointer arithmetic
  bounds-checking fixes into this unified patch series.
- Cleaned up the OperationRegion handler modifications to avoid
  conflicts between the leak fix and truncation check.
- Added code documentation comments explaining locking requirements in
  acpi_gpiochip_find_conn().
- Reworded the commit subject line of Patch 2 to follow standard
  kernel conventions.
- Optimized the concurrent double-request rollback path in Patch 2 to
  free the descriptor outside the mutex (`conn_lock`) to prevent
  potential lockdep issues.
- Refined the `-EBUSY` recovery comment in Patch 2 to accurately
  characterize it as a best-effort recovery path.

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

Marco Scardovi (scardracs) (2):
  gpiolib: acpi: Add robust bounds-checking for GPIO pin resources
  gpiolib: acpi: fix resource leak in OpRegion

 drivers/gpio/gpiolib-acpi-core.c | 186 ++++++++++++++++++++++++-------
 1 file changed, 147 insertions(+), 39 deletions(-)

-- 
2.54.0

[PATCH v4 1/4] gpiolib: acpi: Add robust bounds-checking for GPIO pin resources

[Marco Scardovi]

From: "Marco Scardovi (scardracs)" <mscardovi95@gmail.com>

Ensure that the GPIO pin resource arrays are safely bounded before
accessing indices. Add bounds checking in acpi_request_own_gpiod(),
acpi_gpio_irq_is_wake(), and acpi_gpiochip_alloc_event() to prevent
out-of-bounds array reads if the ACPI namespace provides malformed or
empty pin tables.

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Marco Scardovi <mscardovi95@gmail.com>
---
 drivers/gpio/gpiolib-acpi-core.c | 24 ++++++++++++++++++++----
 1 file changed, 20 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index eb8a40cfb7a9..a6d78dad299e 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -320,10 +320,18 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 						unsigned int index,
 						const char *label)
 {
-	int polarity = GPIO_ACTIVE_HIGH;
-	enum gpiod_flags flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
-	unsigned int pin = agpio->pin_table[index];
 	struct gpio_desc *desc;
+	enum gpiod_flags flags;
+	unsigned int pin;
+	int polarity;
+
+	polarity = GPIO_ACTIVE_HIGH;
+	flags = acpi_gpio_to_gpiod_flags(agpio, polarity);
+
+	if (index >= agpio->pin_table_length)
+		return ERR_PTR(-EINVAL);
+
+	pin = agpio->pin_table[index];
 
 	desc = gpiochip_request_own_desc(chip, pin, label, polarity, flags);
 	if (IS_ERR(desc))
@@ -337,11 +345,16 @@ static struct gpio_desc *acpi_request_own_gpiod(struct gpio_chip *chip,
 static bool acpi_gpio_irq_is_wake(struct device *parent,
 				  const struct acpi_resource_gpio *agpio)
 {
-	unsigned int pin = agpio->pin_table[0];
+	unsigned int pin;
 
 	if (agpio->wake_capable != ACPI_WAKE_CAPABLE)
 		return false;
 
+	if (agpio->pin_table_length == 0)
+		return false;
+
+	pin = agpio->pin_table[0];
+
 	if (acpi_gpio_in_ignore_list(ACPI_GPIO_IGNORE_WAKE, dev_name(parent), pin)) {
 		dev_info(parent, "Ignoring wakeup on pin %u\n", pin);
 		return false;
@@ -368,6 +381,9 @@ static acpi_status acpi_gpiochip_alloc_event(struct acpi_resource *ares,
 		return AE_OK;
 
 	handle = ACPI_HANDLE(chip->parent);
+	if (agpio->pin_table_length == 0)
+		return AE_OK;
+
 	pin = agpio->pin_table[0];
 
 	if (pin <= 255) {
-- 
2.54.0

[PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Marco Scardovi]

From: "Marco Scardovi (scardracs)" <mscardovi95@gmail.com>

If acpi_remove_address_space_handler() fails, the cleanup function
acpi_gpiochip_free_regions() previously returned early. This leaks
the connections list and all requested GPIO descriptors.

Similarly, if acpi_gpio_adr_space_handler() fails to allocate a connection
or request a GPIO descriptor during a multi-pin transaction, it exits
without freeing the descriptors and connections that were already allocated
in the same call.

To avoid leaks, introduce a localized new_conns list inside the handler to
track the new connections requested during the current transaction. On
error, roll back only the connections in the new_conns list (avoiding
deadlock on the non-recursive conn_lock and preventing over-cleanup of
historic connections). On success, splice the new_conns list into the
global achip->conns list under the conn_lock.

Additionally, rename the global connection cleanup function to
acpi_gpiochip_free_all_connections() and call it in the GPIO chip teardown
path.

Fixes: 473ed7be0da0 ("gpio / ACPI: Add support for ACPI GPIO operation regions")
Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Marco Scardovi <mscardovi95@gmail.com>
---
 drivers/gpio/gpiolib-acpi-core.c | 142 ++++++++++++++++++++++++-------
 1 file changed, 111 insertions(+), 31 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index a6d78dad299e..e3bc4677b51d 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -1094,6 +1094,51 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
 }
 EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_wake_get_by);
 
+static void acpi_gpiochip_free_connection_list(struct list_head *list)
+{
+	struct acpi_gpio_connection *conn;
+	struct acpi_gpio_connection *tmp;
+
+	list_for_each_entry_safe_reverse(conn, tmp, list, node) {
+		gpiochip_free_own_desc(conn->desc);
+		list_del(&conn->node);
+		kfree(conn);
+	}
+}
+
+static void acpi_gpiochip_free_all_connections(struct acpi_gpio_chip *achip)
+{
+	guard(mutex)(&achip->conn_lock);
+
+	acpi_gpiochip_free_connection_list(&achip->conns);
+}
+
+/*
+ * Find a connection in the global or local list.
+ * The caller must hold achip->conn_lock to protect the global list.
+ * The local list new_conns is private to the calling thread.
+ */
+static struct acpi_gpio_connection *
+acpi_gpiochip_find_conn(struct acpi_gpio_chip *achip,
+			struct list_head *new_conns, unsigned int pin)
+{
+	struct acpi_gpio_connection *conn;
+
+	list_for_each_entry(conn, &achip->conns, node) {
+		if (conn->pin == pin)
+			return conn;
+	}
+
+	if (new_conns) {
+		list_for_each_entry(conn, new_conns, node) {
+			if (conn->pin == pin)
+				return conn;
+		}
+	}
+
+	return NULL;
+}
+
 static acpi_status
 acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 			    u32 bits, u64 *value, void *handler_context,
@@ -1101,11 +1146,16 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 {
 	struct acpi_gpio_chip *achip = region_context;
 	struct gpio_chip *chip = achip->chip;
+	struct acpi_gpio_connection *other;
+	struct acpi_gpio_connection *conn;
 	struct acpi_resource_gpio *agpio;
 	struct acpi_resource *ares;
 	u16 pin_index = address;
+	LIST_HEAD(new_conns);
 	acpi_status status;
 	int length;
+	u16 shift;
+	u16 word;
 	int i;
 
 	status = acpi_buffer_to_resource(achip->conn_info.connection,
@@ -1129,20 +1179,15 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 	length = min(agpio->pin_table_length, pin_index + bits);
 	for (i = pin_index; i < length; ++i) {
 		unsigned int pin = agpio->pin_table[i];
-		struct acpi_gpio_connection *conn;
 		struct gpio_desc *desc;
-		u16 word, shift;
-		bool found;
+		bool found = false;
 
 		mutex_lock(&achip->conn_lock);
 
-		found = false;
-		list_for_each_entry(conn, &achip->conns, node) {
-			if (conn->pin == pin) {
-				found = true;
-				desc = conn->desc;
-				break;
-			}
+		conn = acpi_gpiochip_find_conn(achip, &new_conns, pin);
+		if (conn) {
+			desc = conn->desc;
+			found = true;
 		}
 
 		/*
@@ -1163,34 +1208,66 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 			}
 		}
 
+		mutex_unlock(&achip->conn_lock);
+
 		if (!found) {
 			desc = acpi_request_own_gpiod(chip, agpio, i, "ACPI:OpRegion");
 			if (IS_ERR(desc)) {
-				mutex_unlock(&achip->conn_lock);
+				/*
+				 * In case of concurrent handler execution, another
+				 * thread may have already requested the same GPIO,
+				 * causing acpi_request_own_gpiod() to fail with -EBUSY.
+				 * Re-check the visible connection lists and reuse an
+				 * existing connection if one is found.
+				 */
+				if (PTR_ERR(desc) == -EBUSY) {
+					mutex_lock(&achip->conn_lock);
+					conn = acpi_gpiochip_find_conn(achip, &new_conns, pin);
+					if (conn) {
+						desc = conn->desc;
+						found = true;
+					}
+					mutex_unlock(&achip->conn_lock);
+					if (found)
+						goto use_existing;
+				}
 				status = AE_ERROR;
-				goto out;
+				goto err_free_new_conns;
 			}
 
 			conn = kzalloc_obj(*conn);
 			if (!conn) {
 				gpiochip_free_own_desc(desc);
-				mutex_unlock(&achip->conn_lock);
 				status = AE_NO_MEMORY;
-				goto out;
+				goto err_free_new_conns;
 			}
 
 			conn->pin = pin;
 			conn->desc = desc;
-			list_add_tail(&conn->node, &achip->conns);
-		}
 
-		mutex_unlock(&achip->conn_lock);
+			mutex_lock(&achip->conn_lock);
+			/*
+			 * Re-check both lists again before adding to local
+			 * new_conns, as another thread could have completed
+			 * and spliced its list while conn_lock was released.
+			 */
+			other = acpi_gpiochip_find_conn(achip, &new_conns, pin);
+			if (other) {
+				struct gpio_desc *desc_to_free = conn->desc;
+				struct acpi_gpio_connection *c_to_free = conn;
 
-		/*
-		 * For the cases when OperationRegion() consists of more than
-		 * 64 bits calculate the word and bit shift to use that one to
-		 * access the value.
-		 */
+				desc = other->desc;
+				mutex_unlock(&achip->conn_lock);
+
+				gpiochip_free_own_desc(desc_to_free);
+				kfree(c_to_free);
+			} else {
+				list_add_tail(&conn->node, &new_conns);
+				mutex_unlock(&achip->conn_lock);
+			}
+		}
+
+use_existing:
 		word = i / 64;
 		shift = i % 64;
 
@@ -1204,9 +1281,19 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		}
 	}
 
+	mutex_lock(&achip->conn_lock);
+	list_splice_tail(&new_conns, &achip->conns);
+	mutex_unlock(&achip->conn_lock);
+
+	status = AE_OK;
+
 out:
 	ACPI_FREE(ares);
 	return status;
+
+err_free_new_conns:
+	acpi_gpiochip_free_connection_list(&new_conns);
+	goto out;
 }
 
 static void acpi_gpiochip_request_regions(struct acpi_gpio_chip *achip)
@@ -1229,22 +1316,15 @@ static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip)
 {
 	struct gpio_chip *chip = achip->chip;
 	acpi_handle handle = ACPI_HANDLE(chip->parent);
-	struct acpi_gpio_connection *conn, *tmp;
 	acpi_status status;
 
 	status = acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
 						   acpi_gpio_adr_space_handler);
-	if (ACPI_FAILURE(status)) {
+	if (ACPI_FAILURE(status))
 		dev_err(chip->parent,
 			"Failed to remove GPIO OpRegion handler\n");
-		return;
-	}
 
-	list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) {
-		gpiochip_free_own_desc(conn->desc);
-		list_del(&conn->node);
-		kfree(conn);
-	}
+	acpi_gpiochip_free_all_connections(achip);
 }
 
 void acpi_gpiochip_add(struct gpio_chip *chip)
-- 
2.54.0

[PATCH v4 3/4] gpiolib: acpi: prevent address truncation in OperationRegion handler

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

Assisted-by: Antigravity:gemini-3.5-flash
Signed-off-by: Marco Scardovi <scardracs@disroot.org>
---
 drivers/gpio/gpiolib-acpi-core.c | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
index e3bc4677b51d..d12dab42a096 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -1150,13 +1150,13 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 	struct acpi_gpio_connection *conn;
 	struct acpi_resource_gpio *agpio;
 	struct acpi_resource *ares;
-	u16 pin_index = address;
+	unsigned int length;
 	LIST_HEAD(new_conns);
 	acpi_status status;
-	int length;
+	unsigned int i;
+	u16 pin_index;
 	u16 shift;
 	u16 word;
-	int i;
 
 	status = acpi_buffer_to_resource(achip->conn_info.connection,
 					 achip->conn_info.length, &ares);
@@ -1176,7 +1176,17 @@ 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 gpio_desc *desc;
-- 
2.54.0

[PATCH v4 4/4] gpiolib: acpi: fix out-of-bounds pointer arithmetic in acpi_gpio_package_count

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

Assisted-by: Antigravity:gemini-3.5-flash
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 d12dab42a096..b19fd02b64d0 100644
--- a/drivers/gpio/gpiolib-acpi-core.c
+++ b/drivers/gpio/gpiolib-acpi-core.c
@@ -1407,6 +1407,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

Re: [PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Mika Westerberg]

Hi,

On Sun, May 31, 2026 at 02:03:09PM +0200, Marco Scardovi wrote:
> From: "Marco Scardovi (scardracs)" <mscardovi95@gmail.com>
> 
> If acpi_remove_address_space_handler() fails, the cleanup function
> acpi_gpiochip_free_regions() previously returned early. This leaks
> the connections list and all requested GPIO descriptors.
> 
> Similarly, if acpi_gpio_adr_space_handler() fails to allocate a connection
> or request a GPIO descriptor during a multi-pin transaction, it exits
> without freeing the descriptors and connections that were already allocated
> in the same call.
> 
> To avoid leaks, introduce a localized new_conns list inside the handler to
> track the new connections requested during the current transaction. On
> error, roll back only the connections in the new_conns list (avoiding
> deadlock on the non-recursive conn_lock and preventing over-cleanup of
> historic connections). On success, splice the new_conns list into the
> global achip->conns list under the conn_lock.
> 
> Additionally, rename the global connection cleanup function to
> acpi_gpiochip_free_all_connections() and call it in the GPIO chip teardown
> path.
> 
> Fixes: 473ed7be0da0 ("gpio / ACPI: Add support for ACPI GPIO operation regions")
> Assisted-by: Antigravity:gemini-3.5-flash
> Signed-off-by: Marco Scardovi <mscardovi95@gmail.com>
> ---
>  drivers/gpio/gpiolib-acpi-core.c | 142 ++++++++++++++++++++++++-------
>  1 file changed, 111 insertions(+), 31 deletions(-)

This is really complex solution to a problem I'm not sure that even exists.

> diff --git a/drivers/gpio/gpiolib-acpi-core.c b/drivers/gpio/gpiolib-acpi-core.c
> index a6d78dad299e..e3bc4677b51d 100644
> --- a/drivers/gpio/gpiolib-acpi-core.c
> +++ b/drivers/gpio/gpiolib-acpi-core.c
> @@ -1094,6 +1094,51 @@ int acpi_dev_gpio_irq_wake_get_by(struct acpi_device *adev, const char *con_id,
>  }
>  EXPORT_SYMBOL_GPL(acpi_dev_gpio_irq_wake_get_by);
>  
> +static void acpi_gpiochip_free_connection_list(struct list_head *list)
> +{
> +	struct acpi_gpio_connection *conn;
> +	struct acpi_gpio_connection *tmp;
> +
> +	list_for_each_entry_safe_reverse(conn, tmp, list, node) {
> +		gpiochip_free_own_desc(conn->desc);
> +		list_del(&conn->node);
> +		kfree(conn);
> +	}
> +}
> +
> +static void acpi_gpiochip_free_all_connections(struct acpi_gpio_chip *achip)
> +{
> +	guard(mutex)(&achip->conn_lock);
> +
> +	acpi_gpiochip_free_connection_list(&achip->conns);
> +}
> +
> +/*
> + * Find a connection in the global or local list.
> + * The caller must hold achip->conn_lock to protect the global list.
> + * The local list new_conns is private to the calling thread.
> + */
> +static struct acpi_gpio_connection *
> +acpi_gpiochip_find_conn(struct acpi_gpio_chip *achip,
> +			struct list_head *new_conns, unsigned int pin)
> +{
> +	struct acpi_gpio_connection *conn;
> +
> +	list_for_each_entry(conn, &achip->conns, node) {
> +		if (conn->pin == pin)
> +			return conn;
> +	}
> +
> +	if (new_conns) {
> +		list_for_each_entry(conn, new_conns, node) {
> +			if (conn->pin == pin)
> +				return conn;
> +		}
> +	}
> +
> +	return NULL;
> +}
> +
>  static acpi_status
>  acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  			    u32 bits, u64 *value, void *handler_context,
> @@ -1101,11 +1146,16 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  {
>  	struct acpi_gpio_chip *achip = region_context;
>  	struct gpio_chip *chip = achip->chip;
> +	struct acpi_gpio_connection *other;
> +	struct acpi_gpio_connection *conn;
>  	struct acpi_resource_gpio *agpio;
>  	struct acpi_resource *ares;
>  	u16 pin_index = address;
> +	LIST_HEAD(new_conns);
>  	acpi_status status;
>  	int length;
> +	u16 shift;
> +	u16 word;
>  	int i;
>  
>  	status = acpi_buffer_to_resource(achip->conn_info.connection,
> @@ -1129,20 +1179,15 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  	length = min(agpio->pin_table_length, pin_index + bits);
>  	for (i = pin_index; i < length; ++i) {
>  		unsigned int pin = agpio->pin_table[i];
> -		struct acpi_gpio_connection *conn;
>  		struct gpio_desc *desc;
> -		u16 word, shift;
> -		bool found;
> +		bool found = false;
>  
>  		mutex_lock(&achip->conn_lock);
>  
> -		found = false;
> -		list_for_each_entry(conn, &achip->conns, node) {
> -			if (conn->pin == pin) {
> -				found = true;
> -				desc = conn->desc;
> -				break;
> -			}
> +		conn = acpi_gpiochip_find_conn(achip, &new_conns, pin);
> +		if (conn) {
> +			desc = conn->desc;
> +			found = true;
>  		}
>  
>  		/*
> @@ -1163,34 +1208,66 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  			}
>  		}
>  
> +		mutex_unlock(&achip->conn_lock);
> +
>  		if (!found) {
>  			desc = acpi_request_own_gpiod(chip, agpio, i, "ACPI:OpRegion");

If any of the below fails we clean up the descriptor while the prior are
already in the list. Once the GPIO chip is released the list is released as
well.

> -				mutex_unlock(&achip->conn_lock);
> +				/*
> +				 * In case of concurrent handler execution, another
> +				 * thread may have already requested the same GPIO,
> +				 * causing acpi_request_own_gpiod() to fail with -EBUSY.
> +				 * Re-check the visible connection lists and reuse an
> +				 * existing connection if one is found.
> +				 */
> +				if (PTR_ERR(desc) == -EBUSY) {
> +					mutex_lock(&achip->conn_lock);
> +					conn = acpi_gpiochip_find_conn(achip, &new_conns, pin);
> +					if (conn) {
> +						desc = conn->desc;
> +						found = true;
> +					}
> +					mutex_unlock(&achip->conn_lock);
> +					if (found)
> +						goto use_existing;
> +				}
>  				status = AE_ERROR;
> -				goto out;
> +				goto err_free_new_conns;
>  			}
>  
>  			conn = kzalloc_obj(*conn);
>  			if (!conn) {
>  				gpiochip_free_own_desc(desc);
> -				mutex_unlock(&achip->conn_lock);
>  				status = AE_NO_MEMORY;
> -				goto out;
> +				goto err_free_new_conns;
>  			}
>  
>  			conn->pin = pin;
>  			conn->desc = desc;
> -			list_add_tail(&conn->node, &achip->conns);
> -		}
>  
> -		mutex_unlock(&achip->conn_lock);
> +			mutex_lock(&achip->conn_lock);
> +			/*
> +			 * Re-check both lists again before adding to local
> +			 * new_conns, as another thread could have completed
> +			 * and spliced its list while conn_lock was released.
> +			 */
> +			other = acpi_gpiochip_find_conn(achip, &new_conns, pin);
> +			if (other) {
> +				struct gpio_desc *desc_to_free = conn->desc;
> +				struct acpi_gpio_connection *c_to_free = conn;
>  
> -		/*
> -		 * For the cases when OperationRegion() consists of more than
> -		 * 64 bits calculate the word and bit shift to use that one to
> -		 * access the value.
> -		 */
> +				desc = other->desc;
> +				mutex_unlock(&achip->conn_lock);
> +
> +				gpiochip_free_own_desc(desc_to_free);
> +				kfree(c_to_free);
> +			} else {
> +				list_add_tail(&conn->node, &new_conns);
> +				mutex_unlock(&achip->conn_lock);
> +			}
> +		}
> +
> +use_existing:
>  		word = i / 64;
>  		shift = i % 64;
>  
> @@ -1204,9 +1281,19 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
>  		}
>  	}
>  
> +	mutex_lock(&achip->conn_lock);
> +	list_splice_tail(&new_conns, &achip->conns);
> +	mutex_unlock(&achip->conn_lock);
> +
> +	status = AE_OK;
> +
>  out:
>  	ACPI_FREE(ares);
>  	return status;
> +
> +err_free_new_conns:
> +	acpi_gpiochip_free_connection_list(&new_conns);
> +	goto out;
>  }
>  
>  static void acpi_gpiochip_request_regions(struct acpi_gpio_chip *achip)
> @@ -1229,22 +1316,15 @@ static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip)
>  {
>  	struct gpio_chip *chip = achip->chip;
>  	acpi_handle handle = ACPI_HANDLE(chip->parent);
> -	struct acpi_gpio_connection *conn, *tmp;
>  	acpi_status status;
>  
>  	status = acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
>  						   acpi_gpio_adr_space_handler);
> -	if (ACPI_FAILURE(status)) {
> +	if (ACPI_FAILURE(status))
>  		dev_err(chip->parent,
>  			"Failed to remove GPIO OpRegion handler\n");
> -		return;
> -	}

If we fail to remove the OpRegion handler and continue to release the
structures does that mean any AML code that accesses the GPIOs now are
accessing already removed (below) objects? Here _REG is not called (we
failed to remove the OpRegion handler) so the AML does not know that it is
gone so it can happily still access the region. So IMHO it is safer to keep
the connections instead of removing them.

Of course it is possible that I'm missing something. It has been a while
since I looked at any of this stuff.

>  
> -	list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) {
> -		gpiochip_free_own_desc(conn->desc);
> -		list_del(&conn->node);
> -		kfree(conn);
> -	}
> +	acpi_gpiochip_free_all_connections(achip);
>  }
>  
>  void acpi_gpiochip_add(struct gpio_chip *chip)
> -- 
> 2.54.0

Re: [PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Marco Scardovi]

In data lunedì 1 giugno 2026 14:18:15 Ora legale dell’Europa centrale, hai 
scritto:
> Hi,
> 
> On Sun, May 31, 2026 at 02:03:09PM +0200, Marco Scardovi wrote:
> > ...
> 
> If we fail to remove the OpRegion handler and continue to release the
> structures does that mean any AML code that accesses the GPIOs now are
> accessing already removed (below) objects? Here _REG is not called (we
> failed to remove the OpRegion handler) so the AML does not know that it is
> gone so it can happily still access the region. So IMHO it is safer to keep
> the connections instead of removing them.
> 
> Of course it is possible that I'm missing something. It has been a while
> since I looked at any of this stuff.
> 
Hi Mika,

no, I get your point: I was being overly cautious about something that may
or may not happen. Consider these patches as void and sorry for the noise.

Marco

Re: [PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Andy Shevchenko]

On Mon, Jun 01, 2026 at 02:18:15PM +0200, Mika Westerberg wrote:
> On Sun, May 31, 2026 at 02:03:09PM +0200, Marco Scardovi wrote:

...

> This is really complex solution to a problem I'm not sure that even exists.

The problem that potentially may have an attack vector is the malicious ACPI tables
(via SSDT overlay, for example). So some of the validations (like pin index in the
pin list) would be somewhat useful.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Mika Westerberg]

On Tue, Jun 02, 2026 at 10:57:50AM +0300, Andy Shevchenko wrote:
> On Mon, Jun 01, 2026 at 02:18:15PM +0200, Mika Westerberg wrote:
> > On Sun, May 31, 2026 at 02:03:09PM +0200, Marco Scardovi wrote:
> 
> ...
> 
> > This is really complex solution to a problem I'm not sure that even exists.
> 
> The problem that potentially may have an attack vector is the malicious ACPI tables
> (via SSDT overlay, for example). So some of the validations (like pin index in the
> pin list) would be somewhat useful.

Yes but with SSDT overlay you can do much worse things than passing an
invalid pin index.

Re: [PATCH v4 2/4] gpiolib: acpi: fix resource leak in OpRegion

[Andy Shevchenko]

On Tue, Jun 02, 2026 at 11:53:46AM +0200, Mika Westerberg wrote:
> On Tue, Jun 02, 2026 at 10:57:50AM +0300, Andy Shevchenko wrote:
> > On Mon, Jun 01, 2026 at 02:18:15PM +0200, Mika Westerberg wrote:
> > > On Sun, May 31, 2026 at 02:03:09PM +0200, Marco Scardovi wrote:

...

> > > This is really complex solution to a problem I'm not sure that even exists.
> > 
> > The problem that potentially may have an attack vector is the malicious ACPI tables
> > (via SSDT overlay, for example). So some of the validations (like pin index in the
> > pin list) would be somewhat useful.
> 
> Yes but with SSDT overlay you can do much worse things than passing an
> invalid pin index.

Still my point is valid :-)

-- 
With Best Regards,
Andy Shevchenko