linux-kernel - Re: [PATCH 17/24] usb: host: xhci: Remove unused variable 'len'

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]

Date:   Thu, 27 May 2021 12:23:24 +0300
From:   Mathias Nyman <mathias.nyman@...ux.intel.com>
To:     Lee Jones <lee.jones@...aro.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:     linux-kernel@...r.kernel.org,
        Mathias Nyman <mathias.nyman@...el.com>,
        linux-usb@...r.kernel.org
Subject: Re: [PATCH 17/24] usb: host: xhci: Remove unused variable 'len'

On 27.5.2021 11.16, Lee Jones wrote:
> On Thu, 27 May 2021, Greg Kroah-Hartman wrote:
> 
>> On Wed, May 26, 2021 at 02:00:30PM +0100, Lee Jones wrote:
>>> Fixes the following W=1 kernel build warning(s):
>>>
>>>  drivers/usb/host/xhci.c: In function ‘xhci_unmap_temp_buf’:
>>>  drivers/usb/host/xhci.c:1349:15: warning: variable ‘len’ set but not used [-Wunused-but-set-variable]
>>>
>>> Cc: Mathias Nyman <mathias.nyman@...el.com>
>>> Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
>>> Cc: linux-usb@...r.kernel.org
>>> Signed-off-by: Lee Jones <lee.jones@...aro.org>
>>> ---
>>>  drivers/usb/host/xhci.c | 9 ++++-----
>>>  1 file changed, 4 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
>>> index 27283654ca080..ac2a7d4288883 100644
>>> --- a/drivers/usb/host/xhci.c
>>> +++ b/drivers/usb/host/xhci.c
>>> @@ -1346,7 +1346,6 @@ static bool xhci_urb_temp_buffer_required(struct usb_hcd *hcd,
>>>  
>>>  static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb)
>>>  {
>>> -	unsigned int len;
>>>  	unsigned int buf_len;
>>>  	enum dma_data_direction dir;
>>>  
>>> @@ -1362,10 +1361,10 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb)
>>>  				 dir);
>>>  
>>>  	if (usb_urb_dir_in(urb))
>>> -		len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs,
>>> -					   urb->transfer_buffer,
>>> -					   buf_len,
>>> -					   0);
>>> +		sg_pcopy_from_buffer(urb->sg, urb->num_sgs,
>>> +				     urb->transfer_buffer,
>>> +				     buf_len,
>>> +				     0);
>>
>> Sorry, but no, I keep rejecting this over and over, it needs to handle
>> the error handling properly and not paper over it like this :(
> 
> Will fix.
> 
>> All the bots keep tripping up on it, you are not alone.
> 

This is getting a lot of attention. Something like this should fix it:

diff --git a/drivers/usb/host/xhci.c b/drivers/usb/host/xhci.c
index 27283654ca08..306ab81421fd 100644
--- a/drivers/usb/host/xhci.c
+++ b/drivers/usb/host/xhci.c
@@ -1361,12 +1361,16 @@ static void xhci_unmap_temp_buf(struct usb_hcd *hcd, struct urb *urb)
 				 urb->transfer_buffer_length,
 				 dir);
 
-	if (usb_urb_dir_in(urb))
+	if (usb_urb_dir_in(urb)) {
 		len = sg_pcopy_from_buffer(urb->sg, urb->num_sgs,
 					   urb->transfer_buffer,
 					   buf_len,
 					   0);
-
+		if (len != buf_len) {
+			xhci_dbg(xhci, "Copy from tmp buf to urb sg list failed\n");
+			urb->actual_length = len;
+		}
+	}
 	urb->transfer_flags &= ~URB_DMA_MAP_SINGLE;
 	kfree(urb->transfer_buffer);
 	urb->transfer_buffer = NULL;
 
urb->actual_length is now properly set.
The debug level message will help me find the cause if we ever need
to debug oddly behaving devices.

Note this is a very rarly taken codepath for quirky xHC harware that
can't handle a specific sequence of buffer lengths queued.

I can write a proper commit message and push this forward

-Mathias