Re: [PATCH] hw/usb/hcd-dwc2: fix divide-by-zero in dwc2_handle_packet()

[Paul Zimmerman]

On Wed, Oct 14, 2020 at 12:55 PM Mauro Matteo Cascella
<mcascell@redhat.com> wrote:
>
> On Tue, Oct 13, 2020 at 10:41 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> >
> > > > Also: What would be the effect of simply returning here? Would dwc2
> > > > emulation simply stop processing queues? Should we maybe raise an
> > > > error IRQ?
> > >
> > > Not entirely sure, I imagine the emulation will just stop working. I can
> > > test it tomorrow. Also, can you give me a hint what an error IRQ is?
> > > Is that a Qemu thing, or do you mean we should emulate what the
> > > real core does in this case?
> >
> > Same real hardware does.  ehci for example has the USBSTS_ERRINT bit in
> > the IRQ status register to signal errors.

So I tested this patch (modified as I suggested) in two ways. First, I
hacked up the dwc_otg driver in the Raspbian kernel to always set the
'mps' field to zero. Then I booted the 2019-09-26-raspbian-buster
Raspbian image with 4 usb devices, keyboard, tablet, storage, and
network.

In this test, the first USB device failed to enumerate, giving the kernel
error message "usb 1-1: device descriptor read/64, error -110". The
kernel retried the enumeration several times, finally giving the
message "usb usb1-port1: unable to enumerate usb device".
After that, there was no more USB activity shown in the kernel log.
Otherwise the kernel seemed to keep running normally and the
raspi emulation continued to run.

In the second test, I changed the hack to only zero the 'mps' field for
bulk transfers. In that case, the non-bulk devices (keyboard, tablet)
continued to operate normally. The bulk devices (storage, network)
enumerated fine, but after that they failed to work, with various
error messages in the kernel log. Again the kernel seemed to keep
running normally and the raspi emulation continued to run.

So I think the patch works fine, and I don't think we need to do
anything fancier.

- Paul

> >
> > take care,
> >   Gerd
> >
>
> I'll send a new version of the patch with the check moved earlier in
> the function, as suggested by Paul. If raising an error turns out to
> be the right thing to do, I think other checks may need to be updated
> in addition to this one. Hence we can possibly address that in a later
> patch. Thanks a lot for your comments.
>
> --
> Mauro Matteo Cascella, Red Hat Product Security
> 6F78 E20B 5935 928C F0A8  1A9D 4E55 23B8 BB34 10B0
>