UBSAN: shift-out-of-bounds in option_probe

UBSAN: shift-out-of-bounds in option_probe

[syzbot]

Hello,

syzbot found the following issue on:

HEAD commit:    15ac8fdb Add linux-next specific files for 20201207
git tree:       linux-next
console output: https://syzkaller.appspot.com/x/log.txt?x=17dc6adf500000
kernel config:  https://syzkaller.appspot.com/x/.config?x=3696b8138207d24d
dashboard link: https://syzkaller.appspot.com/bug?extid=8881b478dad0a7971f79
compiler:       gcc (GCC) 10.1.0-syz 20200507
syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12e89613500000
C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17993623500000

IMPORTANT: if you fix the issue, please add the following tag to the commit:
Reported-by: syzbot+8881b478dad0a7971f79@syzkaller.appspotmail.com

usb 1-1: config 0 interface 109 has no altsetting 0
usb 1-1: New USB device found, idVendor=12d1, idProduct=02cb, bcdDevice= 1.fb
usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
usb 1-1: config 0 descriptor??
================================================================================
UBSAN: shift-out-of-bounds in drivers/usb/serial/option.c:2120:21
shift exponent 109 is too large for 64-bit type 'long unsigned int'
CPU: 0 PID: 3169 Comm: kworker/0:3 Not tainted 5.10.0-rc6-next-20201207-syzkaller #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
Workqueue: usb_hub_wq hub_event
Call Trace:
 __dump_stack lib/dump_stack.c:79 [inline]
 dump_stack+0x107/0x163 lib/dump_stack.c:120
 ubsan_epilogue+0xb/0x5a lib/ubsan.c:148
 __ubsan_handle_shift_out_of_bounds.cold+0xb1/0x181 lib/ubsan.c:395
 option_probe.cold+0x1a/0x1f drivers/usb/serial/option.c:2120
 usb_serial_probe+0x32d/0xef0 drivers/usb/serial/usb-serial.c:905
 usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396
 really_probe+0x2b1/0xe40 drivers/base/dd.c:554
 driver_probe_device+0x285/0x3f0 drivers/base/dd.c:738
 __device_attach_driver+0x216/0x2d0 drivers/base/dd.c:844


---
This report is generated by a bot. It may contain errors.
See https://goo.gl/tpsmEJ for more information about syzbot.
syzbot engineers can be reached at syzkaller@googlegroups.com.

syzbot will keep track of this issue. See:
https://goo.gl/tpsmEJ#status for how to communicate with syzbot.
syzbot can test patches for this issue, for details see:
https://goo.gl/tpsmEJ#testing-patches

Re: UBSAN: shift-out-of-bounds in option_probe

[Johan Hovold]

On Tue, Dec 08, 2020 at 11:33:11PM -0800, syzbot wrote:
> Hello,
> 
> syzbot found the following issue on:
> 
> HEAD commit:    15ac8fdb Add linux-next specific files for 20201207
> git tree:       linux-next
> console output: https://syzkaller.appspot.com/x/log.txt?x=17dc6adf500000
> kernel config:  https://syzkaller.appspot.com/x/.config?x=3696b8138207d24d
> dashboard link: https://syzkaller.appspot.com/bug?extid=8881b478dad0a7971f79
> compiler:       gcc (GCC) 10.1.0-syz 20200507
> syz repro:      https://syzkaller.appspot.com/x/repro.syz?x=12e89613500000
> C reproducer:   https://syzkaller.appspot.com/x/repro.c?x=17993623500000
> 
> IMPORTANT: if you fix the issue, please add the following tag to the commit:
> Reported-by: syzbot+8881b478dad0a7971f79@syzkaller.appspotmail.com
> 
> usb 1-1: config 0 interface 109 has no altsetting 0
> usb 1-1: New USB device found, idVendor=12d1, idProduct=02cb, bcdDevice= 1.fb
> usb 1-1: New USB device strings: Mfr=0, Product=0, SerialNumber=0
> usb 1-1: config 0 descriptor??
> ================================================================================
> UBSAN: shift-out-of-bounds in drivers/usb/serial/option.c:2120:21
> shift exponent 109 is too large for 64-bit type 'long unsigned int'

Ok, we need to add a sanity check against large interface numbers before
the device-flag tests. I'll cook something up.

Johan

[PATCH] USB: serial: option: add interface-number sanity check to flag handling

[Johan Hovold]

Add an interface-number sanity check before testing the device flags to
avoid relying on undefined behaviour when left shifting in case a device
uses an interface number greater than or equal to BITS_PER_LONG (i.e. 64
or 32).

