* [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues
@ 2020-09-15 18:22 P J P
2020-09-15 18:22 ` [PATCH v2 1/2] hw: usb: hcd-ohci: check len and frame_number variables P J P
` (2 more replies)
0 siblings, 3 replies; 5+ messages in thread
From: P J P @ 2020-09-15 18:22 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Prasad J Pandit, Li Qiang, QEMU Developers, Yi Ren, Yongkang Jia,
Gaoning Pan
From: Prasad J Pandit <pjp@fedoraproject.org>
Hello,
* While servicing transfer descriptors(TD) in ohci_service[_iso]_td
routines, it may lead to out-of-bounds access and/or infinite loop
issues, as the OHCI controller driver may supply malicious values
to derive frame_number, start_addr, end_addr etc. variables.
* This series breaks earlier single patch into two.
One for an out-of-bounds access issue and another to fix infinite
loop case.
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05145.html
Thank you.
--
Prasad J Pandit (2):
hw: usb: hcd-ohci: check len and frame_number variables
hw: usb: hcd-ohci: check for processed TD before retire
hw/usb/hcd-ohci.c | 28 ++++++++++++++++++++++++++--
1 file changed, 26 insertions(+), 2 deletions(-)
--
2.26.2
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH v2 1/2] hw: usb: hcd-ohci: check len and frame_number variables
2020-09-15 18:22 [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues P J P
@ 2020-09-15 18:22 ` P J P
2020-09-15 18:22 ` [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire P J P
2020-09-21 7:53 ` [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues Gerd Hoffmann
2 siblings, 0 replies; 5+ messages in thread
From: P J P @ 2020-09-15 18:22 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Prasad J Pandit, Li Qiang, QEMU Developers, Yi Ren, Yongkang Jia,
Gaoning Pan
From: Prasad J Pandit <pjp@fedoraproject.org>
While servicing the OHCI transfer descriptors(TD), OHCI host
controller derives variables 'start_addr', 'end_addr', 'len'
etc. from values supplied by the host controller driver.
Host controller driver may supply values such that using
above variables leads to out-of-bounds access issues.
Add checks to avoid them.
AddressSanitizer: stack-buffer-overflow on address 0x7ffd53af76a0
READ of size 2 at 0x7ffd53af76a0 thread T0
#0 ohci_service_iso_td ../hw/usb/hcd-ohci.c:734
#1 ohci_service_ed_list ../hw/usb/hcd-ohci.c:1180
#2 ohci_process_lists ../hw/usb/hcd-ohci.c:1214
#3 ohci_frame_boundary ../hw/usb/hcd-ohci.c:1257
#4 timerlist_run_timers ../util/qemu-timer.c:572
#5 qemu_clock_run_timers ../util/qemu-timer.c:586
#6 qemu_clock_run_all_timers ../util/qemu-timer.c:672
#7 main_loop_wait ../util/main-loop.c:527
#8 qemu_main_loop ../softmmu/vl.c:1676
#9 main ../softmmu/main.c:50
Reported-by: Gaoning Pan <pgn@zju.edu.cn>
Reported-by: Yongkang Jia <j_kangel@163.com>
Reported-by: Yi Ren <yunye.ry@alibaba-inc.com>
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/usb/hcd-ohci.c | 24 ++++++++++++++++++++++--
1 file changed, 22 insertions(+), 2 deletions(-)
Update v2: one patch to fix oob access
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05145.html
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 1e6e85e86a..9dc59101f9 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -731,7 +731,11 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
}
start_offset = iso_td.offset[relative_frame_number];
- next_offset = iso_td.offset[relative_frame_number + 1];
+ if (relative_frame_number < frame_count) {
+ next_offset = iso_td.offset[relative_frame_number + 1];
+ } else {
+ next_offset = iso_td.be;
+ }
if (!(OHCI_BM(start_offset, TD_PSW_CC) & 0xe) ||
((relative_frame_number < frame_count) &&
@@ -764,7 +768,12 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
}
} else {
/* Last packet in the ISO TD */
- end_addr = iso_td.be;
+ end_addr = next_offset;
+ }
+
+ if (start_addr > end_addr) {
+ trace_usb_ohci_iso_td_bad_cc_overrun(start_addr, end_addr);
+ return 1;
}
if ((start_addr & OHCI_PAGE_MASK) != (end_addr & OHCI_PAGE_MASK)) {
@@ -773,6 +782,9 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
} else {
len = end_addr - start_addr + 1;
}
+ if (len > sizeof(ohci->usb_buf)) {
+ len = sizeof(ohci->usb_buf);
+ }
if (len && dir != OHCI_TD_DIR_IN) {
if (ohci_copy_iso_td(ohci, start_addr, end_addr, ohci->usb_buf, len,
@@ -975,8 +987,16 @@ 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);
+ ohci_die(ohci);
+ return 1;
+ }
len = (td.be - td.cbp) + 1;
}
+ if (len > sizeof(ohci->usb_buf)) {
+ len = sizeof(ohci->usb_buf);
+ }
pktlen = len;
if (len && dir != OHCI_TD_DIR_IN) {
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire
2020-09-15 18:22 [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues P J P
2020-09-15 18:22 ` [PATCH v2 1/2] hw: usb: hcd-ohci: check len and frame_number variables P J P
@ 2020-09-15 18:22 ` P J P
2020-09-16 14:51 ` Li Qiang
2020-09-21 7:53 ` [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues Gerd Hoffmann
2 siblings, 1 reply; 5+ messages in thread
From: P J P @ 2020-09-15 18:22 UTC (permalink / raw)
To: Gerd Hoffmann
Cc: Prasad J Pandit, Li Qiang, QEMU Developers, Yi Ren, Yongkang Jia,
Gaoning Pan
From: Prasad J Pandit <pjp@fedoraproject.org>
While servicing OHCI transfer descriptors(TD), ohci_service_iso_td
retires a TD if it has passed its time frame. It does not check if
the TD was already processed once and holds an error code in TD_CC.
It may happen if the TD list has a loop. Add check to avoid an
infinite loop condition.
Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
---
hw/usb/hcd-ohci.c | 4 ++++
1 file changed, 4 insertions(+)
Update v2: one patch for loop issue
-> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05145.html
diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
index 9dc59101f9..8b912e95d3 100644
--- a/hw/usb/hcd-ohci.c
+++ b/hw/usb/hcd-ohci.c
@@ -691,6 +691,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
the next ISO TD of the same ED */
trace_usb_ohci_iso_td_relative_frame_number_big(relative_frame_number,
frame_count);
+ if (OHCI_CC_DATAOVERRUN == OHCI_BM(iso_td.flags, TD_CC)) {
+ /* avoid infinite loop */
+ return 1;
+ }
OHCI_SET_BM(iso_td.flags, TD_CC, OHCI_CC_DATAOVERRUN);
ed->head &= ~OHCI_DPTR_MASK;
ed->head |= (iso_td.next & OHCI_DPTR_MASK);
--
2.26.2
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire
2020-09-15 18:22 ` [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire P J P
@ 2020-09-16 14:51 ` Li Qiang
0 siblings, 0 replies; 5+ messages in thread
From: Li Qiang @ 2020-09-16 14:51 UTC (permalink / raw)
To: P J P
Cc: Prasad J Pandit, QEMU Developers, Yi Ren, Yongkang Jia,
Gaoning Pan, Gerd Hoffmann
P J P <ppandit@redhat.com> 于2020年9月16日周三 上午2:25写道:
>
> From: Prasad J Pandit <pjp@fedoraproject.org>
Reviewed-by: Li Qiang <liq3ea@gmail.com>
>
> While servicing OHCI transfer descriptors(TD), ohci_service_iso_td
> retires a TD if it has passed its time frame. It does not check if
> the TD was already processed once and holds an error code in TD_CC.
> It may happen if the TD list has a loop. Add check to avoid an
> infinite loop condition.
>
> Signed-off-by: Prasad J Pandit <pjp@fedoraproject.org>
> ---
> hw/usb/hcd-ohci.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> Update v2: one patch for loop issue
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05145.html
>
> diff --git a/hw/usb/hcd-ohci.c b/hw/usb/hcd-ohci.c
> index 9dc59101f9..8b912e95d3 100644
> --- a/hw/usb/hcd-ohci.c
> +++ b/hw/usb/hcd-ohci.c
> @@ -691,6 +691,10 @@ static int ohci_service_iso_td(OHCIState *ohci, struct ohci_ed *ed,
> the next ISO TD of the same ED */
> trace_usb_ohci_iso_td_relative_frame_number_big(relative_frame_number,
> frame_count);
> + if (OHCI_CC_DATAOVERRUN == OHCI_BM(iso_td.flags, TD_CC)) {
> + /* avoid infinite loop */
> + return 1;
> + }
> OHCI_SET_BM(iso_td.flags, TD_CC, OHCI_CC_DATAOVERRUN);
> ed->head &= ~OHCI_DPTR_MASK;
> ed->head |= (iso_td.next & OHCI_DPTR_MASK);
> --
> 2.26.2
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues
2020-09-15 18:22 [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues P J P
2020-09-15 18:22 ` [PATCH v2 1/2] hw: usb: hcd-ohci: check len and frame_number variables P J P
2020-09-15 18:22 ` [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire P J P
@ 2020-09-21 7:53 ` Gerd Hoffmann
2 siblings, 0 replies; 5+ messages in thread
From: Gerd Hoffmann @ 2020-09-21 7:53 UTC (permalink / raw)
To: P J P
Cc: Prasad J Pandit, Li Qiang, QEMU Developers, Yi Ren, Yongkang Jia,
Gaoning Pan
On Tue, Sep 15, 2020 at 11:52:57PM +0530, P J P wrote:
> From: Prasad J Pandit <pjp@fedoraproject.org>
>
> Hello,
>
> * While servicing transfer descriptors(TD) in ohci_service[_iso]_td
> routines, it may lead to out-of-bounds access and/or infinite loop
> issues, as the OHCI controller driver may supply malicious values
> to derive frame_number, start_addr, end_addr etc. variables.
>
> * This series breaks earlier single patch into two.
> One for an out-of-bounds access issue and another to fix infinite
> loop case.
> -> https://lists.nongnu.org/archive/html/qemu-devel/2020-09/msg05145.html
Added to usb patch queue.
thanks,
Gerd
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2020-09-21 7:55 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-09-15 18:22 [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues P J P
2020-09-15 18:22 ` [PATCH v2 1/2] hw: usb: hcd-ohci: check len and frame_number variables P J P
2020-09-15 18:22 ` [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire P J P
2020-09-16 14:51 ` Li Qiang
2020-09-21 7:53 ` [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues Gerd Hoffmann
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.