linux-kernel - Re: [PATCH linux] usb: xhci-ring: Add return if ret is less than 0

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]

Date:   Thu, 30 Dec 2021 15:31:52 +0200
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Greg Kroah-Hartman <gregkh@...uxfoundation.org>, cgel.zte@...il.com
Cc:     Mathias Nyman <mathias.nyman@...el.com>, linux-usb@...r.kernel.org,
        linux-kernel@...r.kernel.org, luo penghao <luo.penghao@....com.cn>,
        Zeal Robot <zealci@....com.cn>
Subject: Re: [PATCH linux] usb: xhci-ring: Add return if ret is less than 0

On 30.12.2021 9.02, Greg Kroah-Hartman wrote:
> On Thu, Dec 30, 2021 at 06:40:10AM +0000, cgel.zte@...il.com wrote:
>> From: luo penghao <luo.penghao@....com.cn>
>>
>> For the robustness of the code, judgment and return should be added here
> 
> I do not understand this changelog text at all.  Please explain the
> problem and why you are making this change much better.
> 

Agree, this doesn't explain at all what is going on.

So looking at the code it checks if a zero-length transfer after a bulk transfer is
properly prepared before queuing the TRB to hardware.

Nothing wrong with that. We do check that the main part of the bulk transfer
is properly prepared before this, so it's very unlikely to fail, but not impossible. 

>>
>> The clang_analyzer complains as follows:
>>
>> drivers/usb/host/xhci-ring.c:
>>
>> Value stored to 'ret' is never read
>>
>> Reported-by: Zeal Robot <zealci@....com.cn>
>> Signed-off-by: luo penghao <luo.penghao@....com.cn>
>> ---
>>  drivers/usb/host/xhci-ring.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
>> index d0b6806..c4eefe2 100644
>> --- a/drivers/usb/host/xhci-ring.c
>> +++ b/drivers/usb/host/xhci-ring.c
>> @@ -3721,6 +3721,8 @@ int xhci_queue_bulk_tx(struct xhci_hcd *xhci, gfp_t mem_flags,
>>  		ret = prepare_transfer(xhci, xhci->devs[slot_id],
>>  				       ep_index, urb->stream_id,
>>  				       1, urb, 1, mem_flags);
>> +		if (unlikely(ret < 0))
>> +			return ret;

We can't just return if it fails. 
We already queued the main part of the bulk transfer to the ring, but haven't given
those TRBs to hardware yet. This is done in giveback_first_trb() a few lines later.

As this case probably won't happen, we could just add a small debug message here,
and skip the zero-length packet. 
Queue the main part of the bulk transfer and give it to hardware anyway.
It will probably time out later.

>>  		urb_priv->td[1].last_trb = ring->enqueue;
>>  		urb_priv->td[1].last_trb_seg = ring->enq_seg;
>>  		field = TRB_TYPE(TRB_NORMAL) | ring->cycle_state | TRB_IOC;
>> -- 
>> 2.15.2
>>
>>
> 
> How did you test this change?

Wondering the same.

Suggestion:
Add a hack to detect a zero-length transfer in prepare_transfer(), and intentionally 
fail (return error) in places prepare_transfer() could normally fail.

And then check that the system behaves better with your patch than without.

-Mathias