Re: [PATCH 1/2] hw/gpio/aspeed: Don't let guests modify input pins

[Peter Delevoryas]

On Mon, Jul 11, 2022 at 10:56:08PM +0930, Andrew Jeffery wrote:
> 
> 
> On Fri, 8 Jul 2022, at 04:34, Peter Delevoryas wrote:
> > On Thu, Jul 07, 2022 at 10:53:57AM -0700, Peter Delevoryas wrote:
> >> On Thu, Jul 07, 2022 at 10:56:02AM +0200, Cédric Le Goater wrote:
> >> > On 7/7/22 09:17, Peter Delevoryas wrote:
> >> > > It seems that aspeed_gpio_update is allowing the value for input pins 
> >> > > to be
> >> > > modified through register writes and QOM property modification.
> >> > > 
> >> > > The QOM property modification is fine, but modifying the value through
> >> > > register writes from the guest OS seems wrong if the pin's direction 
> >> > > is set
> >> > > to input.
> >> > > 
> >> > > The datasheet specifies that "0" bits in the direction register select 
> >> > > input
> >> > > mode, and "1" selects output mode.
> >> > > 
> >> > > OpenBMC userspace code is accidentally writing 0's to the GPIO data
> >> > > registers somewhere (or perhaps the driver is doing it through a reset 
> >> > > or
> >> > > something), and this is overwriting GPIO FRU information (board ID, 
> >> > > slot
> >> > > presence pins) that is initialized in Aspeed machine reset code (see
> >> > > fby35_reset() in hw/arm/aspeed.c).
> >> > 
> >> > It might be good to log a GUEST_ERROR in that case, when writing to an
> >> > input GPIO and when reading from an output GPIO.
> >> 
> >> Good idea, I'll include a GUEST_ERROR for writing to an input GPIO.
> >> 
> >> I'm actually not totally certain about emitting an error when reading from 
> >> an
> >> output GPIO, because the driver can only do 8-bit reads at the finest
> >> granularity, and if 1 of the 8 pins' direction is output, then it will be
> >> reading the value of an output pin. But, that's not really bad, because
> >> presumably the value will be ignored. Maybe I can go test this out on
> >> hardware and figure out what happens though.
> >
> > Did a small experiment, I was looking at some of the most significant
> > bits:
> >
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x3CFF303E
> > root@dhcp-100-96-192-133:~# devmem 0x1e780004
> > 0x2800000C
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000 32 0xffffffff
> > root@dhcp-100-96-192-133:~# devmem 0x1e780004
> > 0x2800000C
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x3CFF303E
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x3CFF303E
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000 32 0
> > root@dhcp-100-96-192-133:~# devmem 0x1e780000
> > 0x14FF303A
> >
> > Seems like the output pin 0x20000000 was initially high, and the input
> > pin right next to it 0x10000000 was also high. After writing 0 to the
> > data register, the value in the data register changed for the output
> > pin, but not the input pin.  Which matches what we're planning on doing
> > in the controller.
> >
> > So yeah, I'll add GUEST_ERROR for writes to input pins but not output
> > pins.  The driver should probably be doing a read-modify-update.
> > Although...if it's not, that technically wouldn't matter, behavior
> > wise...maybe GUEST_ERROR isn't appropriate for writes to input pins
> > either, for the same reason as I mentioned with reads of output pins.
> > I'll let you guys comment on what you think we should do.
> >
> 
> With the quick hack below I think I got sensible behaviour?
> 
> ```
> # devmem 0x1e780000
> 0x00000000
> # devmem 0x1e780004
> 0x00000000
> # devmem 0x1e780004 32 1
> # devmem 0x1e780000 32 1
> # devmem 0x1e780000
> 0x00000001
> # devmem 0x1e780000 32 3
> # devmem 0x1e780000
> 0x00000001
> # QEMU 7.0.0 monitor - type 'help' for more information
> (qemu) qom-set gpio gpioA1 on
> (qemu) 
> 
> # devmem 0x1e780000
> 0x00000003
> # (qemu) qom-set gpio gpioA1 off
> (qemu) 
> 
> # devmem 0x1e780000
> 0x00000001
> # (qemu) qom-set gpio gpioA0 off
> (qemu) 
> # devmem 0x1e780000
> 0x00000001
> # 
> ```
> 
> That was with the patch below. However, I think there's a deeper issue 
> with the input masking that needs to be addressed. Essentially we lack 
> modelling for the actual line state, we were proxying that with 
> register state. As it stands if we input-mask a line and use qom-set to 
> change its state the state update will go missing. However, as Joel 
> notes, I don't think we have anything configuring input masking.
> 
> diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> index c63634d3d3e2..a1aa6504a8d8 100644
> --- a/hw/gpio/aspeed_gpio.c
> +++ b/hw/gpio/aspeed_gpio.c
> @@ -244,7 +244,7 @@ static ptrdiff_t aspeed_gpio_set_idx(AspeedGPIOState *s, 
> GPIOSets *regs)
>  }
>  
>  static void aspeed_gpio_update(AspeedGPIOState *s, GPIOSets *regs,
> -                               uint32_t value)
> +                               uint32_t value, uint32_t mode_mask)
>  {
>      uint32_t input_mask = regs->input_mask;
>      uint32_t direction = regs->direction;
> @@ -253,7 +253,8 @@ static void aspeed_gpio_update(AspeedGPIOState *s, 
> GPIOSets *regs,
>      uint32_t diff;
>      int gpio;
>  
> -    diff = old ^ new;
> +    diff = (old ^ new);
> +    diff &= mode_mask;
>      if (diff) {
>          for (gpio = 0; gpio < ASPEED_GPIOS_PER_SET; gpio++) {
>              uint32_t mask = 1 << gpio;
> @@ -315,7 +316,7 @@ static void aspeed_gpio_set_pin_level(AspeedGPIOState *s, 
> uint32_t set_idx,
>          value &= !pin_mask;
>      }
>  
> -    aspeed_gpio_update(s, &s->sets[set_idx], value);
> +    aspeed_gpio_update(s, &s->sets[set_idx], value, 
> ~s->sets[set_idx].direction);
>  }
>  
>  /*
> @@ -607,7 +608,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr 
> offset, uint64_t data,
>          data &= props->output;
>          data = update_value_control_source(set, set->data_value, data);
>          set->data_read = data;
> -        aspeed_gpio_update(s, set, data);
> +        aspeed_gpio_update(s, set, data, set->direction);
>          return;
>      case gpio_reg_direction:
>          /*
> @@ -683,7 +684,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr 
> offset, uint64_t data,
>                        HWADDR_PRIx"\n", __func__, offset);
>          return;
>      }
> -    aspeed_gpio_update(s, set, set->data_value);
> +    aspeed_gpio_update(s, set, set->data_value, UINT32_MAX);

Looks great overall, but why is the mode_mask UINT32_MAX here? Shouldn't it be
set->direction? We only want to let the guest OS write to output pins, right?
Or is that not how the register works?

>      return;
>  }
>  
>