[v1] hw: aspeed_gpio: Fix GPIO array indexing

[PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing

Posted by pdel@fb.com 4 years, 4 months ago

From: Peter Delevoryas <pdel@fb.com>

Hey everyone,

I think there might be a bug in aspeed_gpio_update, where it's selecting
a GPIO IRQ to update. The indexing that maps from GPIO pin to IRQ leads
to an out-of-bounds array access and a segfault after that.

tl;dr

There's 8 rows of 32 pins (8 * 32 == 256 total) on the AST2500, but some
of the pins are not actually active: there's only 228 pins actually
active in the AST2500.

The GPIO IRQ array has length 228, but we index it using a matrix
indexing scheme like [row][column], and end up out-of-bounds for
high-numbered pins.

I fixed this by converting the IRQ array to a matrix, where some
of the entries are uninitialized (zero). This retains the matrix
indexing scheme, which I think is easy to understand.

Notes on reproducing:

I was testing booting Facebook's OpenBMC platform "YosemiteV2" (fby2)
and hit a segfault:

  qemu-system-arm -machine ast2500-evb \
      -drive file=fby2.mtd,format=raw,if=mtd \
      -serial stdio -display none
  ...
  Setup Caching for Bridge IC info..done.
  Setup Front Panel Daemon..done.
  Setup fan speed...
  FAN CONFIG : Single Rotor FAN
  Unexpected 4 Servers config! Run FSC 4 TLs Config as default config
  Setting Zone 0 speed to 70%
  Setting Zone 1 speed to 70%
  ok: run: fscd: (pid 1726) 0s
  done.
  Powering fru 1 to ON state...
  Segmentation fault (core dumped)

In gdb:

  Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
  [Switching to Thread 0x7ffff20ee700 (LWP 1840353)]
  qemu_set_irq (irq=0xffffffff00000000, level=1) at ../hw/core/irq.c:45
  45          irq->handler(irq->opaque, irq->n, level);
  (gdb) p irq
  $1 = (qemu_irq) 0xffffffff00000000
  (gdb) up
  #1  0x00005555558e36f5 in aspeed_gpio_update (s=0x7ffff7ecffb0, regs=0x7ffff7ed0c94, value=128) at ../hw/gpio/aspeed_gpio.c:287
  287                     qemu_set_irq(s->gpios[offset], !!(new & mask));
  (gdb) p s->gpios
  $2 = {0x0 <repeats 228 times>}
  (gdb) p offset
  $3 = 231
  (gdb) p set
  $5 = 7
  (gdb) p gpio
  $4 = 7

With my fix, I can boot the fby2 platform. The image I was using is here:

https://github.com/peterdelevoryas/openbmc/releases/tag/fby2.debug.mtd

Peter Delevoryas (1):
  hw: aspeed_gpio: Fix GPIO array indexing

 hw/gpio/aspeed_gpio.c         | 72 ++++++++++++++---------------------
 include/hw/gpio/aspeed_gpio.h |  5 +--
 2 files changed, 31 insertions(+), 46 deletions(-)

-- 
2.30.2

Re: [PATCH 0/1] hw: aspeed_gpio: Fix GPIO array indexing

Posted by Peter Delevoryas 4 years, 4 months ago


> On Sep 27, 2021, at 8:43 PM, pdel@fb.com wrote:
> 
> From: Peter Delevoryas <pdel@fb.com>
> 
> Hey everyone,
> 
> I think there might be a bug in aspeed_gpio_update, where it's selecting
> a GPIO IRQ to update. The indexing that maps from GPIO pin to IRQ leads
> to an out-of-bounds array access and a segfault after that.
> 
> tl;dr
> 
> There's 8 rows of 32 pins (8 * 32 == 256 total) on the AST2500, but some
> of the pins are not actually active: there's only 228 pins actually
> active in the AST2500.
> 
> The GPIO IRQ array has length 228, but we index it using a matrix
> indexing scheme like [row][column], and end up out-of-bounds for
> high-numbered pins.
> 
> I fixed this by converting the IRQ array to a matrix, where some
> of the entries are uninitialized (zero). This retains the matrix
> indexing scheme, which I think is easy to understand.
> 
> Notes on reproducing:
> 
> I was testing booting Facebook's OpenBMC platform "YosemiteV2" (fby2)
> and hit a segfault:
> 
>  qemu-system-arm -machine ast2500-evb \
>      -drive file=fby2.mtd,format=raw,if=mtd \
>      -serial stdio -display none
>  ...
>  Setup Caching for Bridge IC info..done.
>  Setup Front Panel Daemon..done.
>  Setup fan speed...
>  FAN CONFIG : Single Rotor FAN
>  Unexpected 4 Servers config! Run FSC 4 TLs Config as default config
>  Setting Zone 0 speed to 70%
>  Setting Zone 1 speed to 70%
>  ok: run: fscd: (pid 1726) 0s
>  done.
>  Powering fru 1 to ON state...
>  Segmentation fault (core dumped)
> 
> In gdb:
> 
>  Thread 3 "qemu-system-arm" received signal SIGSEGV, Segmentation fault.
>  [Switching to Thread 0x7ffff20ee700 (LWP 1840353)]
>  qemu_set_irq (irq=0xffffffff00000000, level=1) at ../hw/core/irq.c:45
>  45          irq->handler(irq->opaque, irq->n, level);
>  (gdb) p irq
>  $1 = (qemu_irq) 0xffffffff00000000
>  (gdb) up
>  #1  0x00005555558e36f5 in aspeed_gpio_update (s=0x7ffff7ecffb0, regs=0x7ffff7ed0c94, value=128) at ../hw/gpio/aspeed_gpio.c:287
>  287                     qemu_set_irq(s->gpios[offset], !!(new & mask));
>  (gdb) p s->gpios
>  $2 = {0x0 <repeats 228 times>}
>  (gdb) p offset
>  $3 = 231
>  (gdb) p set
>  $5 = 7
>  (gdb) p gpio
>  $4 = 7
> 
> With my fix, I can boot the fby2 platform. The image I was using is here:
> 
> https://github.com/peterdelevoryas/openbmc/releases/tag/fby2.debug.mtd
> 
> Peter Delevoryas (1):
>  hw: aspeed_gpio: Fix GPIO array indexing
> 
> hw/gpio/aspeed_gpio.c         | 72 ++++++++++++++---------------------
> include/hw/gpio/aspeed_gpio.h |  5 +--
> 2 files changed, 31 insertions(+), 46 deletions(-)
> 
> -- 
> 2.30.2
> 

cc’ing Dan