Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Fri, Jun 21, 2024 at 10:21 AM Peter Maydell wrote: > On Wed, 12 Jun 2024 at 20:36, Alex Bennée wrote: > > > > Cord Amfmgm writes: > > > > > On Wed, Jun 12, 2024 at 9:21 AM Alex Bennée > wrote: > > > > > > David Hubbard writes: > > > > > > > From: Cord Amfmgm > > > > > > > > This changes the way the ohci emulation handles a Transfer > Descriptor with > > > > "Current Buffer Pointer" set to "Buffer End" + 1. > > > > > > > > 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. Currently qemu only accepts > zero-length > > > > Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI > hardware > > > > accepts both cases. > > > > > > > > The qemu ohci emulation has a regression in ohci_service_td. > Version 4.2 > > > > and earlier matched the spec. (I haven't taken the time to bisect > exactly > > > > where the logic was changed.) > > > > > > I find it hard to characterise this as a regression because we've > > > basically gone from no checks to actually doing bounds checks: > > > > > >1328fe0c32 (hw: usb: hcd-ohci: check len and frame_number variables) > > > > > > The argument here seems to be that real hardware is laxer than the > specs > > > in what it accepts. > > > > > > > > > > > With the updated commit message: > > > > > > Reviewed-by: Alex Bennée > > > > > > Please forgive my lack of experience on this mailing list. I don't see > a suggested commit message from Alex but in case that > > > was what is being considered, here is one. Feedback welcome, also if > this is not what is wanted, please just say it. > > > > > > > Something along the lines of what you suggest here > > Thanks; I've picked up this patch for target-arm.next (as with > your previous one for hcd-ohci, adjusting the Author and > Signed-off-by lines to both read David Hubbard). > > I tweaked the commit message a little bit, so the middle part reads: > > What this patch does is loosen the qemu ohci implementation to allow a > zero-length packet if td.be (Buffer End) is set to td.cbp - 1, and > with a > non-zero td.cbp value. > > The spec is unclear whether this is valid or not -- it is not the > clearly documented way to send a zero length TD (which is CBP=BE=0), > but it isn't specifically forbidden. Actual hw seems to be ok with it. > > thanks > -- PMM > That tweak looks great. Thank you for your patience working with me to get this patch into a good shape. This was my first attempt to contribute to qemu - really appreciate it.
Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Wed, Jun 12, 2024 at 2:36 PM Alex Bennée wrote:
> Cord Amfmgm writes:
>
> > On Wed, Jun 12, 2024 at 9:21 AM Alex Bennée
> wrote:
> >
> > David Hubbard writes:
> >
> > > From: Cord Amfmgm
> > >
> > > This changes the way the ohci emulation handles a Transfer Descriptor
> with
> > > "Current Buffer Pointer" set to "Buffer End" + 1.
> > >
> > > 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. Currently qemu only accepts
> zero-length
> > > Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI
> hardware
> > > accepts both cases.
> > >
> > > The qemu ohci emulation has a regression in ohci_service_td. Version
> 4.2
> > > and earlier matched the spec. (I haven't taken the time to bisect
> exactly
> > > where the logic was changed.)
> >
> > I find it hard to characterise this as a regression because we've
> > basically gone from no checks to actually doing bounds checks:
> >
> >1328fe0c32 (hw: usb: hcd-ohci: check len and frame_number variables)
> >
> > The argument here seems to be that real hardware is laxer than the specs
> > in what it accepts.
> >
>
> >
> > With the updated commit message:
> >
> > Reviewed-by: Alex Bennée
> >
> > Please forgive my lack of experience on this mailing list. I don't see a
> suggested commit message from Alex but in case that
> > was what is being considered, here is one. Feedback welcome, also if
> this is not what is wanted, please just say it.
> >
>
> Something along the lines of what you suggest here (where did this come
> from?)
>
> >> From: Cord Amfmgm
> >>
> >> This changes the way the ohci emulation handles a Transfer Descriptor
> with
> >> "Buffer End" set to "Current Buffer Pointer" - 1, specifically in the
> case of a
> >> zero-length packet.
> >>
> >> The OHCI spec 4.3.1.2 Table 4-2 specifies td.cbp to be zero for a
> zero-length
> >> packet. Peter Maydell tracked down commit
> >> 1328fe0c32 (hw: usb: hcd-ohci: check len and frame_number variables)
> >> where qemu started checking this according to the spec.
> >>
> >> What this patch does is loosen the qemu ohci implementation to allow a
> >> zero-length packet if td.be (Buffer End) is set to td.cbp - 1, and
> with a
> >> non-zero td.cbp value.
> >>
> >> Is this correct? It appears to not follow the spec, so no. But actual hw
> >> seems to be ok with it.
> >>
> >> Does any OS rely on this behavior? There have been no reports to
> >> qemu-devel of this problem.
> >>
> >> This is attempting to have qemu behave like actual hardware,
> >> but this is just a minor change.
> >>
> >> With a tiny OS[1] that boots and executes a test, the behavior is
> >> reproducible:
> >>
> >> * OS that sends USB requests to a USB mass storage device
> >> but sends td.be = td.cbp - 1
> >> * qemu 4.2
> >> * qemu HEAD (4e66a0854)
> >> * Actual OHCI controller (hardware)
> >>
> >> Command line:
> >> qemu-system-x86_64 -m 20 \
> >> -device pci-ohci,id=ohci \
> >> -drive if=none,format=raw,id=d,file=testmbr.raw \
> >> -device usb-storage,bus=ohci.0,drive=d \
> >> --trace "usb_*" --trace "ohci_*" -D qemu.log
> >>
> >> Results are:
> >>
> >> qemu 4.2 | qemu HEAD | actual HW
> >> ++
> >> works fine | ohci_die() | works fine
> >>
> >> Tip: if the flags "-serial pty -serial stdio" are added to the command
> line
> >> the test will output USB requests like this:
> >>
> >> Testing qemu HEAD:
> >>
> >>> Free mem 2M ohci port2 conn FS
> >>> setup { 80 6 0 1 0 0 8 0 }
> >>> ED info=8 { mps=8 en=0 d=0 } tail=c20920
> >>> td0 c20880 nxt=c20960 f200 setup cbp=c20900 be=c20907
> >>> td1 c20960 nxt=c20980 f314in cbp=c20908 be=c2090f
> >>> td2 c20980 nxt=c20920 f308 out cbp=c20910 be=c2090f ohci20
> host err
> >>> usb stopped
> >>
> >> And in qemu.log:
> >>
> >> usb_ohci_iso_td_bad_cc_overrun ISO_TD start_offset=0x00c20910 >
> next_offset=0x00c2090f
> >>
> >> Testing qemu 4.2:
> >
Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Wed, Jun 12, 2024 at 9:21 AM Alex Bennée wrote:
> David Hubbard writes:
>
> > From: Cord Amfmgm
> >
> > This changes the way the ohci emulation handles a Transfer Descriptor
> with
> > "Current Buffer Pointer" set to "Buffer End" + 1.
> >
> > 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. Currently qemu only accepts
> zero-length
> > Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI
> hardware
> > accepts both cases.
> >
> > The qemu ohci emulation has a regression in ohci_service_td. Version 4.2
> > and earlier matched the spec. (I haven't taken the time to bisect exactly
> > where the logic was changed.)
>
> I find it hard to characterise this as a regression because we've
> basically gone from no checks to actually doing bounds checks:
>
> 1328fe0c32 (hw: usb: hcd-ohci: check len and frame_number variables)
>
> The argument here seems to be that real hardware is laxer than the specs
> in what it accepts.
>
> > With a tiny OS[1] that boots and executes a test, the issue can be seen:
> >
> > * OS that sends USB requests to a USB mass storage device
> > but sends td.cbp = td.be + 1
> > * qemu 4.2
> > * qemu HEAD (4e66a0854)
> > * Actual OHCI controller (hardware)
> >
> > Command line:
> > qemu-system-x86_64 -m 20 \
> > -device pci-ohci,id=ohci \
> > -drive if=none,format=raw,id=d,file=testmbr.raw \
> > -device usb-storage,bus=ohci.0,drive=d \
> > --trace "usb_*" --trace "ohci_*" -D qemu.log
> >
> > Results are:
> >
> > qemu 4.2 | qemu HEAD | actual HW
> > ++
> > works fine | ohci_die() | works fine
> >
> > Tip: if the flags "-serial pty -serial stdio" are added to the command
> line
> > the test will output USB requests like this:
> >
> > Testing qemu HEAD:
> >
> >> Free mem 2M ohci port2 conn FS
> >> setup { 80 6 0 1 0 0 8 0 }
> >> ED info=8 { mps=8 en=0 d=0 } tail=c20920
> >> td0 c20880 nxt=c20960 f200 setup cbp=c20900 be=c20907
> >> td1 c20960 nxt=c20980 f314in cbp=c20908 be=c2090f
> >> td2 c20980 nxt=c20920 f308 out cbp=c20910 be=c2090f ohci20 host
> err
> >> usb stopped
> >
> > And in qemu.log:
> >
> > usb_ohci_iso_td_bad_cc_overrun ISO_TD start_offset=0x00c20910 >
> next_offset=0x00c2090f
> >
> > Testing qemu 4.2:
> >
> >> Free mem 2M ohci port2 conn FS
> >> setup { 80 6 0 1 0 0 8 0 }
> >> ED info=8 { mps=8 en=0 d=0 } tail=620920
> >> td0 620880 nxt=620960 f200 setup cbp=620900 be=620907 cbp=0
> be=620907
> >> td1 620960 nxt=620980 f314in cbp=620908 be=62090f cbp=0
> be=62090f
> >> td2 620980 nxt=620920 f308 out cbp=620910 be=62090f cbp=0
> be=62090f
> >>rx { 12 1 0 2 0 0 0 8 }
> >> setup { 0 5 1 0 0 0 0 0 } tx {}
> >> ED info=8 { mps=8 en=0 d=0 } tail=620880
> >> td0 620920 nxt=620960 f200 setup cbp=620900 be=620907 cbp=0
> be=620907
> >> td1 620960 nxt=620880 f310in cbp=620908 be=620907 cbp=0
> be=620907
> >> setup { 80 6 0 1 0 0 12 0 }
> >> ED info=80001 { mps=8 en=0 d=1 } tail=620960
> >> td0 620880 nxt=6209c0 f200 setup cbp=620920 be=620927 cbp=0
> be=620927
> >> td1 6209c0 nxt=6209e0 f314in cbp=620928 be=620939 cbp=0
> be=620939
> >> td2 6209e0 nxt=620960 f308 out cbp=62093a be=620939 cbp=0
> be=620939
> >>rx { 12 1 0 2 0 0 0 8 f4 46 1 0 0 0 1 2 3 1 }
> >> setup { 80 6 0 2 0 0 0 1 }
> >> ED info=80001 { mps=8 en=0 d=1 } tail=620880
> >> td0 620960 nxt=6209a0 f200 setup cbp=620a20 be=620a27 cbp=0
> be=620a27
> >> td1 6209a0 nxt=6209c0 f3140004in cbp=620a28 be=620b27
> cbp=620a48 be=620b27
> >> td2 6209c0 nxt=620880 f308 out cbp=620b28 be=620b27 cbp=0
> be=620b27
> >>rx { 9 2 20 0 1 1 4 c0 0 9 4 0 0 2 8 6 50 0 7 5 81 2 40 0 0 7 5 2 2
> 40 0 0 }
> >> setup { 0 9 1 0 0 0 0 0 } tx {}
> >> ED info=80001 { mps=8 en=0 d=1 } tail=620900
> >> td0 620880 nxt=620940 f200 setup cbp=620a00 be=620a07 cbp=0
> be=620a07
> >> td1 620940 nxt=620900 f310in cbp=620a08 be=620a07 cbp=0
> be=620a07
> >
> > [1] The OS disk image has been emailed to phi...@linaro.org,
>
Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Fri, Jun 7, 2024 at 8:23 AM Peter Maydell wrote: > On Fri, 31 May 2024 at 19:16, Cord Amfmgm wrote: > > On Fri, May 31, 2024 at 9:03 AM Peter Maydell > wrote: > >> What I would like to see is what we could classify under > >> "rationale", which is to say "what prompted us to make this > >> change?". In my experience it's important to record this > >> (including in the commit message). There are of course > >> many cases in QEMU's git history where we failed to do that, > >> but in general I think it's a good standard to meet. (I > >> am also erring on the side of caution in reviewing this > >> particular patch, because I don't know the relevant standards > >> or this bit of the code very well.) > > > Thanks, in responding to that, I'm breaking down my responses into the > following answers: > > > > Q1: (summarizing) What prompted us to make this change? > > > > A1: I'm summarizing what I wrote in previous emails and the commit > message - > > > > * Bring qemu closer to actual hw with a neatly packaged test case to > demonstrate the behavior > > * I interpret the spec (Table 4-2) as saying the "be = cbp - 1" is > valid, in addition to setting "cbp = 0" > > * I interpret it that way due to a comment in an old linux kernel > version in the 2.x range, ohci-hcd.c file. It said (paraphrasing) some > misbehaving ohci controllers would fetch physical memory at 0 when cbp = 0 > was in the Transfer Descriptor > > Interesting. Do you have a more specific version for that? > I had a look at various 2.x Linux OHCI drivers but they all seem to do > zero-length packets "by the spec" with CBP=BE=0. eg 2.6.39.4: > > https://elixir.bootlin.com/linux/v2.6.39.4/source/drivers/usb/host/ohci-q.c#L539 > and there's no hardware-quirk handling to do it differently on > some controllers. (The USB OHCI driver seems to have gone through > a couple of rewrites in the 2.x kernel timeframe; the earlier 2.3.51 > version does the TD fill here: > https://elixir.bootlin.com/linux/2.3.51/source/drivers/usb/usb-ohci.c#L812 > and again it handles zero length as BE=CBP=0.) > > > But I only have a test case I created myself, and am not an expert on > computer history here. I think "be liberal in what you accept, by > conservative in what you send" applies when I don't know which historical > OSes, if any, need this behavior. I think the behavior of actual hardware > weighs more heavily since that *is* available and testable. Are there > additional checks that would expand on what's known about actual ohci hw? > > The other side of the argument is "if in doubt and you don't > know of any concrete problem being caused, don't change > working code". If there are any historical OSes that rely on > being able to send zero packets with be=cbp-1 for some nonzero > be, and anybody wants to run them on QEMU, then presumably they > can send us a bug report saying "XYZ's USB support doesn't work". > That nobody has ever does this is evidence on the side of > "there is no such OS out there, everybody writing an OHCI driver > read the spec and made their driver send zero length packets the > way the spec very clearly says you should". Please correct me > if I'm wrong, but my interpretation of your helpful explanation > above is that this is essentially a theoretical problem rather > than one that's caused you a problem you need to fix. > I think that's fair. I sent in the patch more out of a desire for qemu to have the greatest possible ohci implementation, than due to knowledge of an actual OS that couldn't work. Up to you what to do from here.
Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Fri, May 31, 2024 at 9:03 AM Peter Maydell
wrote:
> On Tue, 21 May 2024 at 00:26, David Hubbard wrote:
> >
> > From: Cord Amfmgm
> >
> > This changes the way the ohci emulation handles a Transfer Descriptor
> with
> > "Current Buffer Pointer" set to "Buffer End" + 1.
> >
> > 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. Currently qemu only accepts
> zero-length
> > Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI
> hardware
> > accepts both cases.
> >
> > The qemu ohci emulation has a regression in ohci_service_td. Version 4.2
> > and earlier matched the spec. (I haven't taken the time to bisect exactly
> > where the logic was changed.)
> >
> > With a tiny OS[1] that boots and executes a test, the issue can be seen:
> >
> > * OS that sends USB requests to a USB mass storage device
> > but sends td.cbp = td.be + 1
> > * qemu 4.2
> > * qemu HEAD (4e66a0854)
> > * Actual OHCI controller (hardware)
> >
> > Command line:
> > qemu-system-x86_64 -m 20 \
> > -device pci-ohci,id=ohci \
> > -drive if=none,format=raw,id=d,file=testmbr.raw \
> > -device usb-storage,bus=ohci.0,drive=d \
> > --trace "usb_*" --trace "ohci_*" -D qemu.log
> >
> > Results are:
> >
> > qemu 4.2 | qemu HEAD | actual HW
> > ++
> > works fine | ohci_die() | works fine
> >
> > Tip: if the flags "-serial pty -serial stdio" are added to the command
> line
> > the test will output USB requests like this:
> >
> > Testing qemu HEAD:
> >
> > > Free mem 2M ohci port2 conn FS
> > > setup { 80 6 0 1 0 0 8 0 }
> > > ED info=8 { mps=8 en=0 d=0 } tail=c20920
> > > td0 c20880 nxt=c20960 f200 setup cbp=c20900 be=c20907
> > > td1 c20960 nxt=c20980 f314in cbp=c20908 be=c2090f
> > > td2 c20980 nxt=c20920 f308 out cbp=c20910 be=c2090f ohci20
> host err
> > > usb stopped
> >
> > And in qemu.log:
> >
> > usb_ohci_iso_td_bad_cc_overrun ISO_TD start_offset=0x00c20910 >
> next_offset=0x00c2090f
> >
> > Testing qemu 4.2:
> >
> > > Free mem 2M ohci port2 conn FS
> > > setup { 80 6 0 1 0 0 8 0 }
> > > ED info=8 { mps=8 en=0 d=0 } tail=620920
> > > td0 620880 nxt=620960 f200 setup cbp=620900 be=620907
> cbp=0 be=620907
> > > td1 620960 nxt=620980 f314in cbp=620908 be=62090f
> cbp=0 be=62090f
> > > td2 620980 nxt=620920 f308 out cbp=620910 be=62090f
> cbp=0 be=62090f
> > >rx { 12 1 0 2 0 0 0 8 }
> > > setup { 0 5 1 0 0 0 0 0 } tx {}
> > > ED info=8 { mps=8 en=0 d=0 } tail=620880
> > > td0 620920 nxt=620960 f200 setup cbp=620900 be=620907
> cbp=0 be=620907
> > > td1 620960 nxt=620880 f310in cbp=620908 be=620907
> cbp=0 be=620907
> > > setup { 80 6 0 1 0 0 12 0 }
> > > ED info=80001 { mps=8 en=0 d=1 } tail=620960
> > > td0 620880 nxt=6209c0 f200 setup cbp=620920 be=620927
> cbp=0 be=620927
> > > td1 6209c0 nxt=6209e0 f314in cbp=620928 be=620939
> cbp=0 be=620939
> > > td2 6209e0 nxt=620960 f308 out cbp=62093a be=620939
> cbp=0 be=620939
> > >rx { 12 1 0 2 0 0 0 8 f4 46 1 0 0 0 1 2 3 1 }
> > > setup { 80 6 0 2 0 0 0 1 }
> > > ED info=80001 { mps=8 en=0 d=1 } tail=620880
> > > td0 620960 nxt=6209a0 f200 setup cbp=620a20 be=620a27
> cbp=0 be=620a27
> > > td1 6209a0 nxt=6209c0 f3140004in cbp=620a28 be=620b27
> cbp=620a48 be=620b27
> > > td2 6209c0 nxt=620880 f308 out cbp=620b28 be=620b27
> cbp=0 be=620b27
> > >rx { 9 2 20 0 1 1 4 c0 0 9 4 0 0 2 8 6 50 0 7 5 81 2 40 0 0 7 5 2 2
> 40 0 0 }
> > > setup { 0 9 1 0 0 0 0 0 } tx {}
> > > ED info=80001 { mps=8 en=0 d=1 } tail=620900
> > > td0 620880 nxt=620940 f200 setup cbp=620a00 be=620a07
> cbp=0 be=620a07
> > > td1 620940 nxt=620900 f310in cbp=620a08 be=620a07
> cbp=0 be=620a07
> >
> > [1] The OS disk image has been emailed to phi...@linaro.org,
> m...@tls.msk.ru,
> > and kra...@redhat.com:
> >
> > * testCbpOffBy1.img.xz
> > * sha256:
> f87baddcb86de845de12f002c698670a426affb40946025cc32694f9daa3abed
> >
> > Signed-off-by: Cord Amfmgm
> > ---
> > hw/usb/hcd-ohci.c | 4 ++--
> > hw/usb/trace-events | 1 +
> > 2 files changed, 3 insertions(+), 2
Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Thu, May 30, 2024 at 2:14 PM Alex Bennée wrote: > David Hubbard writes: > > > From: Cord Amfmgm > > > > This changes the way the ohci emulation handles a Transfer Descriptor > with > > "Current Buffer Pointer" set to "Buffer End" + 1. > > > > 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. Currently qemu only accepts > zero-length > > Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI > hardware > > accepts both cases. > > Which version of the OHCI spec is this? I can't find it in the one copy > Google throws up: > > > http://download.microsoft.com/download/1/6/1/161ba512-40e2-4cc9-843a-923143f3456c/ohci_11.pdf > > Replace http with https in that URL and it downloads the PDF OK - it is for IEEE-1394 Firewire though. Try this link: https://www.cs.usfca.edu/~cruse/cs698s10/hcir1_0a.pdf - I am on page 35/160 (the page is numbered "21" on the bottom) for the Table 4-2.
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Thu, May 30, 2024 at 2:12 PM Alex Bennée wrote: > Cord Amfmgm writes: > > > On Thu, May 30, 2024 at 3:33 AM Alex Bennée > wrote: > > > > Cord Amfmgm writes: > > > > > On Tue, May 28, 2024 at 11:32 AM Peter Maydell < > peter.mayd...@linaro.org> wrote: > > > > > > On Tue, 28 May 2024 at 16:37, Cord Amfmgm > wrote: > > > > > > > > On Tue, May 28, 2024 at 9:03 AM Peter Maydell < > peter.mayd...@linaro.org> wrote: > > > >> > > > >> On Mon, 20 May 2024 at 23:24, Cord Amfmgm > wrote: > > > >> > On Mon, May 20, 2024 at 12:05 PM Peter Maydell < > peter.mayd...@linaro.org> wrote: > > > > > >> > 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]; > > > >> > > > >> Right, but why do you think this is valid, rather than > > > >> being a guest software bug? My reading of the spec is that it's > > > >> pretty clear about how to say "zero length buffer", and this > > > >> isn't it. > > > >> > > > >> Is there some real-world guest OS that programs the OHCI > > > >> controller this way that we're trying to accommodate? > > > > > > > > > > > > qemu versions 4.2 and before allowed this behavior. > > > > > > So? That might just mean we had a bug and we fixed it. > > > 4.2 is a very old version of QEMU and nobody seems to have > > > complained in the four years since we released 5.0 about this, > > > which suggests that generally guest OS drivers don't try > > > to send zero-length buffers in this way. > > > > > > > I don't think it's valid to ask for a *popular* guest OS as a > proof-of-concept because I'm not an expert on those. > > > > > > I didn't ask for "popular"; I asked for "real-world". > > > What is the actual guest code you're running that falls over > > > because of the behaviour change? > > > > > > More generally, why do you want this behaviour to be > > > changed? Reasonable reasons might include: > > > * we're out of spec based on reading the documentation > > > * you're trying to run some old Windows VM/QNX/etc image, > > > and it doesn't work any more > > > * all the real hardware we tested behaves this way > > > > > > But don't necessarily include: > > > * something somebody wrote and only tested on QEMU happens to > > > assume the old behaviour rather than following the hw spec > > > > > > QEMU occasionally works around guest OS bugs, but only as > > > when we really have to. It's usually better to fix the > > > bug in the guest. > > > > > > It's not, and I've already demonstrated that real hardware is > consistent with the fix in this patch. > > > > > > Please check your tone. > > > > I don't think that is a particularly helpful comment for someone who is > > taking the time to review your patches. Reading through the thread I > > didn't see anything that said this is how real HW behaves but I may well > > have missed it. However you have a number of review comments to address > > so I suggest you spin a v2 of the series to address them and outline the > > reason to accept an out of spec transaction. > > > > I did a rework of the patch -- see my email from May 20, quoted below -- > and I was under the impression it addressed all the > > review comments. Did I miss something? I apologize if I did. > > Ahh I see - I'd only seen this thread continue so wasn't aware a new > version had been posted. For future patches consider using -vN when > sending them so we can clearly see a new revision is available. > > > > >
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Thu, May 30, 2024 at 3:33 AM Alex Bennée wrote:
> Cord Amfmgm writes:
>
> > On Tue, May 28, 2024 at 11:32 AM Peter Maydell
> wrote:
> >
> > On Tue, 28 May 2024 at 16:37, Cord Amfmgm wrote:
> > >
> > > On Tue, May 28, 2024 at 9:03 AM Peter Maydell <
> peter.mayd...@linaro.org> wrote:
> > >>
> > >> On Mon, 20 May 2024 at 23:24, Cord Amfmgm
> wrote:
> > >> > On Mon, May 20, 2024 at 12:05 PM Peter Maydell <
> peter.mayd...@linaro.org> wrote:
>
> > >> > 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];
> > >>
> > >> Right, but why do you think this is valid, rather than
> > >> being a guest software bug? My reading of the spec is that it's
> > >> pretty clear about how to say "zero length buffer", and this
> > >> isn't it.
> > >>
> > >> Is there some real-world guest OS that programs the OHCI
> > >> controller this way that we're trying to accommodate?
> > >
> > >
> > > qemu versions 4.2 and before allowed this behavior.
> >
> > So? That might just mean we had a bug and we fixed it.
> > 4.2 is a very old version of QEMU and nobody seems to have
> > complained in the four years since we released 5.0 about this,
> > which suggests that generally guest OS drivers don't try
> > to send zero-length buffers in this way.
> >
> > > I don't think it's valid to ask for a *popular* guest OS as a
> proof-of-concept because I'm not an expert on those.
> >
> > I didn't ask for "popular"; I asked for "real-world".
> > What is the actual guest code you're running that falls over
> > because of the behaviour change?
> >
> > More generally, why do you want this behaviour to be
> > changed? Reasonable reasons might include:
> > * we're out of spec based on reading the documentation
> > * you're trying to run some old Windows VM/QNX/etc image,
> > and it doesn't work any more
> > * all the real hardware we tested behaves this way
> >
> > But don't necessarily include:
> > * something somebody wrote and only tested on QEMU happens to
> > assume the old behaviour rather than following the hw spec
> >
> > QEMU occasionally works around guest OS bugs, but only as
> > when we really have to. It's usually better to fix the
> > bug in the guest.
> >
> > It's not, and I've already demonstrated that real hardware is consistent
> with the fix in this patch.
> >
> > Please check your tone.
>
> I don't think that is a particularly helpful comment for someone who is
> taking the time to review your patches. Reading through the thread I
> didn't see anything that said this is how real HW behaves but I may well
> have missed it. However you have a number of review comments to address
> so I suggest you spin a v2 of the series to address them and outline the
> reason to accept an out of spec transaction.
>
>
I did a rework of the patch -- see my email from May 20, quoted below --
and I was under the impression it addressed all the review comments. Did I
miss something? I apologize if I did.
> index acd6016980..71b54914d3 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -941,8 +941,8 @@ static int ohci_service_td(OHCIState *ohci, struct
ohci_ed *ed)
> if ((td.cbp & 0xf000) != (td.be & 0xf000)) {
> 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 - 1 > td.be) { /* rely on td.cbp != 0 */
> Reading through the thread I didn't see anything that said this is how
real HW behaves but I may well have missed it.
This is what I wrote regarding real HW:
Results are:
qemu 4.2 | qemu HEAD | actual HW
++
works fine | ohci_di
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Tue, May 28, 2024 at 11:32 AM Peter Maydell wrote: > On Tue, 28 May 2024 at 16:37, Cord Amfmgm wrote: > > > > > > > > On Tue, May 28, 2024 at 9:03 AM Peter Maydell > wrote: > >> > >> On Mon, 20 May 2024 at 23:24, Cord Amfmgm wrote: > >> > On Mon, May 20, 2024 at 12:05 PM Peter Maydell < > peter.mayd...@linaro.org> wrote: > >> >> 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). > >> > >> Yeah, I can see the argument for "we should treat all cbp == 0 as > >> zero-length buffer, not just cbp == be == 0". > >> > >> > 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" > >> > >> Mmm, but for a zero length buffer there is no "last byte" to > >> have this be the physical address of. > >> > >> > 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]; > >> > >> Right, but why do you think this is valid, rather than > >> being a guest software bug? My reading of the spec is that it's > >> pretty clear about how to say "zero length buffer", and this > >> isn't it. > >> > >> Is there some real-world guest OS that programs the OHCI > >> controller this way that we're trying to accommodate? > > > > > > qemu versions 4.2 and before allowed this behavior. > > So? That might just mean we had a bug and we fixed it. > 4.2 is a very old version of QEMU and nobody seems to have > complained in the four years since we released 5.0 about this, > which suggests that generally guest OS drivers don't try > to send zero-length buffers in this way. > > > I don't think it's valid to ask for a *popular* guest OS as a > proof-of-concept because I'm not an expert on those. > > I didn't ask for "popular"; I asked for "real-world". > What is the actual guest code you're running that falls over > because of the behaviour change? > > More generally, why do you want this behaviour to be > changed? Reasonable reasons might include: > * we're out of spec based on reading the documentation > * you're trying to run some old Windows VM/QNX/etc image, >and it doesn't work any more > * all the real hardware we tested behaves this way > > But don't necessarily include: > * something somebody wrote and only tested on QEMU happens to >assume the old behaviour rather than following the hw spec > > QEMU occasionally works around guest OS bugs, but only as > when we really have to. It's usually better to fix the > bug in the guest. > It's not, and I've already demonstrated that real hardware is consistent with the fix in this patch. Please check your tone. > > thanks > -- PMM >
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Tue, May 28, 2024 at 9:03 AM Peter Maydell wrote: > On Mon, 20 May 2024 at 23:24, Cord Amfmgm wrote: > > On Mon, May 20, 2024 at 12:05 PM Peter Maydell > wrote: > >> 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). > > Yeah, I can see the argument for "we should treat all cbp == 0 as > zero-length buffer, not just cbp == be == 0". > > > 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" > > Mmm, but for a zero length buffer there is no "last byte" to > have this be the physical address of. > > > 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]; > > Right, but why do you think this is valid, rather than > being a guest software bug? My reading of the spec is that it's > pretty clear about how to say "zero length buffer", and this > isn't it. > > Is there some real-world guest OS that programs the OHCI > controller this way that we're trying to accommodate? > qemu versions 4.2 and before allowed this behavior. I don't think it's valid to ask for a *popular* guest OS as a proof-of-concept because I'm not an expert on those. The spec says "last byte" which can (not "should" just "can") be interpreted as "cbp - 1". Therefore, I raised this patch request to re-enable the previous qemu behavior, in the sense of ""be conservative in what you do, be liberal in what you accept from others" - https://en.wikipedia.org/wiki/Robustness_principle It is *not* valid to say the spec disallows "cbp - 1" to mean "empty buffer." That is what I am arguing :) > > thanks > -- PMM >
Re: [PATCH] hw/usb/hcd-ohci: Fix ohci_service_td: accept valid TDs
On Mon, May 20, 2024 at 6:24 PM David Hubbard wrote:
> From: Cord Amfmgm
>
> This changes the way the ohci emulation handles a Transfer Descriptor with
> "Current Buffer Pointer" set to "Buffer End" + 1.
>
Please disregard, this patch is no different from the previous one sent a
couple weeks ago. Resending...
>
> 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. Currently qemu only accepts
> zero-length
> Transfer Descriptors if the td.cbp is equal to 0, while actual OHCI
> hardware
> accepts both cases.
>
> The qemu ohci emulation has a regression in ohci_service_td. Version 4.2
> and earlier matched the spec. (I haven't taken the time to bisect exactly
> where the logic was changed.)
>
> With a tiny OS[1] that boots and executes a test, the issue can be seen:
>
> * OS that sends USB requests to a USB mass storage device
> but sends td.cbp = td.be + 1
> * qemu 4.2
> * qemu HEAD (4e66a0854)
> * Actual OHCI controller (hardware)
>
> Command line:
> qemu-system-x86_64 -m 20 \
> -device pci-ohci,id=ohci \
> -drive if=none,format=raw,id=d,file=testmbr.raw \
> -device usb-storage,bus=ohci.0,drive=d \
> --trace "usb_*" --trace "ohci_*" -D qemu.log
>
> Results are:
>
> qemu 4.2 | qemu HEAD | actual HW
> ++
> works fine | ohci_die() | works fine
>
> Tip: if the flags "-serial pty -serial stdio" are added to the command line
> the test will output USB requests like this:
>
> Testing qemu HEAD:
>
> > Free mem 2M ohci port2 conn FS
> > setup { 80 6 0 1 0 0 8 0 }
> > ED info=8 { mps=8 en=0 d=0 } tail=c20920
> > td0 c20880 nxt=c20960 f200 setup cbp=c20900 be=c20907
> > td1 c20960 nxt=c20980 f314in cbp=c20908 be=c2090f
> > td2 c20980 nxt=c20920 f308 out cbp=c20910 be=c2090f ohci20 host
> err
> > usb stopped
>
> And in qemu.log:
>
> usb_ohci_iso_td_bad_cc_overrun ISO_TD start_offset=0x00c20910 >
> next_offset=0x00c2090f
>
> Testing qemu 4.2:
>
> > Free mem 2M ohci port2 conn FS
> > setup { 80 6 0 1 0 0 8 0 }
> > ED info=8 { mps=8 en=0 d=0 } tail=620920
> > td0 620880 nxt=620960 f200 setup cbp=620900 be=620907 cbp=0
> be=620907
> > td1 620960 nxt=620980 f314in cbp=620908 be=62090f cbp=0
> be=62090f
> > td2 620980 nxt=620920 f308 out cbp=620910 be=62090f cbp=0
> be=62090f
> >rx { 12 1 0 2 0 0 0 8 }
> > setup { 0 5 1 0 0 0 0 0 } tx {}
> > ED info=8 { mps=8 en=0 d=0 } tail=620880
> > td0 620920 nxt=620960 f200 setup cbp=620900 be=620907 cbp=0
> be=620907
> > td1 620960 nxt=620880 f310in cbp=620908 be=620907 cbp=0
> be=620907
> > setup { 80 6 0 1 0 0 12 0 }
> > ED info=80001 { mps=8 en=0 d=1 } tail=620960
> > td0 620880 nxt=6209c0 f200 setup cbp=620920 be=620927 cbp=0
> be=620927
> > td1 6209c0 nxt=6209e0 f314in cbp=620928 be=620939 cbp=0
> be=620939
> > td2 6209e0 nxt=620960 f308 out cbp=62093a be=620939 cbp=0
> be=620939
> >rx { 12 1 0 2 0 0 0 8 f4 46 1 0 0 0 1 2 3 1 }
> > setup { 80 6 0 2 0 0 0 1 }
> > ED info=80001 { mps=8 en=0 d=1 } tail=620880
> > td0 620960 nxt=6209a0 f200 setup cbp=620a20 be=620a27 cbp=0
> be=620a27
> > td1 6209a0 nxt=6209c0 f3140004in cbp=620a28 be=620b27
> cbp=620a48 be=620b27
> > td2 6209c0 nxt=620880 f308 out cbp=620b28 be=620b27 cbp=0
> be=620b27
> >rx { 9 2 20 0 1 1 4 c0 0 9 4 0 0 2 8 6 50 0 7 5 81 2 40 0 0 7 5 2 2
> 40 0 0 }
> > setup { 0 9 1 0 0 0 0 0 } tx {}
> > ED info=80001 { mps=8 en=0 d=1 } tail=620900
> > td0 620880 nxt=620940 f200 setup cbp=620a00 be=620a07 cbp=0
> be=620a07
> > td1 620940 nxt=620900 f310in cbp=620a08 be=620a07 cbp=0
> be=620a07
>
> [1] The OS disk image has been emailed to phi...@linaro.org,
> m...@tls.msk.ru,
> and kra...@redhat.com:
>
> * testCbpOffBy1.img.xz
> * sha256: f87baddcb86de845de12f002c698670a426affb40946025cc32694f9daa3abed
>
> Signed-off-by: Cord Amfmgm
> ---
> hw/usb/hcd-ohci.c | 4 ++--
> hw/usb/trace-events | 1 +
> 2 files changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index acd6016980..86caf5e43b 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -941,8 +941,8 @@ static int ohci_service_td(OHCIState *ohci, struct
> ohci_ed *ed)
> if ((td.cbp & 0xf000) != (td.be & 0xf000)) {
>
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Mon, May 20, 2024 at 12:05 PM Peter Maydell
wrote:
> On Tue, 6 Feb 2024 at 13:25, 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]) & 0xfff0)
> >| (/* ToggleCarry= */ 0 << 1);
> > ed->next_ed = (/* NextED= */ 0 & 0xfff0)
> >
> > 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 & 0xf000) != (td.be & 0xf000)) {
> > 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) {
>
>
Re: [PATCH 1/2] hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Mon, May 20, 2024 at 11:55 AM Peter Maydell
wrote:
> On Thu, 9 May 2024 at 01:30, David Hubbard wrote:
> >
> > From: Cord Amfmgm
> >
> > This changes the ohci validation to not assert if invalid data is fed to
> the
> > ohci controller. The poc in https://bugs.launchpad.net/qemu/+bug/1907042
> and
> > migrated to bug #303 does the following to feed it a SETUP pid (valid)
> > at an EndPt of 1 (invalid - all SETUP pids must be addressed to EndPt 0):
> >
> > 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]) & 0xfff0)
> >| (/* ToggleCarry= */ 0 << 1);
> > ed->next_ed = (/* NextED= */ 0 & 0xfff0)
> >
> > qemu-fuzz also caught the same issue in #1510. They are both fixed by
> this
> > patch.
> >
> > With a tiny OS[1] that boots and executes the poc the repro shows the
> issue:
> >
> > * OS that sends USB requests to a USB mass storage device
> > but sends a SETUP with EndPt = 1
> > * qemu 6.2.0 (Debian 1:6.2+dfsg-2ubuntu6.19)
> > * qemu HEAD (4e66a0854)
> > * Actual OHCI controller (hardware)
> >
> > Command line:
> > qemu-system-x86_64 -m 20 \
> > -device pci-ohci,id=ohci \
> > -drive if=none,format=raw,id=d,file=testmbr.raw \
> > -device usb-storage,bus=ohci.0,drive=d \
> > --trace "usb_*" --trace "ohci_*" -D qemu.log
> >
> > Results are:
> >
> > qemu 6.2.0 | qemu HEAD | actual HW
> > +---+
> > assertion | assertion | sets stall bit
> >
> > The assertion message is:
> >
> > > qemu-system-x86_64: ../../hw/usb/core.c:744: usb_ep_get: Assertion
> `pid == USB_TOKEN_IN || pid == USB_TOKEN_OUT' failed.
> > > Aborted (core dumped)
> >
> > Tip: if the flags "-serial pty -serial stdio" are added to the command
> line
> > the poc outputs its USB requests like this:
> >
> > > Free mem 2M ohci port0 conn FS
> > > setup { 80 6 0 1 0 0 8 0 }
> > > ED info=8 { mps=8 en=0 d=0 } tail=c20920
> > > td0 c20880 nxt=c20960 f200 setup cbp=c20900 be=c20907
> cbp=0 be=c20907
> > > td1 c20960 nxt=c20980 f314in cbp=c20908 be=c2090f
> cbp=0 be=c2090f
> > > td2 c20980 nxt=c20920 f308 out cbp=0 be=0
> cbp=0 be=0
> > >rx { 12 1 0 2 0 0 0 8 }
> > > setup { 0 5 1 0 0 0 0 0 } tx {}
> > > ED info=8 { mps=8 en=0 d=0 } tail=c20880
> > > td0 c20920 nxt=c20960 f200 setup cbp=c20900 be=c20907
> cbp=0 be=c20907
> > > td1 c20960 nxt=c20880 f310in cbp=0 be=0
> cbp=0 be=0
> > > setup { 80 6 0 1 0 0 12 0 }
> > > ED info=80081 { mps=8 en=0 d=1 } tail=c20960
> > > td0 c20880 nxt=c209c0 f200 setup cbp=c20920 be=c20927
> > > td1 c209c0 nxt=c209e0 f314in cbp=c20928 be=c20939
> > > td2 c209e0 nxt=c20960 f308 out cbp=0 be=0qemu-system-x86_64:
> ../../hw/usb/core.c:744: usb_ep_get: Assertion `pid == USB_TOKEN_IN || pid
> == USB_TOKEN_OUT' failed.
> > > Aborted (core dumped)
> >
> > [1] The OS disk image has been emailed to phi...@linaro.org,
> m...@tls.msk.ru,
> > and kra...@redhat.com:
> >
> > * testBadSetup.img.xz
> > * sha256:
> 045b43f4396de02b149518358bf8025d5ba11091e86458875339fc649e6e5ac6
> >
> > Signed-off-by: Cord Amfmgm
> > ---
> > hw/usb/hcd-ohci.c | 5 +
> > hw/usb/trace-events | 1 +
> > 2 files changed, 6 insertions(+)
> >
> > diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> > index fc8fc91a1d..acd6016980 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_oh
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Sat, May 11, 2024 at 5:25 AM Peter Maydell wrote: > On Thu, 9 May 2024 at 19:17, Cord Amfmgm wrote: > > > > > > > > On Thu, May 9, 2024 at 12:48 PM Peter Maydell > wrote: > >> > >> On Wed, 8 May 2024 at 16:29, Cord Amfmgm wrote: > >> > On Wed, May 8, 2024 at 3:45 AM Thomas Huth 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 > >> > >> I'm confused now. Of the two names you've used in this > >> patch (Cord Amfmgm and David Hubbard), are they both > >> pseudonyms, or is one a pseudonym and one your real name? > >> > > > > Hi Peter, > > > > I am attempting to submit a small patch. For context, I'm getting > broader attention now because apparently OHCI is one of the less used > components of qemu and maybe the review process was taking a while. That's > relevant because I wasn't able to get prompt feedback and am now choosing > what appears to be the most expeditious approach -- all I want is to get > this patch done and be out of your hair. If Thomas Huth wants me to use a > consistent name, have I not complied? Are you asking out of curiosity or is > there a valid reason why I should answer your question in order to get the > patch submitted? Would you like to have a friendly chat over virtual coffee > sometime (but off-list)? > > > > If you could please clarify I'm sure the answer is an easy one. > > I'm asking because our basic expected position is "commits > are from the submitter's actual name, not a pseudonym". Obviously > we can't tell if people use a consistent plausible looking > pseudonym whether that corresponds to their real-world name > or not, but if you have a real name you're happy to attach > to this patch and are merely using a pseudonym for Google > email, then the resubmit of this patch didn't seem to me > to do that. i.e. I was expecting the change to be "make the > patch From: match the Signed-off-by line", not "make the > Signed-off-by line match the patch From:". (For avoidance > of doubt, we don't care about the email From: line, which > is distinct from the commit message From: i.e. author.) > So I was essentially asking "did you mean to do this, or did > you misunderstand what we were asking for?". > I think that is what caught me off guard. I'm learning how to submit the correctly formatted patch. I would very much like to disconnect the patch From: from the email From: line. > On the question of the actual patch, I'll try to get to it > if Gerd doesn't first (though I have a conference next week > so it might be the week after). The main thing I need to chase > down is whether it's OK to call usb_packet_addbuf() with a > zero length or not. > Good catch. I have no problem modifying the patch with better logic for a zero length packet.
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Thu, May 9, 2024 at 3:37 PM BALATON Zoltan wrote: > On Thu, 9 May 2024, Cord Amfmgm wrote: > > On Thu, May 9, 2024 at 12:48 PM Peter Maydell > > wrote: > > > >> On Wed, 8 May 2024 at 16:29, Cord Amfmgm wrote: > >>> On Wed, May 8, 2024 at 3:45 AM Thomas Huth 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 > >> > >> I'm confused now. Of the two names you've used in this > >> patch (Cord Amfmgm and David Hubbard), are they both > >> pseudonyms, or is one a pseudonym and one your real name? > >> > >> > > Hi Peter, > > > > I am attempting to submit a small patch. For context, I'm getting broader > > attention now because apparently OHCI is one of the less used components > of > > qemu and maybe the review process was taking a while. That's relevant > > because I wasn't able to get prompt feedback and am now choosing what > > appears to be the most expeditious approach -- all I want is to get this > > patch done and be out of your hair. If Thomas Huth wants me to use a > > consistent name, have I not complied? Are you asking out of curiosity or > is > > there a valid reason why I should answer your question in order to get > the > > patch submitted? Would you like to have a friendly chat over virtual > coffee > > sometime (but off-list)? > > See here: > > https://www.qemu.org/docs/master/devel/submitting-a-patch.html#patch-emails-must-include-a-signed-off-by-line > and also the document linked from there: > > http://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/tree/Documentation/SubmittingPatches?id=f6f94e2ab1b33f0082ac22d71f66385a60d8157f#n297 Yeah the policy makes sense. So it sounds like we're all good for that. > > > As for getting the patch reviewed, it may be difficult as the USB > maintainer is practically absent and has no time for QEMU for a while and > as OHCI as you said is not odten used there aren't many people who could > review it. Getting at least the formal stuff out of the way may help > though to get somebody to try to review the patch. > > Regards, > BALATON Zoltan I understand. Well, that's unfortunate that the patch is going back on the backlog. I'll leave it alone then? There's always the option if anyone has an old enough system that the EHCI on it has an actual OHCI companion controller, then they can use actual hardware to validate the behavior. Barring some message saying the patch has been approved or that someone wants me to rework the patch, I'll leave this as abandoned.
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Thu, May 9, 2024 at 12:48 PM Peter Maydell wrote: > On Wed, 8 May 2024 at 16:29, Cord Amfmgm wrote: > > On Wed, May 8, 2024 at 3:45 AM Thomas Huth 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 > > I'm confused now. Of the two names you've used in this > patch (Cord Amfmgm and David Hubbard), are they both > pseudonyms, or is one a pseudonym and one your real name? > > Hi Peter, I am attempting to submit a small patch. For context, I'm getting broader attention now because apparently OHCI is one of the less used components of qemu and maybe the review process was taking a while. That's relevant because I wasn't able to get prompt feedback and am now choosing what appears to be the most expeditious approach -- all I want is to get this patch done and be out of your hair. If Thomas Huth wants me to use a consistent name, have I not complied? Are you asking out of curiosity or is there a valid reason why I should answer your question in order to get the patch submitted? Would you like to have a friendly chat over virtual coffee sometime (but off-list)? If you could please clarify I'm sure the answer is an easy one. Thanks
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Wed, May 8, 2024 at 4:53 AM Philippe Mathieu-Daudé
wrote:
> On 7/5/24 22:20, Cord Amfmgm wrote:
> >
> >
> > On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm > <mailto:dmamf...@gmail.com>> wrote:
> >
> > On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev > <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]) & 0xfff0)
> > > | (/* ToggleCarry= */ 0 << 1);
> > > ed->next_ed = (/* NextED= */ 0 & 0xfff0)
> > >
> > > 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 > <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
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm wrote:
> On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev 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]) & 0xfff0)
>> > | (/* ToggleCarry= */ 0 << 1);
>> > ed->next_ed = (/* NextED= */ 0 & 0xfff0)
>> >
>> > 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
>>
>> 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 & 0xf000) != (td.be & 0xf000)) {
>> > 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"
>> >
>>
>
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev 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]) & 0xfff0)
> > | (/* ToggleCarry= */ 0 << 1);
> > ed->next_ed = (/* NextED= */ 0 & 0xfff0)
> >
> > 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
>
> 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.
>
> > 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 & 0xf000) != (td.be & 0xf000)) {
> > 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"
> >
>
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
Hi Michael,
This just got lost somehow. It is still an issue (see
https://gitlab.com/qemu-project/qemu/-/issues/1510 ). I believe this change
fixes the issue.
On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev 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]) & 0xfff0)
> > | (/* ToggleCarry= */ 0 << 1);
> > ed->next_ed = (/* NextED= */ 0 & 0xfff0)
> >
> > 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
>
> Wonder if this got lost somehow. Or is it not needed?
>
> Thanks,
>
> /mjt
>
> > 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 & 0xf000) != (td.be & 0xf000)) {
> > 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"
> >
>
>
Re: hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
Attempting to resend with both files in the patch this time:
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
migrated to #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]) & 0xfff0)
| (/* ToggleCarry= */ 0 << 1);
ed->next_ed = (/* NextED= */ 0 & 0xfff0)
qemu-fuzz also caught the same issue in #1510
The if (td.cbp > td.be) logic in ohci_service_td() causes an
ohci_die() as well. 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.
Signed-off-by: David Hubbard
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index d73b53f33c..d087f36618 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 & 0xf000) != (td.be & 0xf000)) {
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..46cab1d550 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 = %x > be = %x"
+usb_ohci_td_bad_pid(const char *s, uint32_t edf, uint32_t tdf) "Bad
pid %s: ed.flags %x td.flags %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"
hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
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]) & 0xfff0)
| (/* ToggleCarry= */ 0 << 1);
ed->next_ed = (/* NextED= */ 0 & 0xfff0)
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
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 & 0xf000) != (td.be & 0xf000)) {
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"
hw/usb/hcd-ohci: Fix #1510, #303: pid not IN or OUT
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
migrated to #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]) & 0xfff0) | (/*
ToggleCarry= */ 0 << 1);
ed->next_ed = (/* NextED= */ 0 & 0xfff0)
qemu-fuzz also caught the same issue in #1510
The if (td.cbp > td.be) logic in ohci_service_td() causes an
ohci_die() as well. 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.
Signed-off-by: David Hubbard
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index d73b53f33c..d087f36618 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 & 0xf000) != (td.be & 0xf000)) {
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;
}
