On Wed, May 8, 2024 at 4:53 AM Philippe Mathieu-Daudé <phi...@linaro.org> wrote:
> On 7/5/24 22:20, Cord Amfmgm wrote: > > > > > > On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamf...@gmail.com > > <mailto:dmamf...@gmail.com>> wrote: > > > > On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <m...@tls.msk.ru > > <mailto:m...@tls.msk.ru>> wrote: > > > > 06.02.2024 10:13, Cord Amfmgm wrote: > > > This changes the ohci validation to not assert if invalid > > > data is fed to the ohci controller. The poc suggested in > > > https://bugs.launchpad.net/qemu/+bug/1907042 > > <https://bugs.launchpad.net/qemu/+bug/1907042> > > > and then migrated to bug #303 does the following to > > > feed it a SETUP pid and EndPt of 1: > > > > > > uint32_t MaxPacket = 64; > > > uint32_t TDFormat = 0; > > > uint32_t Skip = 0; > > > uint32_t Speed = 0; > > > uint32_t Direction = 0; /* #define > > OHCI_TD_DIR_SETUP 0 */ > > > uint32_t EndPt = 1; > > > uint32_t FuncAddress = 0; > > > ed->attr = (MaxPacket << 16) | (TDFormat << 15) | > > (Skip << 14) > > > | (Speed << 13) | (Direction << 11) | > > (EndPt << 7) > > > | FuncAddress; > > > ed->tailp = /*TDQTailPntr= */ 0; > > > ed->headp = ((/*TDQHeadPntr= */ &td[0]) & 0xfffffff0) > > > | (/* ToggleCarry= */ 0 << 1); > > > ed->next_ed = (/* NextED= */ 0 & 0xfffffff0) > > > > > > qemu-fuzz also caught the same issue in #1510. They are > > > both fixed by this patch. > > > > > > The if (td.cbp > td.be <http://td.be>) logic in > > ohci_service_td() causes an > > > ohci_die(). My understanding of the OHCI spec 4.3.1.2 > > > Table 4-2 allows td.cbp to be one byte more than td.be > > <http://td.be> to > > > signal the buffer has zero length. The new check in qemu > > > appears to have been added since qemu-4.2. This patch > > > includes both fixes since they are located very close > > > together. > > > > > > Signed-off-by: David Hubbard <dmamf...@gmail.com > > <mailto:dmamf...@gmail.com>> > > > > Wonder if this got lost somehow. Or is it not needed? > > > > Thanks, > > > > /mjt > > > > > > Friendly ping! Gerd, can you chime in with how you would like to > > approach this? I still need this patch to unblock my qemu workflow - > > custom OS development. > > > > > > Can I please ask for an update on this? I'm attempting to figure out if > > this patch has been rejected and I need to resubmit / rework it at HEAD? > > > > > > > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c > > > index d73b53f33c..a53808126f 100644 > > > --- a/hw/usb/hcd-ohci.c > > > +++ b/hw/usb/hcd-ohci.c > > > @@ -927,6 +927,11 @@ static int ohci_service_td(OHCIState > *ohci, > > > struct ohci_ed *ed) > > > case OHCI_TD_DIR_SETUP: > > > str = "setup"; > > > pid = USB_TOKEN_SETUP; > > > + if (OHCI_BM(ed->flags, ED_EN) > 0) { /* setup only > > allowed to ep 0 */ > > > + trace_usb_ohci_td_bad_pid(str, ed->flags, > td.flags); > > > + ohci_die(ohci); > > > + return 1; > > > + } > > > break; > > I made a comment on April 18 but it is not showing on the list... > > https://lore.kernel.org/qemu-devel/593072d7-614b-4197-9c9a-12bb70c31...@linaro.org/ > > It was: > > > Please split in 2 different patches. > > Even if closely related, it simplifies the workflow to have > single fix in single commit; for example if one is invalid, > we can revert it and not the other. > Sure, I can submit 2 separate patches. I'm unfamiliar with how to get those to show up in this patch request, I assume it's not too bad if I submit that as a separate patch request? On Wed, May 8, 2024 at 3:45 AM Thomas Huth <th...@redhat.com> wrote: > Your Signed-off-by line does not match the From: line ... could you please > fix this? (see > > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line > , too) I'll submit the new patch request with my pseudonym in the From: and Signed-off-by: lines, per your request. Doesn't matter to me. However, this arises simply because I don't give gmail my real name - https://en.wikipedia.org/wiki/Nymwars > > > > default: > > > trace_usb_ohci_td_bad_direction(dir); > > > @@ -936,8 +941,8 @@ static int ohci_service_td(OHCIState > > *ohci, struct > > > ohci_ed *ed) > > > if ((td.cbp & 0xfffff000) != (td.be <http://td.be> > > & 0xfffff000)) { > > > len = (td.be <http://td.be> & 0xfff) + 0x1001 - > > (td.cbp & 0xfff); > > > } else { > > > - if (td.cbp > td.be <http://td.be>) { > > > - trace_usb_ohci_iso_td_bad_cc_overrun(td.cbp, > > td.be <http://td.be>); > > > + if (td.cbp > td.be <http://td.be> + 1) { > > > + trace_usb_ohci_td_bad_buf(td.cbp, td.be > > <http://td.be>); > > > ohci_die(ohci); > > > return 1; > > > } > > > diff --git a/hw/usb/trace-events b/hw/usb/trace-events > > > index ed7dc210d3..b47d082fa3 100644 > > > --- a/hw/usb/trace-events > > > +++ b/hw/usb/trace-events > > > @@ -28,6 +28,8 @@ usb_ohci_iso_td_data_overrun(int ret, > > ssize_t len) > > > "DataOverrun %d > %zu" > > > usb_ohci_iso_td_data_underrun(int ret) "DataUnderrun %d" > > > usb_ohci_iso_td_nak(int ret) "got NAK/STALL %d" > > > usb_ohci_iso_td_bad_response(int ret) "Bad device response > %d" > > > +usb_ohci_td_bad_buf(uint32_t cbp, uint32_t be) "Bad cbp = > > 0x%x > be = 0x%x" > > > +usb_ohci_td_bad_pid(const char *s, uint32_t edf, uint32_t > > tdf) "Bad > > > pid %s: ed.flags 0x%x td.flags 0x%x" > > > usb_ohci_port_attach(int index) "port #%d" > > > usb_ohci_port_detach(int index) "port #%d" > > > usb_ohci_port_wakeup(int index) "port #%d" > > > > > > >
