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

[Cédric Le Goater]

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.

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