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

[Peter Delevoryas <peter@pjd.dev>]

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.

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;
> >   }
>