[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: |
Mon, 29 Apr 2019 19:42:43 +0800 |
User-agent: |
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:11.0) Gecko/20120327 Thunderbird/11.0.1 |
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)
#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
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.
>> 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)
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