linux-kernel - RE: [PATCH] usb: xhci: fixes lost of data for xHCI Cadence Controllers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID:
 <PH7PR07MB953853EB6AF92AD5ED02A304DD8D2@PH7PR07MB9538.namprd07.prod.outlook.com>
Date: Tue, 20 Aug 2024 09:25:29 +0000
From: Pawel Laszczak <pawell@...ence.com>
To: Mathias Nyman <mathias.nyman@...ux.intel.com>,
        "mathias.nyman@...el.com"
	<mathias.nyman@...el.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
        "peter.chen@...nel.org" <peter.chen@...nel.org>,
        "linux-usb@...r.kernel.org"
	<linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>,
        "stable@...r.kernel.org"
	<stable@...r.kernel.org>
Subject: RE: [PATCH] usb: xhci: fixes lost of data for xHCI Cadence
 Controllers

>> Stream endpoint can skip part of TD during next transfer
>> initialization after beginning stopped during active stream data transfer.
>> The Set TR Dequeue Pointer command does not clear all internal
>> transfer-related variables that position stream endpoint on transfer ring.
>>
>> USB Controller stores all endpoint state information within RsvdO
>> fields inside endpoint context structure. For stream endpoints, all
>> relevant information regarding particular StreamID is stored within
>> corresponding Stream Endpoint context.
>> Whenever driver wants to stop stream endpoint traffic, it invokes Stop
>> Endpoint command which forces the controller to dump all endpoint
>> state-related variables into RsvdO spaces into endpoint context and
>> stream endpoint context. Whenever driver wants to reinitialize
>> endpoint starting point on Transfer Ring, it uses the Set TR Dequeue
>> Pointer command to update dequeue pointer for particular stream in
>> Stream Endpoint Context. When stream endpoint is forced to stop active
>> transfer in the middle of TD, it dumps an information about TRB bytes
>> left in RsvdO fields in Stream Endpoint Context which will be used in
>> next transfer initialization to designate starting point for XDMA.
>> This field is not cleared during Set TR Dequeue Pointer command which
>> causes XDMA to skip over transfer ring and leads to data loss on stream
>pipe.
>
>xHC should clear the EDTLA field when processing a Set TR Dequeue Pointer
>command:
>
>xhci spec v1.2, section 4.6.10 Set TR Dequeue Pointer:
>"The xHC shall perform the following operations when setting a ring address:
>  ...
>Clear any prior transfer state, e.g. setting the EDTLA to 0, clearing any partially
>completed USB2 split transactions, etc."
>
>>
>> Patch fixes this by clearing out all RsvdO fields before initializing
>> new transfer via that StreamID.
>
>Looks like patch also writes edtl back to ctx->reserved[0], is there a reason for
>this?
>
>>
>> Field Rsvd0 is reserved field, so patch should not have impact for
>> other xHCI controllers.
>>
>> Fixes: 3d82904559f4 ("usb: cdnsp: cdns3 Add main part of Cadence
>> USBSSP DRD Driver")
>> cc: <stable@...r.kernel.org>
>> Signed-off-by: Pawel Laszczak <pawell@...ence.com>
>> ---
>>   drivers/usb/host/xhci-ring.c | 13 +++++++++++++
>>   1 file changed, 13 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c
>> b/drivers/usb/host/xhci-ring.c index 1dde53f6eb31..7fc1c4efcae2 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -1385,7 +1385,20 @@ static void xhci_handle_cmd_set_deq(struct
>xhci_hcd *xhci, int slot_id,
>>   		if (ep->ep_state & EP_HAS_STREAMS) {
>>   			struct xhci_stream_ctx *ctx =
>>   				&ep->stream_info-
>>stream_ctx_array[stream_id];
>> +			u32 edtl;
>> +
>>   			deq = le64_to_cpu(ctx->stream_ring) &
>SCTX_DEQ_MASK;
>> +			edtl = EVENT_TRB_LEN(le32_to_cpu(ctx-
>>reserved[1]));
>
>Isn't edtl in reserved[0], not in reserved[1]?
>
>Also unclear why we read it at all, it should be set to 0 by controller, right?
>
>> +
>> +			/*
>> +			 * Existing Cadence xHCI controllers store some
>endpoint state information
>> +			 * within Rsvd0 fields of Stream Endpoint context. This
>field is
>> +not
>
>Aren't these fields RsvdO (Reserved and Opaque)?
>Driver shouldn't normally touch these fields, they are used by xHC as
>temporary workspace.

Yes, it should be. This a real issue that occurred during testing UASP disk and
this workaround fixes it, so it's looks like that this Rsvd0 field can be changed.

>
>> +			 * cleared during Set TR Dequeue Pointer command
>which causes XDMA to skip
>> +			 * over transfer ring and leads to data loss on stream
>pipe.
>> +			 * To fix this issue driver must clear Rsvd0 field.
>> +			 */
>> +			ctx->reserved[1] = 0;
>> +			ctx->reserved[0] = cpu_to_le32(edtl);
>
>why are we writing back edtl?, also note that it's read from ctx->reserved[1]
>above, and now written to back to ctx->reserved[0].
>
>I understood that issue was those RsvdO fields needs to be cleared manually
>for this host as hardware fails to clear them during a "Set TR Dequeue
>Pointer" command.
>
>Am I misunderstanding something?

Yes, you understand it so well.

In fact edtl should be zeroed and simple clearing of reserved[0] and reserved[1]  should be sufficient.

Thanks 
Pawel

>
>Thanks
>Mathias