Re: [PATCH v3 1/5] hw/usb: Add basic i.MX USB Phy support

[Peter Maydell]

On Fri, 13 Mar 2020 at 01:45, Guenter Roeck <linux@roeck-us.net> wrote:
>
> Add basic USB PHY support as implemented in i.MX23, i.MX28, i.MX6,
> and i.MX7 SoCs.
>
> The only support really needed - at least to boot Linux - is support
> for soft reset, which needs to reset various registers to their initial
> value. Otherwise, just record register values.
>
> Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Guenter Roeck <linux@roeck-us.net>

Hi Guenter; we've had a fuzzer report that this device model
accesses off the end of the usbphy[] array:
https://gitlab.com/qemu-project/qemu/-/issues/1408

> +static uint64_t imx_usbphy_read(void *opaque, hwaddr offset, unsigned size)
> +{
> +    IMXUSBPHYState *s = (IMXUSBPHYState *)opaque;
> +    uint32_t index = offset >> 2;
> +    uint32_t value;


> +    default:
> +        value = s->usbphy[index];

No bounds check in the default case (or ditto in the write function)...

> +        break;
> +    }
> +    return (uint64_t)value;
> +}

> +static void imx_usbphy_realize(DeviceState *dev, Error **errp)
> +{
> +    IMXUSBPHYState *s = IMX_USBPHY(dev);
> +
> +    memory_region_init_io(&s->iomem, OBJECT(s), &imx_usbphy_ops, s,
> +                          "imx-usbphy", 0x1000);

...and the memory region is created at size 0x1000 so the read/write
fns can be called with offsets up to that size...

> +    sysbus_init_mmio(SYS_BUS_DEVICE(s), &s->iomem);
> +}

> +enum IMXUsbPhyRegisters {
> +    USBPHY_PWD,
> +    USBPHY_PWD_SET,
> +    USBPHY_PWD_CLR,
> +    USBPHY_PWD_TOG,
> +    USBPHY_TX,
> +    USBPHY_TX_SET,
> +    USBPHY_TX_CLR,
> +    USBPHY_TX_TOG,
> +    USBPHY_RX,
> +    USBPHY_RX_SET,
> +    USBPHY_RX_CLR,
> +    USBPHY_RX_TOG,
> +    USBPHY_CTRL,
> +    USBPHY_CTRL_SET,
> +    USBPHY_CTRL_CLR,
> +    USBPHY_CTRL_TOG,
> +    USBPHY_STATUS,
> +    USBPHY_DEBUG = 0x14,
> +    USBPHY_DEBUG_SET,
> +    USBPHY_DEBUG_CLR,
> +    USBPHY_DEBUG_TOG,
> +    USBPHY_DEBUG0_STATUS,
> +    USBPHY_DEBUG1 = 0x1c,
> +    USBPHY_DEBUG1_SET,
> +    USBPHY_DEBUG1_CLR,
> +    USBPHY_DEBUG1_TOG,
> +    USBPHY_VERSION,
> +    USBPHY_MAX
> +};
> +
> +#define USBPHY_CTRL_SFTRST BIT(31)
> +
> +#define TYPE_IMX_USBPHY "imx.usbphy"
> +#define IMX_USBPHY(obj) OBJECT_CHECK(IMXUSBPHYState, (obj), TYPE_IMX_USBPHY)
> +
> +typedef struct IMXUSBPHYState {
> +    /* <private> */
> +    SysBusDevice parent_obj;
> +
> +    /* <public> */
> +    MemoryRegion iomem;
> +
> +    uint32_t usbphy[USBPHY_MAX];

...but the array is only created with USBPHY_MAX entries.

Do you know what the device is supposed to do with these
off-the-end acceses? We could either reduce the memory region
size or bounds check and RAZ/WI the out-of-range accesses.

thanks
-- PMM