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

[Mika Westerberg <mika.westerberg@linux.intel.com>]

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