linux-kernel - RE: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in __dwc3_prepare_one

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <BN3PR0201MB0993325C7125DB1A084BF9BBA7020@BN3PR0201MB0993.namprd02.prod.outlook.com>
Date:   Wed, 5 Sep 2018 09:19:40 +0000
From:   Anurag Kumar Vulisha <anuragku@...inx.com>
To:     Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
        "balbi@...nel.org" <balbi@...nel.org>,
        "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>
CC:     "v.anuragkumar@...il.com" <v.anuragkumar@...il.com>,
        "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: RE: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full
 in __dwc3_prepare_one_trb()

Hi Thinh,

Thanks for spending your time in reviewing this code, please find my comments inline

>-----Original Message-----
>From: Thinh Nguyen [mailto:Thinh.Nguyen@...opsys.com]
>Sent: Wednesday, September 05, 2018 11:04 AM
>To: Anurag Kumar Vulisha <anuragku@...inx.com>; balbi@...nel.org;
>gregkh@...uxfoundation.org
>Cc: v.anuragkumar@...il.com; linux-usb@...r.kernel.org; linux-
>kernel@...r.kernel.org
>Subject: Re: [PATCH v2 1/8] usb: dwc3: Correct the logic for checking TRB full in
>__dwc3_prepare_one_trb()
>
>Hi Anurag,
>
>On 8/17/2018 5:25 AM, Anurag Kumar Vulisha wrote:
>> Availability of TRB's are calculated using dwc3_calc_trbs_left(), which
>> determines available TRB's based on the HWO bit set in a TRB.
>>
>> __dwc3_prepare_one_trb() is called with a TRB which needs to be prepared
>> for transfer. This __dwc3_prepare_one_trb() calls dwc3_calc_trbs_left()
>> to determine total available TRBs and set IOC bit if the total available
>> TRBs are zero. Since the present working TRB(which is passed as an
>> argument to __dwc3_prepare_one_trb() )  doesn't have HWO bit already set,
>> there are chances where dwc3_calc_trbs_left() wrongly calculates this
>> present working TRB as free(since the HWO bit is not yet set). This could
>> be a problem. This patch correct this issue by setting HWO bit before
>> calling dwc3_calc_trbs_left()
>>
>> Signed-off-by: Anurag Kumar Vulisha <anurag.kumar.vulisha@...inx.com>
>> ---
>>  Changes in v2:
>> 	1. Changed the commit message
>> ---
>>  drivers/usb/dwc3/gadget.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/usb/dwc3/gadget.c b/drivers/usb/dwc3/gadget.c
>> index 69bf137..f73d219 100644
>> --- a/drivers/usb/dwc3/gadget.c
>> +++ b/drivers/usb/dwc3/gadget.c
>> @@ -990,6 +990,8 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep *dep,
>struct dwc3_trb *trb,
>>  			trb->ctrl |= DWC3_TRB_CTRL_ISP_IMI;
>>  	}
>>
>> +	trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> +
>>  	if ((!no_interrupt && !chain) ||
>>  			(dwc3_calc_trbs_left(dep) == 0))
>>  		trb->ctrl |= DWC3_TRB_CTRL_IOC;
>> @@ -1000,8 +1002,6 @@ static void __dwc3_prepare_one_trb(struct dwc3_ep
>*dep, struct dwc3_trb *trb,
>>  	if (usb_endpoint_xfer_bulk(dep->endpoint.desc) && dep->stream_capable)
>>  		trb->ctrl |= DWC3_TRB_CTRL_SID_SOFN(stream_id);
>>
>> -	trb->ctrl |= DWC3_TRB_CTRL_HWO;
>> -
>>  	trace_dwc3_prepare_trb(dep, trb);
>>  }
>>
>How do you reproduce this issue? We should not set HWO before setting
>other trb->ctrl bits. Can you provide a driver tracepoint? If there's an
>issue with the check if ((!no_interrupt && !chain) ||
>dwc3_calc_trbs_left == 0), then we may need to fix the check there.
>

This issue gets triggered very rarely on long runs when dep->trb_enqueue == dep->trb_dequeue.
In __dwc3_prepare_one_trb() , IOC bit is set when no TRBs are available, so that a complete
event can be generated and TRBs can be cleaned  after complete . Dwc3_calc_trbs_left() is called 
to determine the available TRBs, which depends on the previous TRB's HWO bit set when 
dep->trb_enqueue == dep->trb_dequeue. There are chances where the dwc3_calc_trbs_left() wrongly
returns DWC3_TRB_NUM -1 instead of 0 , even though there are no available TRBs. Please
consider the below example

1.  Consider a TRB passed to __dwc3_prepare_one_trb() is the last available TRB in the pool. 
2.  __dwc3_prepare_one_trb() calls dwc3_ep_inc_enq() which increments the dep->trb_enqueue
   before preparing the TRB and since the current TRB is the last available, incrementing
  dep->enqueue will make dep->enqueue == dep->dequeue 
3. IOC bit is set by __dwc3_prepare_one_trb()  only when dwc3_calc_trbs_left()  returns 0 (empty TRBs)
4. Since dep->enqueue == dep->dequeue and the previous TRB(the one which we are working)
    doesn't yet has the HWO bit set, dwc3_calc_trbs_left() returns DWC3_TRB_NUM -1 instead of
    zero (Though there are no available TRBs)
5. Since  Dwc3_calc_trbs_left() returns non-zero value, IOC bit is not set in __dwc3_prepare_one_trb()
   for the last TRB and no complete event is generated. Because of this no further TRBs are queued.

To avoid the above mentioned issue, I moved the code logic for setting HWO bit before setting IOC bit.
I agree that HWO bit should always be set at the last, but I didn't find any better logic for fixing this.
Please suggest if any better way to handle this situation.   

Thanks,
Anurag Kumar Vulisha