linux-kernel - [PATCH 3.2 082/152] USB: avoid left shift by -1

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:   Mon, 14 Nov 2016 00:14:07 +0000
From:   Ben Hutchings <ben@...adent.org.uk>
To:     linux-kernel@...r.kernel.org, stable@...r.kernel.org
CC:     akpm@...ux-foundation.org,
        "Greg Kroah-Hartman" <gregkh@...uxfoundation.org>,
        "Alan Stern" <stern@...land.harvard.edu>,
        "Vittorio Zecca" <zeccav@...il.com>,
        "Bjørn Mork" <bjorn@...k.no>
Subject: [PATCH 3.2 082/152] USB: avoid left shift by -1

3.2.84-rc1 review patch.  If anyone has any objections, please let me know.

------------------

From: Alan Stern <stern@...land.harvard.edu>

commit 53e5f36fbd2453ad69a3369a1db62dc06c30a4aa upstream.

UBSAN complains about a left shift by -1 in proc_do_submiturb().  This
can occur when an URB is submitted for a bulk or control endpoint on
a high-speed device, since the code doesn't bother to check the
endpoint type; normally only interrupt or isochronous endpoints have
a nonzero bInterval value.

Aside from the fact that the operation is illegal, it shouldn't matter
because the result isn't used.  Still, in theory it could cause a
hardware exception or other problem, so we should work around it.
This patch avoids doing the left shift unless the shift amount is >= 0.

The same piece of code has another problem.  When checking the device
speed (the exponential encoding for interrupt endpoints is used only
by high-speed or faster devices), we need to look for speed >=
USB_SPEED_SUPER as well as speed == USB_SPEED HIGH.  The patch adds
this check.

Signed-off-by: Alan Stern <stern@...land.harvard.edu>
Reported-by: Vittorio Zecca <zeccav@...il.com>
Tested-by: Vittorio Zecca <zeccav@...il.com>
Suggested-by: Bjørn Mork <bjorn@...k.no>
Signed-off-by: Greg Kroah-Hartman <gregkh@...uxfoundation.org>
[bwh: Backported to 3.2: adjust context]
Signed-off-by: Ben Hutchings <ben@...adent.org.uk>
---
 drivers/usb/core/devio.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

--- a/drivers/usb/core/devio.c
+++ b/drivers/usb/core/devio.c
@@ -1281,11 +1281,17 @@ static int proc_do_submiturb(struct dev_
 	as->urb->setup_packet = (unsigned char *)dr;
 	as->urb->start_frame = uurb->start_frame;
 	as->urb->number_of_packets = uurb->number_of_packets;
-	if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
-			ps->dev->speed == USB_SPEED_HIGH)
-		as->urb->interval = 1 << min(15, ep->desc.bInterval - 1);
-	else
-		as->urb->interval = ep->desc.bInterval;
+
+	if (ep->desc.bInterval) {
+		if (uurb->type == USBDEVFS_URB_TYPE_ISO ||
+				ps->dev->speed == USB_SPEED_HIGH ||
+				ps->dev->speed >= USB_SPEED_SUPER)
+			as->urb->interval = 1 <<
+					min(15, ep->desc.bInterval - 1);
+		else
+			as->urb->interval = ep->desc.bInterval;
+	}
+
 	as->urb->context = as;
 	as->urb->complete = async_completed;
 	for (totlen = u = 0; u < uurb->number_of_packets; u++) {