[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest writ
From: |
Longpeng (Mike) |
Subject: |
Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid |
Date: |
Tue, 30 Apr 2019 10:02:47 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
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.
>>> assert(epid >= 1 && epid <= 31);
>>>
>>> Regards,
>>>
>>> Phil.
>>>
>
> .
>
--
Regards,
Longpeng(Mike)
Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid, no-reply, 2019/04/29
Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid, no-reply, 2019/04/29