Re: [PATCH] usb/hcd-xhci: Fix an index-out-of-bounds in xhci_runtime_write and xhci_runtime_read

[Alex Chen]

Kindly ping...

On 2020/11/5 22:02, AlexChen wrote:
> Currently, the 'v' is not checked whether it is between 0 and 16,
> which may result in an out-of-bounds access to the array 'xhci->intr[]'.
> This is LP#1902112. Following is the reproducer provided in:
> -->https://bugs.launchpad.net/qemu/+bug/1902112
> 
> === Reproducer (build with --enable-sanitizers) ===
> export UBSAN_OPTIONS="print_stacktrace=1:silence_unsigned_overflow=1"
> cat << EOF | ./qemu-system-i386 -display none -machine\
>  accel=qtest, -m 512M -machine q35 -nodefaults -drive\
>  file=null-co://,if=none,format=raw,id=disk0 -device\
>  qemu-xhci,id=xhci -device usb-tablet,bus=xhci.0\
>  -device usb-bot -device usb-storage,drive=disk0\
>  -chardev null,id=cd0 -chardev null,id=cd1 -device\
>  usb-braille,chardev=cd0 -device usb-ccid -device\
>  usb-ccid -device usb-kbd -device usb-mouse -device\
>  usb-serial,chardev=cd1 -device usb-tablet -device\
>  usb-wacom-tablet -device usb-audio -qtest stdio
> outl 0xcf8 0x80000803
> outl 0xcfc 0x18caffff
> outl 0xcf8 0x80000810
> outl 0xcfc 0x555a2e46
> write 0x555a1004 0x4 0xe7b9aa7a
> EOF
> 
> === Stack Trace ===
> 
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior 
> ../hw/usb/hcd-xhci.c:3012:30 in
> ../hw/usb/hcd-xhci.c:3012:30: runtime error: index -1 out of bounds for type 
> 'XHCIInterrupter [16]'
> #0 0x55bd2e97c8b0 in xhci_runtime_write /src/qemu/hw/usb/hcd-xhci.c:3012:30
> #1 0x55bd2edfdd13 in memory_region_write_accessor 
> /src/qemu/softmmu/memory.c:484:5
> #2 0x55bd2edfdb14 in access_with_adjusted_size 
> /src/qemu/softmmu/memory.c:545:18
> #3 0x55bd2edfd54b in memory_region_dispatch_write 
> /src/qemu/softmmu/memory.c:0:13
> #4 0x55bd2ed7fa46 in flatview_write_continue 
> /src/qemu/softmmu/physmem.c:2767:23
> #5 0x55bd2ed7cac0 in flatview_write /src/qemu/softmmu/physmem.c:2807:14
> 
> This patch fixes this bug.
> 
> Buglink: https://bugs.launchpad.net/qemu/+bug/1902112
> Reported-by: Alexander Bulekov <alxndr@bu.edu>
> Signed-off-by: Alex Chen <alex.chen@huawei.com>
> ---
>  hw/usb/hcd-xhci.c | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 79ce5c4be6..50abef40ad 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -2962,8 +2962,9 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
>  {
>      XHCIState *xhci = ptr;
>      uint32_t ret = 0;
> +    int v = (reg - 0x20) / 0x20;
> 
> -    if (reg < 0x20) {
> +    if (reg < 0x20 || v < 0 || v >= XHCI_MAXINTRS) {
>          switch (reg) {
>          case 0x00: /* MFINDEX */
>              ret = xhci_mfindex_get(xhci) & 0x3fff;
> @@ -2973,7 +2974,6 @@ static uint64_t xhci_runtime_read(void *ptr, hwaddr reg,
>              break;
>          }
>      } else {
> -        int v = (reg - 0x20) / 0x20;
>          XHCIInterrupter *intr = &xhci->intr[v];
>          switch (reg & 0x1f) {
>          case 0x00: /* IMAN */
> @@ -3009,14 +3009,16 @@ static void xhci_runtime_write(void *ptr, hwaddr reg,
>  {
>      XHCIState *xhci = ptr;
>      int v = (reg - 0x20) / 0x20;
> -    XHCIInterrupter *intr = &xhci->intr[v];
> +    XHCIInterrupter *intr;
>      trace_usb_xhci_runtime_write(reg, val);
> 
> -    if (reg < 0x20) {
> +    if (reg < 0x20 || v < 0 || v >= XHCI_MAXINTRS) {
>          trace_usb_xhci_unimplemented("runtime write", reg);
>          return;
>      }
> 
> +    intr = &xhci->intr[v];
> +
>      switch (reg & 0x1f) {
>      case 0x00: /* IMAN */
>          if (val & IMAN_IP) {
>