[PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues

[PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues

[P J P]

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

[PATCH v2 1/2] hw: usb: hcd-ohci: check len and frame_number variables

[P J P]

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

[PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire

[P J P]

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

Re: [PATCH v2 2/2] hw: usb: hcd-ohci: check for processed TD before retire

[Li Qiang]

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
>

Re: [PATCH v2 0/2] hw: usb: hcd-ohci: fix oob access and loop issues

[Gerd Hoffmann]

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