Re: [Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

Re: [Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

[anonym]

Gentle ping! :)

I've lost the Message-IDs of my previous posts on this topic but its about this patch submission:

* https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg03433.html
* https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg03434.html

Cheers!

> Hi, list!
> 
> I guess you can take this as a ping for this patch submission. We'd very much 
> like this to be dealt with soon, so the fix can land into Debian Stretch before 
> it is released, otherwise some of us will have to work around this issue for 
> the next two years. :/
> 
> Hi, Gerd!
> 
> You are the author of QEMU commit 05f43d4 which fixed CVE-2016-8576, which I 
> believe introduced the regression described below. Do you think my patch's 
> approach is safe and makes sense?
> 
> At the moment this issue is causing trouble for Tails [1] since its installer 
> hits this bug, which affects both users of QEMU + Tails, and our automated QA 
> infrastructure, which uses QEMU.
> 
> Cheers!
> 
> [1] https://tails.boum.org/
> 
> address@hidden:
>> From: anonym <address@hidden>
>> 
>> While formatting partitions (on virtual USB drives and the nec-xhci
>> virtual USB controller) to EXT4, I have observed errors like these:
>> 
>>     kernel: sd 8:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ABORT
>>     driverbyte=DRIVER_OK
>>     kernel: sd 8:0:0:0: [sda] tag#0 CDB: Write(10) 2a 00 00 66 49 86
>>     00 08 00 00
>>     kernel: blk_update_request: I/O error, dev sda, sector 6703494
>>     kernel: Buffer I/O error on dev dm-0, logical block 1573254, lost
>>     async page write
>> 
>> Raising TRB_LINK_LIMIT fixes the limit, but the new value was
>> admittedly arbitrarily chosen.
>> 
>> Regarding cycle detection in general, allowing at most 4 levels of
>> links seems pretty low. This bump should be safe: a high number only
>> means that we get a performance hit when encountering cycles but then
>> we should have a fatal error any way; a low number instead means that
>> we'll incorrectly identify cycles and abort operations that otherwise
>> would succeed, like in the case above.
>> 
>> Signed-off-by: anonym <address@hidden>
>> ---
>>  hw/usb/hcd-xhci.c | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>> 
>> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
>> index 4acf0c6dd8..d14ce126a2 100644
>> --- a/hw/usb/hcd-xhci.c
>> +++ b/hw/usb/hcd-xhci.c
>> @@ -53,7 +53,7 @@
>>   * to the specs when it gets them */
>>  #define ER_FULL_HACK
>>  
>> -#define TRB_LINK_LIMIT  4
>> +#define TRB_LINK_LIMIT  32
>>  
>>  #define LEN_CAP         0x40
>>  #define LEN_OPER        (0x400 + 0x10 * MAXPORTS)

Re: [Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

[Gerd Hoffmann]

On Fr, 2017-05-19 at 19:42 +0000, anonym wrote:
> Gentle ping! :)
> 
> I've lost the Message-IDs of my previous posts on this topic but its about this patch submission:
> 
> * https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg03433.html
> * https://lists.nongnu.org/archive/html/qemu-devel/2017-01/msg03434.html
> 

That one slipped though.  Same thing was noticed by someone else
meanwhile though and it is fixed in master (commit
99f9aeba5d461f79c9ce73f968ba0feb77fc1f5a).

cheers,
  Gerd

[Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

[anonym]

Hi!

[Please keep me Cc:ed as I am not subscribed to this list!]

It seems the fix of CVE-2016-8576 (commit 05f43d4) introduced a regression in QEMU 2.8.

Cheers!

[Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

[anonym]

From: anonym <anonym@riseup.net>

While formatting partitions (on virtual USB drives and the nec-xhci
virtual USB controller) to EXT4, I have observed errors like these:

    kernel: sd 8:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ABORT
    driverbyte=DRIVER_OK
    kernel: sd 8:0:0:0: [sda] tag#0 CDB: Write(10) 2a 00 00 66 49 86
    00 08 00 00
    kernel: blk_update_request: I/O error, dev sda, sector 6703494
    kernel: Buffer I/O error on dev dm-0, logical block 1573254, lost
    async page write

Raising TRB_LINK_LIMIT fixes the limit, but the new value was
admittedly arbitrarily chosen.

Regarding cycle detection in general, allowing at most 4 levels of
links seems pretty low. This bump should be safe: a high number only
means that we get a performance hit when encountering cycles but then
we should have a fatal error any way; a low number instead means that
we'll incorrectly identify cycles and abort operations that otherwise
would succeed, like in the case above.

Signed-off-by: anonym <anonym@riseup.net>
---
 hw/usb/hcd-xhci.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
index 4acf0c6dd8..d14ce126a2 100644
--- a/hw/usb/hcd-xhci.c
+++ b/hw/usb/hcd-xhci.c
@@ -53,7 +53,7 @@
  * to the specs when it gets them */
 #define ER_FULL_HACK
 
-#define TRB_LINK_LIMIT  4
+#define TRB_LINK_LIMIT  32
 
 #define LEN_CAP         0x40
 #define LEN_OPER        (0x400 + 0x10 * MAXPORTS)
-- 
2.11.0

Re: [Qemu-devel] [PATCH] xhci: bump the link TRB cycle detection limit.

[anonym]

Hi, list!

I guess you can take this as a ping for this patch submission. We'd very much like this to be dealt with soon, so the fix can land into Debian Stretch before it is released, otherwise some of us will have to work around this issue for the next two years. :/

Hi, Gerd!

You are the author of QEMU commit 05f43d4 which fixed CVE-2016-8576, which I believe introduced the regression described below. Do you think my patch's approach is safe and makes sense?

At the moment this issue is causing trouble for Tails [1] since its installer hits this bug, which affects both users of QEMU + Tails, and our automated QA infrastructure, which uses QEMU.

Cheers!

[1] https://tails.boum.org/

anonym@riseup.net:
> From: anonym <anonym@riseup.net>
> 
> While formatting partitions (on virtual USB drives and the nec-xhci
> virtual USB controller) to EXT4, I have observed errors like these:
> 
>     kernel: sd 8:0:0:0: [sda] tag#0 FAILED Result: hostbyte=DID_ABORT
>     driverbyte=DRIVER_OK
>     kernel: sd 8:0:0:0: [sda] tag#0 CDB: Write(10) 2a 00 00 66 49 86
>     00 08 00 00
>     kernel: blk_update_request: I/O error, dev sda, sector 6703494
>     kernel: Buffer I/O error on dev dm-0, logical block 1573254, lost
>     async page write
> 
> Raising TRB_LINK_LIMIT fixes the limit, but the new value was
> admittedly arbitrarily chosen.
> 
> Regarding cycle detection in general, allowing at most 4 levels of
> links seems pretty low. This bump should be safe: a high number only
> means that we get a performance hit when encountering cycles but then
> we should have a fatal error any way; a low number instead means that
> we'll incorrectly identify cycles and abort operations that otherwise
> would succeed, like in the case above.
> 
> Signed-off-by: anonym <anonym@riseup.net>
> ---
>  hw/usb/hcd-xhci.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/hw/usb/hcd-xhci.c b/hw/usb/hcd-xhci.c
> index 4acf0c6dd8..d14ce126a2 100644
> --- a/hw/usb/hcd-xhci.c
> +++ b/hw/usb/hcd-xhci.c
> @@ -53,7 +53,7 @@
>   * to the specs when it gets them */
>  #define ER_FULL_HACK
>  
> -#define TRB_LINK_LIMIT  4
> +#define TRB_LINK_LIMIT  32
>  
>  #define LEN_CAP         0x40
>  #define LEN_OPER        (0x400 + 0x10 * MAXPORTS)
>