On 4/30/19 4:02 AM, Longpeng (Mike) wrote:
> On 2019/4/29 20:10, Philippe Mathieu-Daudé wrote:
>> On 4/29/19 1:42 PM, Longpeng (Mike) wrote:
>>> 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=...)
>>
>> So this is a 32-bit access, to address 156 (which is the slotid) and
>> data=1648892416=0x62481a00 indeed.
>>
>> But watch out access_with_adjusted_size() calls adjust_endianness()...
>>
>>> #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
>>
>> ... since memory_region_write_accessor() has the same argument, then I
>> can assume your guest is running in Little-Endian.
>>
>
> Yes.
>
>>> 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.
>>
>> If you use mainstream QEMU, we have:
>>
>> static void qemu_xhci_instance_init(Object *obj)
>> {
>> ...
>> xhci->numslots = MAXSLOTS;
>> ...
>> }
>>
>> $ git grep define.*MAXSLOTS
>> hw/usb/hcd-xhci.c:52:#define LEN_DOORBELL ((MAXSLOTS + 1) * 0x20)
>> hw/usb/hcd-xhci.h:33:#define MAXSLOTS 64
>> hw/usb/hcd-xhci.h:37:#define EV_QUEUE (((3 * 24) + 16) * MAXSLOTS)
>>
>>>
>>>>> 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); // <===
>>
>> slotid >= 1 // true
>> slotid <= xhci->numslots // FALSE (156 > 64)
>>
>> So this assertion won't pass.
>>
>
> Oh, you miss the following code in xhci_doorbell_write():
> reg >>= 2;
>
> So the @slotid pass to xhci_kick_ep() is 156/4 = 39 < 64 and the
> 'assert(slotid >= 1 && slotid <= xhci->numslots)' assertion will pass.
>
> Check the @epid in xhci_doorbell_write() is still needed.
Oh! My bad, I missed that, I'm confused :S
So for your next patch with simply this change:
- } else if (epid > 31) {
+ } else if (epid == 0 || epid > 31) {
You can include:
Reviewed-by: Philippe Mathieu-Daudé <phi...@redhat.com>
(Please include the full backtrace in the description).
Thanks for being patient with me with this patch :)
>>>> assert(epid >= 1 && epid <= 31);
>>>>
>>>> Regards,
>>>>
>>>> Phil.
>>>>
>>
>> .
>>
