Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid

[Philippe Mathieu-Daudé]

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 <address@hidden>
>>>
>>> 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 (address@hidden)
>>> 12 0x000000000045dcf5 in qemu_kvm_cpu_thread_fn (arg=0x1ab11b0)
>>> 13 0x0000000000870631 in qemu_thread_start (address@hidden)
>>> 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
> (address@hidden, address@hidden, data=1648892416,
> address@hidden, address@hidden)

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 (address@hidden)
> #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.

> 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 <address@hidden>
>>> Signed-off-by: Longpeng <address@hidden>
>>> ---
>>>  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.

>>     assert(epid >= 1 && epid <= 31);
>>
>> Regards,
>>
>> Phil.
>>