Hi Philippe, On 2019/4/29 19:16, Philippe Mathieu-Daudé wrote:
> Hi Mike, > > On 4/29/19 9:39 AM, Longpeng(Mike) wrote: >> From: Longpeng <longpe...@huawei.com> >> >> we found the following core in our environment: >> 0 0x00007fc6b06c2237 in raise () >> 1 0x00007fc6b06c3928 in abort () >> 2 0x00007fc6b06bb056 in __assert_fail_base () >> 3 0x00007fc6b06bb102 in __assert_fail () >> 4 0x0000000000702e36 in xhci_kick_ep (...) > > 5 xhci_doorbell_write? > >> 6 0x000000000047767f in access_with_adjusted_size (...) >> 7 0x000000000047944d in memory_region_dispatch_write (...) >> 8 0x000000000042df17 in address_space_write_continue (...) >> 10 0x000000000043084d in address_space_rw (...) >> 11 0x000000000047451b in kvm_cpu_exec (cpu=cpu@entry=0x1ab11b0) >> 12 0x000000000045dcf5 in qemu_kvm_cpu_thread_fn (arg=0x1ab11b0) >> 13 0x0000000000870631 in qemu_thread_start (args=args@entry=0x1acfb50) >> 14 0x00000000008959a7 in thread_entry_for_hotfix (pthread_cb=<optimized out>) >> 15 0x00007fc6b0a60dd5 in start_thread () >> 16 0x00007fc6b078a59d in clone () >> (gdb) bt >> (gdb) f 5 > > This is the frame you removed... > >> (gdb) p /x tmp >> $9 = 0x62481a00 <-- last byte 0x00 is @epid > > I don't see 'tmp' in xhci_doorbell_write(). > > Can you use trace events? > > There we have trace_usb_xhci_doorbell_write(). > Sorry , I'm careless to remove the important information. This is our whole frame: (gdb) bt #0 0x00007fc6b06c2237 in raise () from /usr/lib64/libc.so.6 #1 0x00007fc6b06c3928 in abort () from /usr/lib64/libc.so.6 #2 0x00007fc6b06bb056 in __assert_fail_base () from /usr/lib64/libc.so.6 #3 0x00007fc6b06bb102 in __assert_fail () from /usr/lib64/libc.so.6 #4 0x0000000000702e36 in xhci_kick_ep (...) #5 0x000000000047897a in memory_region_write_accessor (...) #6 0x000000000047767f in access_with_adjusted_size (...) #7 0x000000000047944d in memory_region_dispatch_write (mr=mr@entry=0x7fc6a0138df0, addr=addr@entry=156, data=1648892416, size=size@entry=4, attrs=attrs@entry=...) #8 0x000000000042df17 in address_space_write_continue (...) #9 0x00000000004302d5 in address_space_write (...) #10 0x000000000043084d in address_space_rw (...) #11 0x000000000047451b in kvm_cpu_exec (...) #12 0x000000000045dcf5 in qemu_kvm_cpu_thread_fn (arg=0x1ab11b0) #13 0x0000000000870631 in qemu_thread_start (args=args@entry=0x1acfb50) #14 0x00000000008959a7 in thread_entry_for_hotfix (pthread_cb=<optimized out>) #15 0x00007fc6b0a60dd5 in start_thread () from /usr/lib64/libpthread.so.0 #16 0x00007fc6b078a59d in clone () from /usr/lib64/libc.so.6 (gdb) f 5 #5 0x000000000047897a in memory_region_write_accessor (...) 529 mr->ops->write(mr->opaque, addr, tmp, size); (gdb) p /x tmp $9 = 0x62481a00 static void xhci_doorbell_write(void *ptr, hwaddr reg, uint64_t val, unsigned size) So, the @val is 0x62481a00, and the last byte is epid, right? >> >> xhci_doorbell_write() already check the upper bound of @slotid an @epid, >> it also need to check the lower bound. >> >> Cc: Gonglei <arei.gong...@huawei.com> >> Signed-off-by: Longpeng <longpe...@huawei.com> >> --- >> hw/usb/hcd-xhci.c | 4 ++-- >> 1 file changed, 2 insertions(+), 2 deletions(-) >> >> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c >> index ec28bee..b4e6bfc 100644 >> --- a/hw/usb/hcd-xhci.c >> +++ b/hw/usb/hcd-xhci.c >> @@ -3135,9 +3135,9 @@ static void xhci_doorbell_write(void *ptr, hwaddr reg, > > Expanding the diff: > > if (reg == 0) { > if (val == 0) { > xhci_process_commands(xhci); > } else { > DPRINTF("xhci: bad doorbell 0 write: 0x%x\n", > (uint32_t)val); > } >> } else { >> epid = val & 0xff; >> streamid = (val >> 16) & 0xffff; >> - if (reg > xhci->numslots) { >> + if (reg == 0 || reg > xhci->numslots) { > > So 'reg' can not be zero here... > Oh, you're right. >> DPRINTF("xhci: bad doorbell %d\n", (int)reg); >> - } else if (epid > 31) { >> + } else if (epid == 0 || epid > 31) { > > Here neither. > In our frame, the epid is zero. The @val is from guest which is untrusted, when this problem happened, I saw it wrote many invalid value, not only usb but also other devices. >> DPRINTF("xhci: bad doorbell %d write: 0x%x\n", >> (int)reg, (uint32_t)val); >> } else { >> > > Isn't it the other line that triggered the assertion? > > static void xhci_kick_ep(XHCIState *xhci, unsigned int slotid, > unsigned int epid, unsigned int streamid) > { > XHCIEPContext *epctx; > > assert(slotid >= 1 && slotid <= xhci->numslots); // <=== > assert(epid >= 1 && epid <= 31); > > Regards, > > Phil. > > > -- Regards, Longpeng(Mike)