Commit 319e1109 authored by Alan Stern's avatar Alan Stern Committed by Lin Yujun
Browse files

USB: core: Fix race by not overwriting udev->descriptor in hub_port_init() 

stable inclusion
from stable-v5.10.195
commit 9d241c5d9a9b7ad95c90c6520272fe404d5ac88f
category: bugfix
bugzilla: https://gitee.com/src-openeuler/kernel/issues/I819LJ
CVE: CVE-2023-37453

Reference: https://git.kernel.org/pub/scm/linux/kernel/git/stable/linux.git/commit/?id=9d241c5d9a9b7ad95c90c6520272fe404d5ac88f



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

commit ff33299e upstream.

Syzbot reported an out-of-bounds read in sysfs.c:read_descriptors():

BUG: KASAN: slab-out-of-bounds in read_descriptors+0x263/0x280 drivers/usb/core/sysfs.c:883
Read of size 8 at addr ffff88801e78b8c8 by task udevd/5011

CPU: 0 PID: 5011 Comm: udevd Not tainted 6.4.0-rc6-syzkaller-00195-g40f71e7cd3c6 #0
Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 05/27/2023
Call Trace:
 <TASK>
 __dump_stack lib/dump_stack.c:88 [inline]
 dump_stack_lvl+0xd9/0x150 lib/dump_stack.c:106
 print_address_description.constprop.0+0x2c/0x3c0 mm/kasan/report.c:351
 print_report mm/kasan/report.c:462 [inline]
 kasan_report+0x11c/0x130 mm/kasan/report.c:572
 read_descriptors+0x263/0x280 drivers/usb/core/sysfs.c:883
...
Allocated by task 758:
...
 __do_kmalloc_node mm/slab_common.c:966 [inline]
 __kmalloc+0x5e/0x190 mm/slab_common.c:979
 kmalloc include/linux/slab.h:563 [inline]
 kzalloc include/linux/slab.h:680 [inline]
 usb_get_configuration+0x1f7/0x5170 drivers/usb/core/config.c:887
 usb_enumerate_device drivers/usb/core/hub.c:2407 [inline]
 usb_new_device+0x12b0/0x19d0 drivers/usb/core/hub.c:2545

As analyzed by Khazhy Kumykov, the cause of this bug is a race between
read_descriptors() and hub_port_init(): The first routine uses a field
in udev->descriptor, not expecting it to change, while the second
overwrites it.

Prior to commit 45bf39f8 ("USB: core: Don't hold device lock while
reading the "descriptors" sysfs file") this race couldn't occur,
because the routines were mutually exclusive thanks to the device
locking.  Removing that locking from read_descriptors() exposed it to
the race.

The best way to fix the bug is to keep hub_port_init() from changing
udev->descriptor once udev has been initialized and registered.
Drivers expect the descriptors stored in the kernel to be immutable;
we should not undermine this expectation.  In fact, this change should
have been made long ago.

So now hub_port_init() will take an additional argument, specifying a
buffer in which to store the device descriptor it reads.  (If udev has
not yet been initialized, the buffer pointer will be NULL and then
hub_port_init() will store the device descriptor in udev as before.)
This eliminates the data race responsible for the out-of-bounds read.

The changes to hub_port_init() appear more extensive than they really
are, because of indentation changes resulting from an attempt to avoid
writing to other parts of the usb_device structure after it has been
initialized.  Similar changes should be made to the code that reads
the BOS descriptor, but that can be handled in a separate patch later
on.  This patch is sufficient to fix the bug found by syzbot.

Reported-and-tested-by: default avatar <syzbot+18996170f8096c6174d0@syzkaller.appspotmail.com>
Closes: https://lore.kernel.org/linux-usb/000000000000c0ffe505fe86c9ca@google.com/#r


Signed-off-by: default avatarAlan Stern <stern@rowland.harvard.edu>
Cc: Khazhy Kumykov <khazhy@google.com>
Fixes: 45bf39f8 ("USB: core: Don't hold device lock while reading the "descriptors" sysfs file")
Cc: stable@vger.kernel.org
Link: https://lore.kernel.org/r/b958b47a-9a46-4c22-a9f9-e42e42c31251@rowland.harvard.edu


Signed-off-by: default avatarGreg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: default avatarLin Yujun <linyujun809@huawei.com>
parent 4e85a930

drivers/usb/core/hub.c

+70 −44
Original line number Diff line number Diff line
@@ -4669,10 +4669,17 @@ static int get_bMaxPacketSize0(struct usb_device *udev, 
 * the port lock.  For a newly detected device that is not accessible

 * through any global pointers, it's not necessary to lock the device,

 * but it is still necessary to lock the port.

 *

 * For a newly detected device, @dev_descr must be NULL.  The device

 * descriptor retrieved from the device will then be stored in

 * @udev->descriptor.  For an already existing device, @dev_descr

 * must be non-NULL.  The device descriptor will be stored there,

 * not in @udev->descriptor, because descriptors for registered

 * devices are meant to be immutable.

 */

static int

hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1,

		int retry_counter)

		int retry_counter, struct usb_device_descriptor *dev_descr)

