On Mon, May 20, 2024 at 12:05 PM Peter Maydell <peter.mayd...@linaro.org> wrote:
> On Tue, 6 Feb 2024 at 13:25, Cord Amfmgm <dmamf...@gmail.com> 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. > > For the "zero length buffer" case, do you have a more detailed > pointer to the bit of the spec that says that "cbp = be + 1" is a > valid way to specify a zero length buffer? Table 4-2 in the copy I > have says for CurrentBufferPointer "a value of 0 indicates > a zero-length data packet or that all bytes have been transferred", > and the sample host OS driver function QueueGeneralRequest() > later in the spec handles a 0 length packet by setting > TD->HcTD.CBP = TD->HcTD.BE = 0; > (which our emulation's code does handle). > Reading the spec carefully, a CBP set to 0 should always mean the zero-length buffer case (or that all bytes have been transferred, so the buffer is now zero-length - the same thing). Table 4-2 is the correct reference, and this part is clear. It's the part you quoted. "Contains the physical address of the next memory location that will be accessed for transfer to/from the endpoint. A value of 0 indicates a zero-length data packet or that all bytes have been transferred." Table 4-2 has this additional nugget that may be confusingly worded, for BufferEnd: "Contains physical address of the last byte in the buffer for this TD" As you say, QueueGeneralRequest() handles a 0 length packet by setting CBP = BE = 0. There's a little bit more right below Table 4-2 in section 4.3.1.3.1: "The CurrentBufferPointer value in the General TD is the address of the data buffer that will be used for a data packet transfer to/from the endpoint addressed by the ED. When the transfer is completed without an error of any kind, the Host Controller advances the value of CurrentBufferPointer by the number of bytes transferred" I'll put it in the context of an example buffer of length 8. Perhaps this is the easiest answer about Table 4-2's BufferEnd definition... char buf[8]; char * CurrentBufferPointer = &buf[0]; char * BufferEnd = &buf[7]; // "address of the last byte in the buffer" // The OHCI Host Controller than advances CurrentBufferPointer like this: CurrentBufferPointer += 8 // After the transfer: // CurrentBufferPointer = &buf[8]; // BufferEnd = &buf[7]; And here's an example buffer of length 0 -- you probably already know what I'm going to do here: char buf[0]; char * CurrentBufferPointer = &buf[0]; char * BufferEnd = &buf[-1]; // "address of the last byte in the buffer" // The OHCI Host Controller than advances CurrentBufferPointer like this: CurrentBufferPointer += 0 // After the transfer: // CurrentBufferPointer = &buf[0]; // BufferEnd = &buf[-1]; > > @@ -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) { > > I think this has an overflow if td.be is 0xffffffff. > Opps, yes. I will submit a revised patch. Since this change is protected inside a condition if (td.cbp && td.be) I plan to rewrite it as: if (td.cbp - 1 > td.be) { // rely on td.cbp != 0 > > > + trace_usb_ohci_td_bad_buf(td.cbp, td.be); > > ohci_die(ohci); > > return 1; > > } > > (On the other hand having looked at the code I'm happy > now that having a len of 0 passed into usb_packet_addbuf() > is OK because we already do that for the "cbp = be = 0" > way of specifying a zero length packet.) I came to the same conclusion. The zero length packet is already being passed into usb_packet_addbuf(), so this change should be ok.