Reported-by: syzbot+8881b478dad0a7971f79@syzkaller.appspotmail.com
Fixes: c3a65808f04a ("USB: serial: option: reimplement interface masking")
Cc: stable@vger.kernel.org
Signed-off-by: Johan Hovold <johan@kernel.org>
---
 drivers/usb/serial/option.c | 23 +++++++++++++++++++++--
 1 file changed, 21 insertions(+), 2 deletions(-)

diff --git a/drivers/usb/serial/option.c b/drivers/usb/serial/option.c
index 2a3bfd6f867e..c5908c4f2046 100644
--- a/drivers/usb/serial/option.c
+++ b/drivers/usb/serial/option.c
@@ -561,6 +561,9 @@ static void option_instat_callback(struct urb *urb);
 
 /* Device flags */
 
+/* Highest interface number which can be used with NCTRL() and RSVD() */
+#define FLAG_IFNUM_MAX	7
+
 /* Interface does not support modem-control requests */
 #define NCTRL(ifnum)	((BIT(ifnum) & 0xff) << 8)
 
@@ -2089,6 +2092,14 @@ static struct usb_serial_driver * const serial_drivers[] = {
 
 module_usb_serial_driver(serial_drivers, option_ids);
 
+static bool iface_is_reserved(unsigned long device_flags, u8 ifnum)
+{
+	if (ifnum > FLAG_IFNUM_MAX)
+		return false;
+
+	return device_flags & RSVD(ifnum);
+}
+
 static int option_probe(struct usb_serial *serial,
 			const struct usb_device_id *id)
 {
@@ -2105,7 +2116,7 @@ static int option_probe(struct usb_serial *serial,
 	 * the same class/subclass/protocol as the serial interfaces.  Look at
 	 * the Windows driver .INF files for reserved interface numbers.
 	 */
-	if (device_flags & RSVD(iface_desc->bInterfaceNumber))
+	if (iface_is_reserved(device_flags, iface_desc->bInterfaceNumber))
 		return -ENODEV;
 
 	/*
@@ -2121,6 +2132,14 @@ static int option_probe(struct usb_serial *serial,
 	return 0;
 }
 
+static bool iface_no_modem_control(unsigned long device_flags, u8 ifnum)
+{
+	if (ifnum > FLAG_IFNUM_MAX)
+		return false;
+
+	return device_flags & NCTRL(ifnum);
+}
+
 static int option_attach(struct usb_serial *serial)
 {
 	struct usb_interface_descriptor *iface_desc;
@@ -2136,7 +2155,7 @@ static int option_attach(struct usb_serial *serial)
 
 	iface_desc = &serial->interface->cur_altsetting->desc;
 
-	if (!(device_flags & NCTRL(iface_desc->bInterfaceNumber)))
+	if (!iface_no_modem_control(device_flags, iface_desc->bInterfaceNumber))
 		data->use_send_setup = 1;
 
 	if (device_flags & ZLP)
-- 
2.26.2

Re: [PATCH] USB: serial: option: add interface-number sanity check to flag handling

[Greg KH]

On Wed, Dec 09, 2020 at 11:42:21AM +0100, Johan Hovold wrote:
> Add an interface-number sanity check before testing the device flags to
> avoid relying on undefined behaviour when left shifting in case a device
> uses an interface number greater than or equal to BITS_PER_LONG (i.e. 64
> or 32).
> 
> Reported-by: syzbot+8881b478dad0a7971f79@syzkaller.appspotmail.com
> Fixes: c3a65808f04a ("USB: serial: option: reimplement interface masking")
> Cc: stable@vger.kernel.org
> Signed-off-by: Johan Hovold <johan@kernel.org>

Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH] USB: serial: option: add interface-number sanity check to flag handling

[Johan Hovold]

On Wed, Dec 09, 2020 at 11:52:45AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Dec 09, 2020 at 11:42:21AM +0100, Johan Hovold wrote:
> > Add an interface-number sanity check before testing the device flags to
> > avoid relying on undefined behaviour when left shifting in case a device
> > uses an interface number greater than or equal to BITS_PER_LONG (i.e. 64
> > or 32).
> > 
> > Reported-by: syzbot+8881b478dad0a7971f79@syzkaller.appspotmail.com
> > Fixes: c3a65808f04a ("USB: serial: option: reimplement interface masking")
> > Cc: stable@vger.kernel.org
> > Signed-off-by: Johan Hovold <johan@kernel.org>
> 
> Reviewed-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Thanks for reviewing. Now applied.

Johan