Re: [PATCH v3 2/9] hw/i2c: pmbus: guard against out of range accesses

["Philippe Mathieu-Daudé" <philippe.mathieu.daude@gmail.com>]

On 2/3/22 02:50, Titus Rwantare wrote:
> Signed-off-by: Titus Rwantare <titusr@google.com>
> ---
>   hw/i2c/pmbus_device.c | 41 ++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 40 insertions(+), 1 deletion(-)

>   static uint8_t pmbus_receive_byte(SMBusDevice *smd)
>   {
>       PMBusDevice *pmdev = PMBUS_DEVICE(smd);
>       PMBusDeviceClass *pmdc = PMBUS_DEVICE_GET_CLASS(pmdev);
>       uint8_t ret = 0xFF;
> -    uint8_t index = pmdev->page;
> +    uint8_t index;
>   
>       if (pmdev->out_buf_len != 0) {
>           ret = pmbus_out_buf_pop(pmdev);
>           return ret;
>       }
>   
> +    /*
> +     * Reading from all pages will return the value from page 0,
> +     * this is unspecified behaviour in general.
> +     */
> +    if (pmdev->page == PB_ALL_PAGES) {
> +        index = 0;
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: tried to read from all pages\n",
> +                      __func__);
> +        pmbus_cml_error(pmdev);
> +    } else if (pmdev->page > pmdev->num_pages - 1) {
> +        qemu_log_mask(LOG_GUEST_ERROR,
> +                      "%s: page %d is out of range\n",
> +                      __func__, pmdev->page);
> +        pmbus_cml_error(pmdev);
> +        return -1;

This file returns a mix of 0xFF/-1 for error. It would be nice
to pick one. Adding a definition (PMBUS_ERR_BYTE?) could help.

Preferably with error unified:
Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>