linux-kernel - Re: [PATCH] xhci: free the correct ring

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 for Android: free password hash cracker in your pocket

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

Date:	Thu, 30 Jun 2016 17:21:57 +0300
From:	Mathias Nyman <mathias.nyman@...el.com>
To:	Arnd Bergmann <arnd@...db.de>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>
Cc:	linux-usb@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xhci: free the correct ring

On 30.06.2016 15:26, Arnd Bergmann wrote:
> gcc warns about what first looks like a reference to an uninitialized
> variable:
>
> drivers/usb/host/xhci-ring.c: In function 'handle_cmd_completion':
> drivers/usb/host/xhci-ring.c:753:4: error: 'ep_ring' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      xhci_unmap_td_bounce_buffer(xhci, ep_ring, cur_td);
>      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> drivers/usb/host/xhci-ring.c:647:20: note: 'ep_ring' was declared here
>    struct xhci_ring *ep_ring;
>                      ^~~~~~~
>
> It's clear to see that the list_empty() check means it can never be
> uninitialized, however it still looks wrong:
>
> When ep->cancelled_td_list contains more than one entry, the
> ep_ring variable will point to the ring that was retrieved
> from the last urb, and we have to look it up again in the
> second loop instead, which fixes the behavior and gets rid of the
> warning too.
>
> Signed-off-by: Arnd Bergmann <arnd@...db.de>
> Fixes: f9c589e142d0 ("xhci: TD-fragment, align the unsplittable case with a bounce buffer")
> ---
>   drivers/usb/host/xhci-ring.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/usb/host/xhci-ring.c b/drivers/usb/host/xhci-ring.c
> index 21e1dd62ebf8..918e0c739b79 100644
> --- a/drivers/usb/host/xhci-ring.c
> +++ b/drivers/usb/host/xhci-ring.c
> @@ -749,6 +749,7 @@ remove_finished_td:
>   		/* Doesn't matter what we pass for status, since the core will
>   		 * just overwrite it (because the URB has been unlinked).
>   		 */
> +		ep_ring = xhci_urb_to_transfer_ring(xhci, cur_td->urb);
>   		if (ep_ring && cur_td->bounce_seg)
>   			xhci_unmap_td_bounce_buffer(xhci, ep_ring, cur_td);
>   		xhci_giveback_urb_in_irq(xhci, cur_td, 0);
>

Thanks,

Acked-by: Mathias Nyman <mathias.nyman@...ux.intel.com>

In most cases each endpoint has only one ring and one list of canceled TDs,
so each TD on a cancelled_td_list is for the same ring.

But with streams in use that might no longer be the case.
This does ensure that the streams work properly.

Greg, any chance to get this into usb-next and 4.8?

-Mathias