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

[Guenter Roeck]

Hi Peter,

On 3/16/23 06:41, Peter Maydell wrote:
> 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

Good catch. And an obvious bug, sorry.

> > +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.

I have no idea what the real hardware would do. The datasheets (at
least the ones I checked) don't say, only that the region size is 4k.
I would suggest a bounds check, ignore out-of-bounds writes (maybe
with a log message), and return 0 for reads (which I think is what
you suggest with RAZ/WI).

Want me to send a patch ?

Thanks,
Guenter