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

[Philippe Mathieu-Daudé]

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 <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.
>>
> 
> 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 <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.
>>
> 
> 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é <address@hidden>

(Please include the full backtrace in the description).

Thanks for being patient with me with this patch :)

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