* [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 7:39 ` Longpeng(Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng(Mike) @ 2019-04-29 7:39 UTC (permalink / raw)
To: kraxel; +Cc: qemu-devel, Longpeng, Gonglei
From: Longpeng <longpeng2@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 (...)
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
(gdb) p /x tmp
$9 = 0x62481a00 <-- last byte 0x00 is @epid
xhci_doorbell_write() already check the upper bound of @slotid an @epid,
it also need to check the lower bound.
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Longpeng <longpeng2@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,
} else {
epid = val & 0xff;
streamid = (val >> 16) & 0xffff;
- if (reg > xhci->numslots) {
+ if (reg == 0 || reg > xhci->numslots) {
DPRINTF("xhci: bad doorbell %d\n", (int)reg);
- } else if (epid > 31) {
+ } else if (epid == 0 || epid > 31) {
DPRINTF("xhci: bad doorbell %d write: 0x%x\n",
(int)reg, (uint32_t)val);
} else {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 7:39 ` Longpeng(Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng(Mike) @ 2019-04-29 7:39 UTC (permalink / raw)
To: kraxel; +Cc: Longpeng, Gonglei, qemu-devel
From: Longpeng <longpeng2@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 (...)
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
(gdb) p /x tmp
$9 = 0x62481a00 <-- last byte 0x00 is @epid
xhci_doorbell_write() already check the upper bound of @slotid an @epid,
it also need to check the lower bound.
Cc: Gonglei <arei.gonglei@huawei.com>
Signed-off-by: Longpeng <longpeng2@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,
} else {
epid = val & 0xff;
streamid = (val >> 16) & 0xffff;
- if (reg > xhci->numslots) {
+ if (reg == 0 || reg > xhci->numslots) {
DPRINTF("xhci: bad doorbell %d\n", (int)reg);
- } else if (epid > 31) {
+ } else if (epid == 0 || epid > 31) {
DPRINTF("xhci: bad doorbell %d write: 0x%x\n",
(int)reg, (uint32_t)val);
} else {
--
1.8.3.1
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
2019-04-29 7:39 ` Longpeng(Mike)
(?)
@ 2019-04-29 11:16 ` Philippe Mathieu-Daudé
2019-04-29 11:42 ` Longpeng (Mike)
-1 siblings, 1 reply; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-29 11:16 UTC (permalink / raw)
To: Longpeng(Mike), kraxel; +Cc: Gonglei, qemu-devel
Hi Mike,
On 4/29/19 9:39 AM, Longpeng(Mike) wrote:
> From: Longpeng <longpeng2@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().
>
> xhci_doorbell_write() already check the upper bound of @slotid an @epid,
> it also need to check the lower bound.
>
> Cc: Gonglei <arei.gonglei@huawei.com>
> Signed-off-by: Longpeng <longpeng2@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...
> DPRINTF("xhci: bad doorbell %d\n", (int)reg);
> - } else if (epid > 31) {
> + } else if (epid == 0 || epid > 31) {
Here neither.
> 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.
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 11:42 ` Longpeng (Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng (Mike) @ 2019-04-29 11:42 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: kraxel, Gonglei, qemu-devel
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 <longpeng2@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=...)
#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
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.gonglei@huawei.com>
>> Signed-off-by: Longpeng <longpeng2@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.
>> 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)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 11:42 ` Longpeng (Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng (Mike) @ 2019-04-29 11:42 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Gonglei, kraxel, qemu-devel
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 <longpeng2@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=...)
#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
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.gonglei@huawei.com>
>> Signed-off-by: Longpeng <longpeng2@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.
>> 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)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 12:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-29 12:10 UTC (permalink / raw)
To: Longpeng (Mike); +Cc: kraxel, Gonglei, qemu-devel
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 <longpeng2@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.
> 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.gonglei@huawei.com>
>>> Signed-off-by: Longpeng <longpeng2@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.
>> assert(epid >= 1 && epid <= 31);
>>
>> Regards,
>>
>> Phil.
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 12:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-29 12:10 UTC (permalink / raw)
To: Longpeng (Mike); +Cc: Gonglei, kraxel, qemu-devel
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 <longpeng2@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.
> 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.gonglei@huawei.com>
>>> Signed-off-by: Longpeng <longpeng2@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.
>> assert(epid >= 1 && epid <= 31);
>>
>> Regards,
>>
>> Phil.
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 15:05 ` no-reply
0 siblings, 0 replies; 19+ messages in thread
From: no-reply @ 2019-04-29 15:05 UTC (permalink / raw)
To: longpeng2; +Cc: fam, kraxel, arei.gonglei, qemu-devel
Patchew URL: https://patchew.org/QEMU/1556523569-44480-1-git-send-email-longpeng2@huawei.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
The full log is available at
http://patchew.org/logs/1556523569-44480-1-git-send-email-longpeng2@huawei.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 15:05 ` no-reply
0 siblings, 0 replies; 19+ messages in thread
From: no-reply @ 2019-04-29 15:05 UTC (permalink / raw)
To: longpeng2; +Cc: fam, longpeng2, arei.gonglei, kraxel, qemu-devel
Patchew URL: https://patchew.org/QEMU/1556523569-44480-1-git-send-email-longpeng2@huawei.com/
Hi,
This series failed the docker-mingw@fedora build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-mingw@fedora SHOW_ENV=1 J=14 NETWORK=1
=== TEST SCRIPT END ===
The full log is available at
http://patchew.org/logs/1556523569-44480-1-git-send-email-longpeng2@huawei.com/testing.docker-mingw@fedora/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 15:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-29 15:10 UTC (permalink / raw)
To: qemu-devel, no-reply, longpeng2; +Cc: fam, arei.gonglei, kraxel, patchew-devel
Hi Paolo, Fam.
On 4/29/19 5:05 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/1556523569-44480-1-git-send-email-longpeng2@huawei.com/
[...]
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
[...]
> The full log is available at
> http://patchew.org/logs/1556523569-44480-1-git-send-email-longpeng2@huawei.com/testing.docker-mingw@fedora/?type=message.
Weird patchew error:
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
>From https://github.com/patchew-project/qemu
- [tag update]
patchew/1554321185-2825-1-git-send-email-sandra@codesourcery.com ->
patchew/1554321185-2825-1-git-send-email-sandra@codesourcery.com
* [new tag]
patchew/1556523569-44480-1-git-send-email-longpeng2@huawei.com ->
patchew/1556523569-44480-1-git-send-email-longpeng2@huawei.com
[...]
error: rev-list died of signal 9
fatal: remote did not send all necessary objects
fatal: The remote end hung up unexpectedly
Traceback (most recent call last):
File "patchew-tester/src/patchew-cli", line 521, in test_one
git_clone_repo(clone, r["repo"], r["head"], logf, True)
File "patchew-tester/src/patchew-cli", line 53, in git_clone_repo
subprocess.check_call(clone_cmd, stderr=logf, stdout=logf)
File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
raise CalledProcessError(retcode, cmd)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 15:10 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-29 15:10 UTC (permalink / raw)
To: qemu-devel, no-reply, longpeng2; +Cc: fam, patchew-devel, arei.gonglei, kraxel
Hi Paolo, Fam.
On 4/29/19 5:05 PM, no-reply@patchew.org wrote:
> Patchew URL: https://patchew.org/QEMU/1556523569-44480-1-git-send-email-longpeng2@huawei.com/
[...]
> This series failed the docker-mingw@fedora build test. Please find the testing commands and
> their output below. If you have Docker installed, you can probably reproduce it
> locally.
[...]
> The full log is available at
> http://patchew.org/logs/1556523569-44480-1-git-send-email-longpeng2@huawei.com/testing.docker-mingw@fedora/?type=message.
Weird patchew error:
Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
From https://github.com/patchew-project/qemu
- [tag update]
patchew/1554321185-2825-1-git-send-email-sandra@codesourcery.com ->
patchew/1554321185-2825-1-git-send-email-sandra@codesourcery.com
* [new tag]
patchew/1556523569-44480-1-git-send-email-longpeng2@huawei.com ->
patchew/1556523569-44480-1-git-send-email-longpeng2@huawei.com
[...]
error: rev-list died of signal 9
fatal: remote did not send all necessary objects
fatal: The remote end hung up unexpectedly
Traceback (most recent call last):
File "patchew-tester/src/patchew-cli", line 521, in test_one
git_clone_repo(clone, r["repo"], r["head"], logf, True)
File "patchew-tester/src/patchew-cli", line 53, in git_clone_repo
subprocess.check_call(clone_cmd, stderr=logf, stdout=logf)
File "/usr/lib64/python3.4/subprocess.py", line 558, in check_call
raise CalledProcessError(retcode, cmd)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 17:58 ` no-reply
0 siblings, 0 replies; 19+ messages in thread
From: no-reply @ 2019-04-29 17:58 UTC (permalink / raw)
To: longpeng2; +Cc: fam, kraxel, arei.gonglei, qemu-devel
Patchew URL: https://patchew.org/QEMU/1556523569-44480-1-git-send-email-longpeng2@huawei.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
The full log is available at
http://patchew.org/logs/1556523569-44480-1-git-send-email-longpeng2@huawei.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-29 17:58 ` no-reply
0 siblings, 0 replies; 19+ messages in thread
From: no-reply @ 2019-04-29 17:58 UTC (permalink / raw)
To: longpeng2; +Cc: fam, longpeng2, arei.gonglei, kraxel, qemu-devel
Patchew URL: https://patchew.org/QEMU/1556523569-44480-1-git-send-email-longpeng2@huawei.com/
Hi,
This series failed the asan build test. Please find the testing commands and
their output below. If you have Docker installed, you can probably reproduce it
locally.
=== TEST SCRIPT BEGIN ===
#!/bin/bash
time make docker-test-debug@fedora TARGET_LIST=x86_64-softmmu J=14 NETWORK=1
=== TEST SCRIPT END ===
The full log is available at
http://patchew.org/logs/1556523569-44480-1-git-send-email-longpeng2@huawei.com/testing.asan/?type=message.
---
Email generated automatically by Patchew [https://patchew.org/].
Please send your feedback to patchew-devel@redhat.com
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-30 2:02 ` Longpeng (Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng (Mike) @ 2019-04-30 2:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: kraxel, Gonglei, qemu-devel
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 <longpeng2@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.gonglei@huawei.com>
>>>> Signed-off-by: Longpeng <longpeng2@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.
>>> assert(epid >= 1 && epid <= 31);
>>>
>>> Regards,
>>>
>>> Phil.
>>>
>
> .
>
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-30 2:02 ` Longpeng (Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng (Mike) @ 2019-04-30 2:02 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Gonglei, kraxel, qemu-devel
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 <longpeng2@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.gonglei@huawei.com>
>>>> Signed-off-by: Longpeng <longpeng2@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.
>>> assert(epid >= 1 && epid <= 31);
>>>
>>> Regards,
>>>
>>> Phil.
>>>
>
> .
>
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-30 5:06 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-30 5:06 UTC (permalink / raw)
To: Longpeng (Mike); +Cc: kraxel, Gonglei, qemu-devel
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 <longpeng2@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.gonglei@huawei.com>
>>>>> Signed-off-by: Longpeng <longpeng2@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é <philmd@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.
>>>>
>>
>> .
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-30 5:06 ` Philippe Mathieu-Daudé
0 siblings, 0 replies; 19+ messages in thread
From: Philippe Mathieu-Daudé @ 2019-04-30 5:06 UTC (permalink / raw)
To: Longpeng (Mike); +Cc: Gonglei, kraxel, qemu-devel
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 <longpeng2@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.gonglei@huawei.com>
>>>>> Signed-off-by: Longpeng <longpeng2@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é <philmd@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.
>>>>
>>
>> .
>>
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-30 5:37 ` Longpeng (Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng (Mike) @ 2019-04-30 5:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: kraxel, Gonglei, qemu-devel
On 2019/4/30 13:06, Philippe Mathieu-Daudé wrote:
> 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 <longpeng2@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.gonglei@huawei.com>
>>>>>> Signed-off-by: Longpeng <longpeng2@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é <philmd@redhat.com>
>
> (Please include the full backtrace in the description).
>
OK.
> Thanks for being patient with me with this patch :)
>
It is a good learning experience for me too :)
>>>>> assert(epid >= 1 && epid <= 31);
>>>>>
>>>>> Regards,
>>>>>
>>>>> Phil.
>>>>>
>>>
>>> .
>>>
>
> .
>
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid
@ 2019-04-30 5:37 ` Longpeng (Mike)
0 siblings, 0 replies; 19+ messages in thread
From: Longpeng (Mike) @ 2019-04-30 5:37 UTC (permalink / raw)
To: Philippe Mathieu-Daudé; +Cc: Gonglei, kraxel, qemu-devel
On 2019/4/30 13:06, Philippe Mathieu-Daudé wrote:
> 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 <longpeng2@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.gonglei@huawei.com>
>>>>>> Signed-off-by: Longpeng <longpeng2@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é <philmd@redhat.com>
>
> (Please include the full backtrace in the description).
>
OK.
> Thanks for being patient with me with this patch :)
>
It is a good learning experience for me too :)
>>>>> assert(epid >= 1 && epid <= 31);
>>>>>
>>>>> Regards,
>>>>>
>>>>> Phil.
>>>>>
>>>
>>> .
>>>
>
> .
>
--
Regards,
Longpeng(Mike)
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2019-04-30 5:38 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-29 7:39 [Qemu-devel] [PATCH] usb/xchi: avoid trigger assertion if guest write wrong epid Longpeng(Mike)
2019-04-29 7:39 ` Longpeng(Mike)
2019-04-29 11:16 ` Philippe Mathieu-Daudé
2019-04-29 11:42 ` Longpeng (Mike)
2019-04-29 11:42 ` Longpeng (Mike)
2019-04-29 12:10 ` Philippe Mathieu-Daudé
2019-04-29 12:10 ` Philippe Mathieu-Daudé
2019-04-30 2:02 ` Longpeng (Mike)
2019-04-30 2:02 ` Longpeng (Mike)
2019-04-30 5:06 ` Philippe Mathieu-Daudé
2019-04-30 5:06 ` Philippe Mathieu-Daudé
2019-04-30 5:37 ` Longpeng (Mike)
2019-04-30 5:37 ` Longpeng (Mike)
2019-04-29 15:05 ` no-reply
2019-04-29 15:05 ` no-reply
2019-04-29 15:10 ` Philippe Mathieu-Daudé
2019-04-29 15:10 ` Philippe Mathieu-Daudé
2019-04-29 17:58 ` no-reply
2019-04-29 17:58 ` no-reply
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.