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)