{

	struct usb_device	*hdev = hub->hdev;

	struct usb_hcd		*hcd = bus_to_hcd(hdev->bus);
@@ -4684,6 +4691,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
	int			devnum = udev->devnum;

	const char		*driver_name;

	bool			do_new_scheme;

	const bool		initial = !dev_descr;

	int			maxp0;

	struct usb_device_descriptor	*buf, *descr;
@@ -4722,6 +4730,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
	}

	oldspeed = udev->speed;



	if (initial) {

		/* USB 2.0 section 5.5.3 talks about ep0 maxpacket ...

		 * it's fixed size except for full speed devices.

		 * For Wireless USB devices, ep0 max packet is always 512 (tho
@@ -4749,6 +4758,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
		default:

			goto fail;

		}

	}



	if (udev->speed == USB_SPEED_WIRELESS)

		speed = "variable speed Wireless";
@@ -4770,9 +4780,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
	if (udev->speed < USB_SPEED_SUPER)

		dev_info(&udev->dev,

				"%s %s USB device number %d using %s\n",

				(udev->config) ? "reset" : "new", speed,

				(initial ? "new" : "reset"), speed,

				devnum, driver_name);



	if (initial) {

		/* Set up TT records, if needed  */

		if (hdev->tt) {

			udev->tt = hdev->tt;
@@ -4787,6 +4798,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
			udev->tt = &hub->tt;

			udev->ttport = port1;

		}

	}



	/* Why interleave GET_DESCRIPTOR and SET_ADDRESS this way?

	 * Because device hardware and firmware is sometimes buggy in
@@ -4814,6 +4826,12 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 


			maxp0 = get_bMaxPacketSize0(udev, buf,

					GET_DESCRIPTOR_BUFSIZE, retries == 0);

			if (maxp0 > 0 && !initial &&

					maxp0 != udev->descriptor.bMaxPacketSize0) {

				dev_err(&udev->dev, "device reset changed ep0 maxpacket size!\n");

				retval = -ENODEV;

				goto fail;

			}



			retval = hub_port_reset(hub, port1, udev, delay, false);

			if (retval < 0)		/* error or disconnect */
@@ -4883,6 +4901,12 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
		} else {

			u32 delay;



			if (!initial && maxp0 != udev->descriptor.bMaxPacketSize0) {

				dev_err(&udev->dev, "device reset changed ep0 maxpacket size!\n");

				retval = -ENODEV;

				goto fail;

			}



			delay = udev->parent->hub_delay;

			udev->hub_delay = min_t(u32, delay,

						USB_TP_TRANSMISSION_DELAY_MAX);
@@ -4926,7 +4950,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, 
					retval);

		goto fail;

	}

	if (initial)

		udev->descriptor = *descr;

	else

		*dev_descr = *descr;

	kfree(descr);



	/*
@@ -5229,7 +5256,7 @@ static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, 
		}



		/* reset (non-USB 3.0 devices) and get descriptor */

		status = hub_port_init(hub, udev, port1, i);

		status = hub_port_init(hub, udev, port1, i, NULL);

		if (status < 0)

			goto loop;
@@ -5845,7 +5872,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev) 
	struct usb_device		*parent_hdev = udev->parent;

	struct usb_hub			*parent_hub;

	struct usb_hcd			*hcd = bus_to_hcd(udev->bus);

	struct usb_device_descriptor	descriptor = udev->descriptor;

	struct usb_device_descriptor	descriptor;

	struct usb_host_bos		*bos;

	int				i, j, ret = 0;

	int				port1 = udev->portnum;
@@ -5887,7 +5914,7 @@ static int usb_reset_and_verify_device(struct usb_device *udev) 
		/* ep0 maxpacket size may change; let the HCD know about it.

		 * Other endpoints will be handled by re-enumeration. */

		usb_ep0_reinit(udev);

		ret = hub_port_init(parent_hub, udev, port1, i);

		ret = hub_port_init(parent_hub, udev, port1, i, &descriptor);

		if (ret >= 0 || ret == -ENOTCONN || ret == -ENODEV)

			break;

	}
@@ -5899,7 +5926,6 @@ static int usb_reset_and_verify_device(struct usb_device *udev) 
	/* Device might have changed firmware (DFU or similar) */

	if (descriptors_changed(udev, &descriptor, bos)) {

		dev_info(&udev->dev, "device firmware changed\n");

		udev->descriptor = descriptor;	/* for disconnect() calls */

		goto re_enumerate;

	}