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

[Marco Scardovi]

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@xxxxxxxxxxx>
> > > > ---
> > > > 
> > > >  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: