On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamf...@gmail.com> wrote:
> On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <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 >> > 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) 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 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> >> >> 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; >> > 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 & 0xfffff000)) { >> > len = (td.be & 0xfff) + 0x1001 - (td.cbp & 0xfff); >> > } else { >> > - if (td.cbp > td.be) { >> > - trace_usb_ohci_iso_td_bad_cc_overrun(td.cbp, td.be); >> > + if (td.cbp > td.be + 1) { >> > + trace_usb_ohci_td_bad_buf(td.cbp, 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" >> > >> >
