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

[Peter Delevoryas]

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.

> 
> Thanks,
> Peter
> 
> > 
> > Thanks,
> > 
> > C.
> > 
> > > 
> > > Signed-off-by: Peter Delevoryas <peter@pjd.dev>
> > > Fixes: 4b7f956862dc ("hw/gpio: Add basic Aspeed GPIO model for AST2400 
> > > and AST2500")
> > > ---
> > >   hw/gpio/aspeed_gpio.c | 22 ++++++++++++----------
> > >   1 file changed, 12 insertions(+), 10 deletions(-)
> > > 
> > > diff --git a/hw/gpio/aspeed_gpio.c b/hw/gpio/aspeed_gpio.c
> > > index a62a673857..2eae427201 100644
> > > --- a/hw/gpio/aspeed_gpio.c
> > > +++ b/hw/gpio/aspeed_gpio.c
> > > @@ -268,7 +268,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, bool force)
> > >   {
> > >       uint32_t input_mask = regs->input_mask;
> > >       uint32_t direction = regs->direction;
> > > @@ -293,10 +293,12 @@ static void aspeed_gpio_update(AspeedGPIOState *s, 
> > > GPIOSets *regs,
> > >               }
> > >               /* ...then update the state. */
> > > -            if (mask & new) {
> > > -                regs->data_value |= mask;
> > > -            } else {
> > > -                regs->data_value &= ~mask;
> > > +            if (direction & mask || force) {
> > > +                if (mask & new) {
> > > +                    regs->data_value |= mask;
> > > +                } else {
> > > +                    regs->data_value &= ~mask;
> > > +                }
> > >               }
> > >               /* If the gpio is set to output... */
> > > @@ -339,7 +341,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, true);
> > >   }
> > >   /*
> > > @@ -653,7 +655,7 @@ static void aspeed_gpio_write_index_mode(void 
> > > *opaque, hwaddr offset,
> > >           reg_value = update_value_control_source(set, set->data_value,
> > >                                                   reg_value);
> > >           set->data_read = reg_value;
> > > -        aspeed_gpio_update(s, set, reg_value);
> > > +        aspeed_gpio_update(s, set, reg_value, false);
> > >           return;
> > >       case gpio_reg_idx_direction:
> > >           reg_value = set->direction;
> > > @@ -753,7 +755,7 @@ static void aspeed_gpio_write_index_mode(void 
> > > *opaque, hwaddr offset,
> > >               __func__, offset, data, reg_idx_type);
> > >           return;
> > >       }
> > > -    aspeed_gpio_update(s, set, set->data_value);
> > > +    aspeed_gpio_update(s, set, set->data_value, false);
> > >       return;
> > >   }
> > > @@ -799,7 +801,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, false);
> > >           return;
> > >       case gpio_reg_direction:
> > >           /*
> > > @@ -875,7 +877,7 @@ static void aspeed_gpio_write(void *opaque, hwaddr 
> > > offset, uint64_t data,
> > >                         PRIx64"\n", __func__, offset);
> > >           return;
> > >       }
> > > -    aspeed_gpio_update(s, set, set->data_value);
> > > +    aspeed_gpio_update(s, set, set->data_value, false);
> > >       return;
> > >   }
> > 
>