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 <dmamfmgm@gmail.com>
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"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 <dmamfmgm@gmail.com>
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 & 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"
>
On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <mjt@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 <dmamfmgm@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.
>
> > 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"
> >
>
On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamfmgm@gmail.com> wrote:
> On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <mjt@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 <dmamfmgm@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"
>> >
>>
>
On 7/5/24 22:20, Cord Amfmgm wrote:
>
>
> On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamfmgm@gmail.com
> <mailto:dmamfmgm@gmail.com>> wrote:
>
> On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <mjt@tls.msk.ru
> <mailto:mjt@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 <dmamfmgm@gmail.com
> <mailto:dmamfmgm@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-12bb70c31d31@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.
> > 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"
> >
>
On Wed, May 8, 2024 at 4:53 AM Philippe Mathieu-Daudé <philmd@linaro.org>
wrote:
> On 7/5/24 22:20, Cord Amfmgm wrote:
> >
> >
> > On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamfmgm@gmail.com
> > <mailto:dmamfmgm@gmail.com>> wrote:
> >
> > On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <mjt@tls.msk.ru
> > <mailto:mjt@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 <dmamfmgm@gmail.com
> > <mailto:dmamfmgm@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-12bb70c31d31@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 <thuth@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"
> > >
> >
>
>
On Wed, 8 May 2024 at 16:29, Cord Amfmgm <dmamfmgm@gmail.com> wrote: > On Wed, May 8, 2024 at 3:45 AM Thomas Huth <thuth@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 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? thanks -- PMM
On Thu, May 9, 2024 at 12:48 PM Peter Maydell <peter.maydell@linaro.org> wrote: > On Wed, 8 May 2024 at 16:29, Cord Amfmgm <dmamfmgm@gmail.com> wrote: > > On Wed, May 8, 2024 at 3:45 AM Thomas Huth <thuth@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 > > 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
On Thu, 9 May 2024 at 19:17, Cord Amfmgm <dmamfmgm@gmail.com> wrote: > > > > On Thu, May 9, 2024 at 12:48 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Wed, 8 May 2024 at 16:29, Cord Amfmgm <dmamfmgm@gmail.com> wrote: >> > On Wed, May 8, 2024 at 3:45 AM Thomas Huth <thuth@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 >> >> 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?". 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. thanks -- PMM
On Thu, 9 May 2024, Cord Amfmgm wrote: > On Thu, May 9, 2024 at 12:48 PM Peter Maydell <peter.maydell@linaro.org> > wrote: > >> On Wed, 8 May 2024 at 16:29, Cord Amfmgm <dmamfmgm@gmail.com> wrote: >>> On Wed, May 8, 2024 at 3:45 AM Thomas Huth <thuth@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 >> >> 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 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
On Thu, May 9, 2024 at 3:37 PM BALATON Zoltan <balaton@eik.bme.hu> wrote: > On Thu, 9 May 2024, Cord Amfmgm wrote: > > On Thu, May 9, 2024 at 12:48 PM Peter Maydell <peter.maydell@linaro.org> > > wrote: > > > >> On Wed, 8 May 2024 at 16:29, Cord Amfmgm <dmamfmgm@gmail.com> wrote: > >>> On Wed, May 8, 2024 at 3:45 AM Thomas Huth <thuth@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 > >> > >> 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.
On Wed, May 8, 2024 at 10:28 AM Cord Amfmgm <dmamfmgm@gmail.com> wrote:
>
>
> On Wed, May 8, 2024 at 4:53 AM Philippe Mathieu-Daudé <philmd@linaro.org>
> wrote:
>
>> On 7/5/24 22:20, Cord Amfmgm wrote:
>> >
>> >
>> > On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamfmgm@gmail.com
>> > <mailto:dmamfmgm@gmail.com>> wrote:
>> >
>> > On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <mjt@tls.msk.ru
>> > <mailto:mjt@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 <dmamfmgm@gmail.com
>> > <mailto:dmamfmgm@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-12bb70c31d31@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 <thuth@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
>
>
I've sent the new patches just now. The repro disk images mentioned in the
patch descriptions are attached to this email.
>
>> > > 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"
>> > >
>> >
>>
>>
On 07/05/2024 22.20, Cord Amfmgm wrote: > > > On Wed, Apr 24, 2024 at 3:43 PM Cord Amfmgm <dmamfmgm@gmail.com > <mailto:dmamfmgm@gmail.com>> wrote: > > On Thu, Apr 18, 2024 at 10:43 AM Michael Tokarev <mjt@tls.msk.ru > <mailto:mjt@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 <dmamfmgm@gmail.com > <mailto:dmamfmgm@gmail.com>> 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) > 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? Looks like it's hard to find someone who still can review OHCI patches these days... Anyway, I tried to get the reproducer running that had been added to the original patch (installed an Ubuntu 18.04 guest and compiled and ran that ohci_poc program in it), but so far, I failed. Could you please provide detailed steps how you can still produce this issue with the latest version of QEMU, please? Thanks, Thomas
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 <mjt@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 <dmamfmgm@gmail.com>
>
> 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 & 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"
> >
>
>
© 2016 - 2024 Red Hat, Inc.