From patchwork Wed Oct 11 22:54:06 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yuxuan Luo X-Patchwork-Id: 1847052 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4S5Sl45yd4z23jX for ; Thu, 12 Oct 2023 09:54:40 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1qqi62-0001VC-JH; Wed, 11 Oct 2023 22:54:33 +0000 Received: from smtp-relay-internal-0.internal ([10.131.114.225] helo=smtp-relay-internal-0.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1qqi5u-0001Td-UN for kernel-team@lists.ubuntu.com; Wed, 11 Oct 2023 22:54:23 +0000 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 7C83F3F231 for ; Wed, 11 Oct 2023 22:54:22 +0000 (UTC) Received: by mail-qt1-f200.google.com with SMTP id d75a77b69052e-418134c43d7so4667531cf.1 for ; Wed, 11 Oct 2023 15:54:22 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697064860; x=1697669660; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=zS8ESSaxAR05W7JxSpA/cNRqtjO9SwhWeE02OEFpUto=; b=iEeDTZBQlrSbi1ES9v4VY3zvVwIvrCVNhwWPqYvzn+ZTY8hRPr0PfGqAgB+2ps3f0G 2wo7i5JtAXtRoMEdYBSGVC2eu2w3Ow0YvATU9c9vos7kIxMHqSb1sCq+86ogoaP1ivC5 7HWwLziuP3xMa4yZxHR2dO8TGduyO/ek7u2nrb1NFDRDf2/huqERFFm7/urj9led06jK JswH/L6IreuGpmhdW+hMWAQuS3RU7jvF/9mHW0mBzB6ujwJCc53UB5i+ocNDNKS99low V0x+Hdnfv+ukpOzMM6lBK80zaGvIvxdMaymHp93sLeieHHAuci4idZYu2frhbE0NCwmp U9fA== X-Gm-Message-State: AOJu0YywE1AtSnj+PQPe2BousviQnvjHTilMZEKXnKscApbjs+v4Xw0S d7UPGTOYS0a/Q4uuUSroXMFNDXrWlmFU5G5e9YMNNKEcUrU3Pb3nNs2yzP6IZ75eZ+q3dpxO3dE lxnAfigzrMhVoX996q87e9abnzJabwKgetuW44pYEla3WKOHNnQ== X-Received: by 2002:ac8:58ce:0:b0:400:9f2c:1211 with SMTP id u14-20020ac858ce000000b004009f2c1211mr30086293qta.29.1697064860274; Wed, 11 Oct 2023 15:54:20 -0700 (PDT) X-Google-Smtp-Source: AGHT+IFQ5BmARSxONv7g9MZVwSbqwMTmqe55YfSZo8WpkVZsXQ88qaVTzTZXA2h4GRKTsubCU0pm2w== X-Received: by 2002:ac8:58ce:0:b0:400:9f2c:1211 with SMTP id u14-20020ac858ce000000b004009f2c1211mr30086266qta.29.1697064859784; Wed, 11 Oct 2023 15:54:19 -0700 (PDT) Received: from cache-ubuntu.hsd1.nj.comcast.net ([2601:86:200:98b0:5308:de8d:b54e:a80e]) by smtp.gmail.com with ESMTPSA id y23-20020ac87097000000b00417db2593bdsm5691098qto.72.2023.10.11.15.54.18 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 15:54:19 -0700 (PDT) From: Yuxuan Luo To: kernel-team@lists.ubuntu.com Subject: [SRU][J/L][PATCH 1/3] USB: core: Unite old scheme and new scheme descriptor reads Date: Wed, 11 Oct 2023 18:54:06 -0400 Message-Id: <20231011225414.50811-2-yuxuan.luo@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231011225414.50811-1-yuxuan.luo@canonical.com> References: <20231011225414.50811-1-yuxuan.luo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Alan Stern In preparation for reworking the usb_get_device_descriptor() routine, it is desirable to unite the two different code paths responsible for initially determining endpoint 0's maximum packet size in a newly discovered USB device. Making this determination presents a chicken-and-egg sort of problem, in that the only way to learn the maxpacket value is to get it from the device descriptor retrieved from the device, but communicating with the device to retrieve a descriptor requires us to know beforehand the ep0 maxpacket size. In practice this problem is solved in two different ways, referred to in hub.c as the "old scheme" and the "new scheme". The old scheme (which is the approach recommended by the USB-2 spec) involves asking the device to send just the first eight bytes of its device descriptor. Such a transfer uses packets containing no more than eight bytes each, and every USB device must have an ep0 maxpacket size >= 8, so this should succeed. Since the bMaxPacketSize0 field of the device descriptor lies within the first eight bytes, this is all we need. The new scheme is an imitation of the technique used in an early Windows USB implementation, giving it the happy advantage of working with a wide variety of devices (some of them at the time would not work with the old scheme, although that's probably less true now). It involves making an initial guess of the ep0 maxpacket size, asking the device to send up to 64 bytes worth of its device descriptor (which is only 18 bytes long), and then resetting the device to clear any error condition that might have resulted from the guess being wrong. The initial guess is determined by the connection speed; it should be correct in all cases other than full speed, for which the allowed values are 8, 16, 32, and 64 (in this case the initial guess is 64). The reason for this patch is that the old- and new-scheme parts of hub_port_init() use different code paths, one involving usb_get_device_descriptor() and one not, for their initial reads of the device descriptor. Since these reads have essentially the same purpose and are made under essentially the same circumstances, this is illogical. It makes more sense to have both of them use a common subroutine. This subroutine does basically what the new scheme's code did, because that approach is more general than the one used by the old scheme. It only needs to know how many bytes to transfer and whether or not it is being called for the first iteration of a retry loop (in case of certain time-out errors). There are two main differences from the former code: We initialize the bDescriptorType field of the transfer buffer to 0 before performing the transfer, to avoid possibly accessing an uninitialized value afterward. We read the device descriptor into a temporary buffer rather than storing it directly into udev->descriptor, which the old scheme implementation used to do. Since the whole point of this first read of the device descriptor is to determine the bMaxPacketSize0 value, that is what the new routine returns (or an error code). The value is stored in a local variable rather than in udev->descriptor. As a side effect, this necessitates moving a section of code that checks the bcdUSB field for SuperSpeed devices until after the full device descriptor has been retrieved. Signed-off-by: Alan Stern Cc: Oliver Neukum Link: https://lore.kernel.org/r/495cb5d4-f956-4f4a-a875-1e67e9489510@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman (cherry picked from commit 85d07c55621676d47d873d2749b88f783cd4d5a1) CVE-2023-37453 Signed-off-by: Yuxuan Luo --- drivers/usb/core/hub.c | 173 ++++++++++++++++++++++------------------- 1 file changed, 94 insertions(+), 79 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 97a0f8faea6e5..cef98758f597e 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4703,6 +4703,67 @@ static int hub_enable_device(struct usb_device *udev) return hcd->driver->enable_device(hcd, udev); } +/* + * Get the bMaxPacketSize0 value during initialization by reading the + * device's device descriptor. Since we don't already know this value, + * the transfer is unsafe and it ignores I/O errors, only testing for + * reasonable received values. + * + * For "old scheme" initialization, size will be 8 so we read just the + * start of the device descriptor, which should work okay regardless of + * the actual bMaxPacketSize0 value. For "new scheme" initialization, + * size will be 64 (and buf will point to a sufficiently large buffer), + * which might not be kosher according to the USB spec but it's what + * Windows does and what many devices expect. + * + * Returns: bMaxPacketSize0 or a negative error code. + */ +static int get_bMaxPacketSize0(struct usb_device *udev, + struct usb_device_descriptor *buf, int size, bool first_time) +{ + int i, rc; + + /* + * Retry on all errors; some devices are flakey. + * 255 is for WUSB devices, we actually need to use + * 512 (WUSB1.0[4.8.1]). + */ + for (i = 0; i < GET_MAXPACKET0_TRIES; ++i) { + /* Start with invalid values in case the transfer fails */ + buf->bDescriptorType = buf->bMaxPacketSize0 = 0; + rc = usb_control_msg(udev, usb_rcvaddr0pipe(), + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, + USB_DT_DEVICE << 8, 0, + buf, size, + initial_descriptor_timeout); + switch (buf->bMaxPacketSize0) { + case 8: case 16: case 32: case 64: case 255: + if (buf->bDescriptorType == USB_DT_DEVICE) { + rc = buf->bMaxPacketSize0; + break; + } + fallthrough; + default: + if (rc >= 0) + rc = -EPROTO; + break; + } + + /* + * Some devices time out if they are powered on + * when already connected. They need a second + * reset, so return early. But only on the first + * attempt, lest we get into a time-out/reset loop. + */ + if (rc > 0 || (rc == -ETIMEDOUT && first_time && + udev->speed > USB_SPEED_FULL)) + break; + } + return rc; +} + +#define GET_DESCRIPTOR_BUFSIZE 64 + /* Reset device, (re)assign address, get device descriptor. * Device connection must be stable, no more debouncing needed. * Returns device in USB_STATE_ADDRESS, except on error. @@ -4727,6 +4788,12 @@ 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; + int maxp0; + struct usb_device_descriptor *buf; + + buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); + if (!buf) + return -ENOMEM; /* root hub ports have a slightly longer reset period * (from USB 2.0 spec, section 7.1.7.5) @@ -4846,9 +4913,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, } if (do_new_scheme) { - struct usb_device_descriptor *buf; - int r = 0; - retval = hub_enable_device(udev); if (retval < 0) { dev_err(&udev->dev, @@ -4857,52 +4921,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, goto fail; } -#define GET_DESCRIPTOR_BUFSIZE 64 - buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); - if (!buf) { - retval = -ENOMEM; - continue; - } - - /* Retry on all errors; some devices are flakey. - * 255 is for WUSB devices, we actually need to use - * 512 (WUSB1.0[4.8.1]). - */ - for (operations = 0; operations < GET_MAXPACKET0_TRIES; - ++operations) { - buf->bMaxPacketSize0 = 0; - r = usb_control_msg(udev, usb_rcvaddr0pipe(), - USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, - USB_DT_DEVICE << 8, 0, - buf, GET_DESCRIPTOR_BUFSIZE, - initial_descriptor_timeout); - switch (buf->bMaxPacketSize0) { - case 8: case 16: case 32: case 64: case 255: - if (buf->bDescriptorType == - USB_DT_DEVICE) { - r = 0; - break; - } - fallthrough; - default: - if (r == 0) - r = -EPROTO; - break; - } - /* - * Some devices time out if they are powered on - * when already connected. They need a second - * reset. But only on the first attempt, - * lest we get into a time out/reset loop - */ - if (r == 0 || (r == -ETIMEDOUT && - retries == 0 && - udev->speed > USB_SPEED_FULL)) - break; - } - udev->descriptor.bMaxPacketSize0 = - buf->bMaxPacketSize0; - kfree(buf); + maxp0 = get_bMaxPacketSize0(udev, buf, + GET_DESCRIPTOR_BUFSIZE, retries == 0); retval = hub_port_reset(hub, port1, udev, delay, false); if (retval < 0) /* error or disconnect */ @@ -4913,14 +4933,13 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, retval = -ENODEV; goto fail; } - if (r) { - if (r != -ENODEV) + if (maxp0 < 0) { + if (maxp0 != -ENODEV) dev_err(&udev->dev, "device descriptor read/64, error %d\n", - r); - retval = -EMSGSIZE; + maxp0); + retval = maxp0; continue; } -#undef GET_DESCRIPTOR_BUFSIZE } /* @@ -4966,19 +4985,17 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, break; } - retval = usb_get_device_descriptor(udev, 8); - if (retval < 8) { + /* !do_new_scheme || wusb */ + maxp0 = get_bMaxPacketSize0(udev, buf, 8, retries == 0); + if (maxp0 < 0) { + retval = maxp0; if (retval != -ENODEV) dev_err(&udev->dev, "device descriptor read/8, error %d\n", retval); - if (retval >= 0) - retval = -EMSGSIZE; } else { u32 delay; - retval = 0; - delay = udev->parent->hub_delay; udev->hub_delay = min_t(u32, delay, USB_TP_TRANSMISSION_DELAY_MAX); @@ -4995,27 +5012,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, if (retval) goto fail; - /* - * Some superspeed devices have finished the link training process - * and attached to a superspeed hub port, but the device descriptor - * got from those devices show they aren't superspeed devices. Warm - * reset the port attached by the devices can fix them. - */ - if ((udev->speed >= USB_SPEED_SUPER) && - (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) { - dev_err(&udev->dev, "got a wrong device descriptor, " - "warm reset device\n"); - hub_port_reset(hub, port1, udev, - HUB_BH_RESET_TIME, true); - retval = -EINVAL; - goto fail; - } - - if (udev->descriptor.bMaxPacketSize0 == 0xff || - udev->speed >= USB_SPEED_SUPER) + if (maxp0 == 0xff || udev->speed >= USB_SPEED_SUPER) i = 512; else - i = udev->descriptor.bMaxPacketSize0; + i = maxp0; if (usb_endpoint_maxp(&udev->ep0.desc) != i) { if (udev->speed == USB_SPEED_LOW || !(i == 8 || i == 16 || i == 32 || i == 64)) { @@ -5041,6 +5041,20 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, goto fail; } + /* + * Some superspeed devices have finished the link training process + * and attached to a superspeed hub port, but the device descriptor + * got from those devices show they aren't superspeed devices. Warm + * reset the port attached by the devices can fix them. + */ + if ((udev->speed >= USB_SPEED_SUPER) && + (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) { + dev_err(&udev->dev, "got a wrong device descriptor, warm reset device\n"); + hub_port_reset(hub, port1, udev, HUB_BH_RESET_TIME, true); + retval = -EINVAL; + goto fail; + } + usb_detect_quirks(udev); if (udev->wusb == 0 && le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0201) { @@ -5063,6 +5077,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, hub_port_disable(hub, port1, 0); update_devnum(udev, devnum); /* for disconnect processing */ } + kfree(buf); return retval; } From patchwork Wed Oct 11 22:54:09 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: Yuxuan Luo X-Patchwork-Id: 1847053 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4S5SlD03flz1yqj for ; Thu, 12 Oct 2023 09:54:47 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1qqi6A-0001Xc-A3; Wed, 11 Oct 2023 22:54:38 +0000 Received: from smtp-relay-internal-0.internal ([10.131.114.225] helo=smtp-relay-internal-0.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1qqi5w-0001Tm-BV for kernel-team@lists.ubuntu.com; Wed, 11 Oct 2023 22:54:25 +0000 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 1B1E23F231 for ; Wed, 11 Oct 2023 22:54:24 +0000 (UTC) Received: by mail-qt1-f200.google.com with SMTP id d75a77b69052e-419754aaa41so3299211cf.2 for ; Wed, 11 Oct 2023 15:54:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697064863; x=1697669663; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MnesND/aWJ3NFOuWZJBnIuwMlONlFs6q04qUZ1MDODk=; b=jIaIVlWcVnoMpDg9vn8zyuPJ8FH74bW0oRvh4XWxzToHhsb2ZzhF0SKCdkK22TfZW+ 6l1ppZZQ8SQ7aNTFaKRzk5g5djl0udqUWnWUzXyjJ0K9zbvhaqINPc/RQyasFmcGP3jW iidsBz/05BVjLqq71XjxjUUmhpuNL5yRHiPPZsX9mcaVeWmiXayj2QvthHbE2T/rgiEU oEanGTkwczNF1z74JYMaq+XxMN96fqwr2qMc8Tb64yxhfOM9DrpCTRAa1DAJYbg4ZoW+ wcacTL2aF+96+8VWmcYFFb/iZTv0+p4Ks6pnxTYU7hAHhS0kC1BbaVnokY+NCJH15pbk eKUQ== X-Gm-Message-State: AOJu0YzNWg1vArT5fnHeWhlUpv9yLQZbiv/Y5IlhKHn0XZVGR41K2KA6 853faYGItT1gDeO6s9xo3+Mj7+0NDrdFBs658AROThk7HDFdjeDwKeSwzyMllAAiNpbZPx15gui CBoZYEJ+8rtQj5vgGkbS+tfTKwH4G6cxll9iJDU31UXMHsuUp4Q== X-Received: by 2002:a05:622a:1009:b0:415:138e:d858 with SMTP id d9-20020a05622a100900b00415138ed858mr22206861qte.60.1697064862804; Wed, 11 Oct 2023 15:54:22 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEpNyoxRDwFtjbs9cxqrA/ZP9btU8pkHXVYciw8AbvMv4LT6FVQMDpIC/gjoohmp29DzekYYA== X-Received: by 2002:a05:622a:1009:b0:415:138e:d858 with SMTP id d9-20020a05622a100900b00415138ed858mr22206850qte.60.1697064862527; Wed, 11 Oct 2023 15:54:22 -0700 (PDT) Received: from cache-ubuntu.hsd1.nj.comcast.net ([2601:86:200:98b0:5308:de8d:b54e:a80e]) by smtp.gmail.com with ESMTPSA id y23-20020ac87097000000b00417db2593bdsm5691098qto.72.2023.10.11.15.54.21 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 15:54:21 -0700 (PDT) From: Yuxuan Luo To: kernel-team@lists.ubuntu.com Subject: [SRU][Focal][PATCH 2/6] USB: hub: Add Kconfig option to reduce number of port initialization retries Date: Wed, 11 Oct 2023 18:54:09 -0400 Message-Id: <20231011225414.50811-5-yuxuan.luo@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231011225414.50811-1-yuxuan.luo@canonical.com> References: <20231011225414.50811-1-yuxuan.luo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Alan Stern Description based on one by Yasushi Asano: According to 6.7.22 A-UUT “Device No Response” for connection timeout of USB OTG and EH automated compliance plan v1.2, enumeration failure has to be detected within 30 seconds. However, the old and new enumeration schemes each make a total of 12 attempts, and each attempt can take 5 seconds to time out, so the PET test fails. This patch adds a new Kconfig option (CONFIG_USB_FEW_INIT_RETRIES); when the option is set all the initialization retry loops except the outermost are reduced to a single iteration. This reduces the total number of attempts to four, allowing Linux hosts to pass the PET test. The new option is disabled by default to preserve the existing behavior. The reduced number of retries may fail to initialize a few devices that currently do work, but for the most part there should be no change. And in cases where the initialization does fail, it will fail much more quickly. Reported-and-tested-by: yasushi asano Signed-off-by: Alan Stern Link: https://lore.kernel.org/r/20200928152217.GB134701@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman (backported from commit fb6f076d543414cec709401ec65e5f8e6985d77a) [yuxuan.luo: excludes the Kconfig modification] CVE-2023-37453 Signed-off-by: Yuxuan Luo --- drivers/usb/core/hub.c | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 3462d59ba53cb..228b882bc56b4 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2715,10 +2715,20 @@ static unsigned hub_is_wusb(struct usb_hub *hub) } +#ifdef CONFIG_USB_FEW_INIT_RETRIES +#define PORT_RESET_TRIES 2 +#define SET_ADDRESS_TRIES 1 +#define GET_DESCRIPTOR_TRIES 1 +#define GET_MAXPACKET0_TRIES 1 +#define PORT_INIT_TRIES 4 + +#else #define PORT_RESET_TRIES 5 #define SET_ADDRESS_TRIES 2 #define GET_DESCRIPTOR_TRIES 2 +#define GET_MAXPACKET0_TRIES 3 #define PORT_INIT_TRIES 4 +#endif /* CONFIG_USB_FEW_INIT_RETRIES */ #define HUB_ROOT_RESET_TIME 60 /* times are in msec */ #define HUB_SHORT_RESET_TIME 10 @@ -4801,7 +4811,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, * 255 is for WUSB devices, we actually need to use * 512 (WUSB1.0[4.8.1]). */ - for (operations = 0; operations < 3; ++operations) { + for (operations = 0; operations < GET_MAXPACKET0_TRIES; + ++operations) { buf->bMaxPacketSize0 = 0; r = usb_control_msg(udev, usb_rcvaddr0pipe(), USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, From patchwork Wed Oct 11 22:54:11 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yuxuan Luo X-Patchwork-Id: 1847056 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4S5Slw0qmzz1ypX for ; Thu, 12 Oct 2023 09:55:24 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1qqi6d-0001od-Ed; Wed, 11 Oct 2023 22:55:07 +0000 Received: from smtp-relay-internal-0.internal ([10.131.114.225] helo=smtp-relay-internal-0.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1qqi5z-0001U5-Vy for kernel-team@lists.ubuntu.com; Wed, 11 Oct 2023 22:54:28 +0000 Received: from mail-oa1-f69.google.com (mail-oa1-f69.google.com [209.85.160.69]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 0BB7B3F231 for ; Wed, 11 Oct 2023 22:54:27 +0000 (UTC) Received: by mail-oa1-f69.google.com with SMTP id 586e51a60fabf-1e96efd9ae0so425185fac.3 for ; Wed, 11 Oct 2023 15:54:26 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697064865; x=1697669665; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=vrOHp7edoIQNjqB/4hkB8IEu6LTr2a+94Y6ZcYK9CL4=; b=YuJUvsAXunFmHYO7UE8tX1iHckHXMJZBbNRs2fQVGN5/YV3HStrz+202YL9Wbz0utN amW8BKYSOsJvSXrkNbM7aH1hT9mDeW9Fl04TqgUW2aR/AgoCQdKDWddOMPzN4WTx5eto o+uW7oS6VUehMjeBT7zhKKHVc8hyYvZ6AtEO0P8qNZm9Pss9dE1or3Er8aEM685+L49p /JGbjmF+35Tt6+SQ0TUxOPuwUa2Sdh/AXZoNYLVrzfxFuFThC3Y40IFt8pbzbmtm0s3P lJxhJmKwU5bgi+KMVLh59t1WTu94vDY+rdClRpTR6/3E1NWxC2y+AWDn4fXKYvkXK4Mk fj5g== X-Gm-Message-State: AOJu0YyT+jA8MOQA6pY3owxgSkA2u2QkBNZyPRjKp3w8Hh5tX5T2RwVX DwctyO84PWXsu0JC/vryNDhvWkd/YNSwbkUKje/d8ffAYJeiVIfMo9h6I9H5WvmrsPQGVPTK0PL 7i3SCSfsEmCSTPtx+//SPZQO0dPA38kBDkJtPTFiLJ7gKw6ufyQ== X-Received: by 2002:a05:6871:8a02:b0:1e9:896a:8055 with SMTP id tl2-20020a0568718a0200b001e9896a8055mr3747694oab.25.1697064865387; Wed, 11 Oct 2023 15:54:25 -0700 (PDT) X-Google-Smtp-Source: AGHT+IGKZNTUGYalIcLTkdEIXAtik/JGIwTiNMseX6yjb+j8iBXaVwU2jiauuZg75sOSihxBKqiI5w== X-Received: by 2002:a05:6871:8a02:b0:1e9:896a:8055 with SMTP id tl2-20020a0568718a0200b001e9896a8055mr3747680oab.25.1697064864909; Wed, 11 Oct 2023 15:54:24 -0700 (PDT) Received: from cache-ubuntu.hsd1.nj.comcast.net ([2601:86:200:98b0:5308:de8d:b54e:a80e]) by smtp.gmail.com with ESMTPSA id y23-20020ac87097000000b00417db2593bdsm5691098qto.72.2023.10.11.15.54.24 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 15:54:24 -0700 (PDT) From: Yuxuan Luo To: kernel-team@lists.ubuntu.com Subject: [SRU][Focal][PATCH 3/6] USB: core: Unite old scheme and new scheme descriptor reads Date: Wed, 11 Oct 2023 18:54:11 -0400 Message-Id: <20231011225414.50811-7-yuxuan.luo@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231011225414.50811-1-yuxuan.luo@canonical.com> References: <20231011225414.50811-1-yuxuan.luo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Alan Stern In preparation for reworking the usb_get_device_descriptor() routine, it is desirable to unite the two different code paths responsible for initially determining endpoint 0's maximum packet size in a newly discovered USB device. Making this determination presents a chicken-and-egg sort of problem, in that the only way to learn the maxpacket value is to get it from the device descriptor retrieved from the device, but communicating with the device to retrieve a descriptor requires us to know beforehand the ep0 maxpacket size. In practice this problem is solved in two different ways, referred to in hub.c as the "old scheme" and the "new scheme". The old scheme (which is the approach recommended by the USB-2 spec) involves asking the device to send just the first eight bytes of its device descriptor. Such a transfer uses packets containing no more than eight bytes each, and every USB device must have an ep0 maxpacket size >= 8, so this should succeed. Since the bMaxPacketSize0 field of the device descriptor lies within the first eight bytes, this is all we need. The new scheme is an imitation of the technique used in an early Windows USB implementation, giving it the happy advantage of working with a wide variety of devices (some of them at the time would not work with the old scheme, although that's probably less true now). It involves making an initial guess of the ep0 maxpacket size, asking the device to send up to 64 bytes worth of its device descriptor (which is only 18 bytes long), and then resetting the device to clear any error condition that might have resulted from the guess being wrong. The initial guess is determined by the connection speed; it should be correct in all cases other than full speed, for which the allowed values are 8, 16, 32, and 64 (in this case the initial guess is 64). The reason for this patch is that the old- and new-scheme parts of hub_port_init() use different code paths, one involving usb_get_device_descriptor() and one not, for their initial reads of the device descriptor. Since these reads have essentially the same purpose and are made under essentially the same circumstances, this is illogical. It makes more sense to have both of them use a common subroutine. This subroutine does basically what the new scheme's code did, because that approach is more general than the one used by the old scheme. It only needs to know how many bytes to transfer and whether or not it is being called for the first iteration of a retry loop (in case of certain time-out errors). There are two main differences from the former code: We initialize the bDescriptorType field of the transfer buffer to 0 before performing the transfer, to avoid possibly accessing an uninitialized value afterward. We read the device descriptor into a temporary buffer rather than storing it directly into udev->descriptor, which the old scheme implementation used to do. Since the whole point of this first read of the device descriptor is to determine the bMaxPacketSize0 value, that is what the new routine returns (or an error code). The value is stored in a local variable rather than in udev->descriptor. As a side effect, this necessitates moving a section of code that checks the bcdUSB field for SuperSpeed devices until after the full device descriptor has been retrieved. Signed-off-by: Alan Stern Cc: Oliver Neukum Link: https://lore.kernel.org/r/495cb5d4-f956-4f4a-a875-1e67e9489510@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman (backported from commit 85d07c55621676d47d873d2749b88f783cd4d5a1) [yuxuan.luo: ignore the fallthrough keyword as it is yet to introduced on this tree. ] CVE-2023-37453 Signed-off-by: Yuxuan Luo --- drivers/usb/core/hub.c | 173 ++++++++++++++++++++++------------------- 1 file changed, 94 insertions(+), 79 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 228b882bc56b4..51dfdc69af10d 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4651,6 +4651,67 @@ static int hub_enable_device(struct usb_device *udev) return hcd->driver->enable_device(hcd, udev); } +/* + * Get the bMaxPacketSize0 value during initialization by reading the + * device's device descriptor. Since we don't already know this value, + * the transfer is unsafe and it ignores I/O errors, only testing for + * reasonable received values. + * + * For "old scheme" initialization, size will be 8 so we read just the + * start of the device descriptor, which should work okay regardless of + * the actual bMaxPacketSize0 value. For "new scheme" initialization, + * size will be 64 (and buf will point to a sufficiently large buffer), + * which might not be kosher according to the USB spec but it's what + * Windows does and what many devices expect. + * + * Returns: bMaxPacketSize0 or a negative error code. + */ +static int get_bMaxPacketSize0(struct usb_device *udev, + struct usb_device_descriptor *buf, int size, bool first_time) +{ + int i, rc; + + /* + * Retry on all errors; some devices are flakey. + * 255 is for WUSB devices, we actually need to use + * 512 (WUSB1.0[4.8.1]). + */ + for (i = 0; i < GET_MAXPACKET0_TRIES; ++i) { + /* Start with invalid values in case the transfer fails */ + buf->bDescriptorType = buf->bMaxPacketSize0 = 0; + rc = usb_control_msg(udev, usb_rcvaddr0pipe(), + USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, + USB_DT_DEVICE << 8, 0, + buf, size, + initial_descriptor_timeout); + switch (buf->bMaxPacketSize0) { + case 8: case 16: case 32: case 64: case 255: + if (buf->bDescriptorType == USB_DT_DEVICE) { + rc = buf->bMaxPacketSize0; + break; + } + /* FALL THROUGH */ + default: + if (rc >= 0) + rc = -EPROTO; + break; + } + + /* + * Some devices time out if they are powered on + * when already connected. They need a second + * reset, so return early. But only on the first + * attempt, lest we get into a time-out/reset loop. + */ + if (rc > 0 || (rc == -ETIMEDOUT && first_time && + udev->speed > USB_SPEED_FULL)) + break; + } + return rc; +} + +#define GET_DESCRIPTOR_BUFSIZE 64 + /* Reset device, (re)assign address, get device descriptor. * Device connection must be stable, no more debouncing needed. * Returns device in USB_STATE_ADDRESS, except on error. @@ -4675,6 +4736,12 @@ 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; + int maxp0; + struct usb_device_descriptor *buf; + + buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); + if (!buf) + return -ENOMEM; /* root hub ports have a slightly longer reset period * (from USB 2.0 spec, section 7.1.7.5) @@ -4789,9 +4856,6 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, for (retries = 0; retries < GET_DESCRIPTOR_TRIES; (++retries, msleep(100))) { if (do_new_scheme) { - struct usb_device_descriptor *buf; - int r = 0; - retval = hub_enable_device(udev); if (retval < 0) { dev_err(&udev->dev, @@ -4800,52 +4864,8 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, goto fail; } -#define GET_DESCRIPTOR_BUFSIZE 64 - buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); - if (!buf) { - retval = -ENOMEM; - continue; - } - - /* Retry on all errors; some devices are flakey. - * 255 is for WUSB devices, we actually need to use - * 512 (WUSB1.0[4.8.1]). - */ - for (operations = 0; operations < GET_MAXPACKET0_TRIES; - ++operations) { - buf->bMaxPacketSize0 = 0; - r = usb_control_msg(udev, usb_rcvaddr0pipe(), - USB_REQ_GET_DESCRIPTOR, USB_DIR_IN, - USB_DT_DEVICE << 8, 0, - buf, GET_DESCRIPTOR_BUFSIZE, - initial_descriptor_timeout); - switch (buf->bMaxPacketSize0) { - case 8: case 16: case 32: case 64: case 255: - if (buf->bDescriptorType == - USB_DT_DEVICE) { - r = 0; - break; - } - /* FALL THROUGH */ - default: - if (r == 0) - r = -EPROTO; - break; - } - /* - * Some devices time out if they are powered on - * when already connected. They need a second - * reset. But only on the first attempt, - * lest we get into a time out/reset loop - */ - if (r == 0 || (r == -ETIMEDOUT && - retries == 0 && - udev->speed > USB_SPEED_FULL)) - break; - } - udev->descriptor.bMaxPacketSize0 = - buf->bMaxPacketSize0; - kfree(buf); + maxp0 = get_bMaxPacketSize0(udev, buf, + GET_DESCRIPTOR_BUFSIZE, retries == 0); retval = hub_port_reset(hub, port1, udev, delay, false); if (retval < 0) /* error or disconnect */ @@ -4856,14 +4876,13 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, retval = -ENODEV; goto fail; } - if (r) { - if (r != -ENODEV) + if (maxp0 < 0) { + if (maxp0 != -ENODEV) dev_err(&udev->dev, "device descriptor read/64, error %d\n", - r); - retval = -EMSGSIZE; + maxp0); + retval = maxp0; continue; } -#undef GET_DESCRIPTOR_BUFSIZE } /* @@ -4905,19 +4924,17 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, break; } - retval = usb_get_device_descriptor(udev, 8); - if (retval < 8) { + /* !do_new_scheme || wusb */ + maxp0 = get_bMaxPacketSize0(udev, buf, 8, retries == 0); + if (maxp0 < 0) { + retval = maxp0; if (retval != -ENODEV) dev_err(&udev->dev, "device descriptor read/8, error %d\n", retval); - if (retval >= 0) - retval = -EMSGSIZE; } else { u32 delay; - retval = 0; - delay = udev->parent->hub_delay; udev->hub_delay = min_t(u32, delay, USB_TP_TRANSMISSION_DELAY_MAX); @@ -4934,27 +4951,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, if (retval) goto fail; - /* - * Some superspeed devices have finished the link training process - * and attached to a superspeed hub port, but the device descriptor - * got from those devices show they aren't superspeed devices. Warm - * reset the port attached by the devices can fix them. - */ - if ((udev->speed >= USB_SPEED_SUPER) && - (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) { - dev_err(&udev->dev, "got a wrong device descriptor, " - "warm reset device\n"); - hub_port_reset(hub, port1, udev, - HUB_BH_RESET_TIME, true); - retval = -EINVAL; - goto fail; - } - - if (udev->descriptor.bMaxPacketSize0 == 0xff || - udev->speed >= USB_SPEED_SUPER) + if (maxp0 == 0xff || udev->speed >= USB_SPEED_SUPER) i = 512; else - i = udev->descriptor.bMaxPacketSize0; + i = maxp0; if (usb_endpoint_maxp(&udev->ep0.desc) != i) { if (udev->speed == USB_SPEED_LOW || !(i == 8 || i == 16 || i == 32 || i == 64)) { @@ -4980,6 +4980,20 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, goto fail; } + /* + * Some superspeed devices have finished the link training process + * and attached to a superspeed hub port, but the device descriptor + * got from those devices show they aren't superspeed devices. Warm + * reset the port attached by the devices can fix them. + */ + if ((udev->speed >= USB_SPEED_SUPER) && + (le16_to_cpu(udev->descriptor.bcdUSB) < 0x0300)) { + dev_err(&udev->dev, "got a wrong device descriptor, warm reset device\n"); + hub_port_reset(hub, port1, udev, HUB_BH_RESET_TIME, true); + retval = -EINVAL; + goto fail; + } + usb_detect_quirks(udev); if (udev->wusb == 0 && le16_to_cpu(udev->descriptor.bcdUSB) >= 0x0201) { @@ -5000,6 +5014,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, hub_port_disable(hub, port1, 0); update_devnum(udev, devnum); /* for disconnect processing */ } + kfree(buf); return retval; } From patchwork Wed Oct 11 22:54:12 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yuxuan Luo X-Patchwork-Id: 1847055 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4S5Sld2jBXz1ypX for ; Thu, 12 Oct 2023 09:55:09 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1qqi6P-0001fl-Ot; Wed, 11 Oct 2023 22:54:54 +0000 Received: from smtp-relay-internal-0.internal ([10.131.114.225] helo=smtp-relay-internal-0.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1qqi60-0001UQ-Bp for kernel-team@lists.ubuntu.com; Wed, 11 Oct 2023 22:54:28 +0000 Received: from mail-qt1-f199.google.com (mail-qt1-f199.google.com [209.85.160.199]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 2C8803FA66 for ; Wed, 11 Oct 2023 22:54:28 +0000 (UTC) Received: by mail-qt1-f199.google.com with SMTP id d75a77b69052e-4191ef94106so3538121cf.0 for ; Wed, 11 Oct 2023 15:54:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697064866; x=1697669666; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=hf7Hi+r+EILNgirOkGsKaXjPck0nTlsc5bkmTXSG0Nw=; b=whF0g8OU24E9a6C+OeVwESmXCPHUNMpUXqEM4gYRujxG7YmvQ1BIJVoovHiltzV2xM Ku9Atx4NU5RgspkRgiUPhBfwQJ8Wu5SMWvwDK6hxhVxnoWi3HAEuPq0z40wHo/toEb/D BXxAM5wOQK2mCoqSkY5JWvLU6s29UYbD0Du8Z90cYNQkoC60R0pyLndGwb7PqvEX29U0 f7BVRPFlr5Ok2pj4+tQwsx9wWqnCPZTJAHEUL8kO6+QOJfx5oxgATK5C6CnBb1cz/RON rJdf0b4LG4C184irJojmjenrzkHpcG47yluQFjfs8HY1t36qBWD6dPR7OU7BMvhyYEv/ 4p3Q== X-Gm-Message-State: AOJu0YwAxSupGqx1oAwnixbTmjKN09QXrwGAqEtctYAo5Yrop/3ZtXIG UO9I5TL+vks1SvTHTqGa7+cNKsKUKlRRi5B7DV9AvJyqyRweqMTviKKQ6THziQMSedpOYvQSPG0 q2tZ0XunPb2vurMKbiydrispwU9xjxxhAuv6DY6z+Y8uHdwVOxQ== X-Received: by 2002:a05:622a:1811:b0:417:934e:ade with SMTP id t17-20020a05622a181100b00417934e0ademr28609083qtc.15.1697064866128; Wed, 11 Oct 2023 15:54:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IG3zOS4M3fDsEmVrOmrRQVa7MnnQOYL84qnTsSqc4K24NDWX/KWsbJ3bpI1nM/Vjs4oUXLQAQ== X-Received: by 2002:a05:622a:1811:b0:417:934e:ade with SMTP id t17-20020a05622a181100b00417934e0ademr28609068qtc.15.1697064865803; Wed, 11 Oct 2023 15:54:25 -0700 (PDT) Received: from cache-ubuntu.hsd1.nj.comcast.net ([2601:86:200:98b0:5308:de8d:b54e:a80e]) by smtp.gmail.com with ESMTPSA id y23-20020ac87097000000b00417db2593bdsm5691098qto.72.2023.10.11.15.54.25 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 15:54:25 -0700 (PDT) From: Yuxuan Luo To: kernel-team@lists.ubuntu.com Subject: [SRU][Focal][PATCH 4/6] usb: hub: Check device descriptor before resusciation Date: Wed, 11 Oct 2023 18:54:12 -0400 Message-Id: <20231011225414.50811-8-yuxuan.luo@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231011225414.50811-1-yuxuan.luo@canonical.com> References: <20231011225414.50811-1-yuxuan.luo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: David Heinzelmann If a device connected to an xHCI host controller disconnects from the USB bus and then reconnects, e.g. triggered by a firmware update, then the host controller automatically activates the connection and the port is enabled. The implementation of hub_port_connect_change() assumes that if the port is enabled then nothing has changed. There is no check if the USB descriptors have changed. As a result, the kernel's internal copy of the descriptors ends up being incorrect and the device doesn't work properly anymore. The solution to the problem is for hub_port_connect_change() always to check whether the device's descriptors have changed before resuscitating an enabled port. Signed-off-by: David Heinzelmann Acked-by: Alan Stern Link: https://lore.kernel.org/r/20191009044647.24536-1-heinzelmann.david@gmail.com Signed-off-by: Greg Kroah-Hartman (cherry picked from commit a4f55d8b8c146f9d99fe004bc9d1403d4c149ae3) CVE-2023-37453 Signed-off-by: Yuxuan Luo --- drivers/usb/core/hub.c | 196 +++++++++++++++++++++++------------------ 1 file changed, 111 insertions(+), 85 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 51dfdc69af10d..61e8944380161 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -5093,6 +5093,91 @@ hub_power_remaining(struct usb_hub *hub) return remaining; } + +static int descriptors_changed(struct usb_device *udev, + struct usb_device_descriptor *old_device_descriptor, + struct usb_host_bos *old_bos) +{ + int changed = 0; + unsigned index; + unsigned serial_len = 0; + unsigned len; + unsigned old_length; + int length; + char *buf; + + if (memcmp(&udev->descriptor, old_device_descriptor, + sizeof(*old_device_descriptor)) != 0) + return 1; + + if ((old_bos && !udev->bos) || (!old_bos && udev->bos)) + return 1; + if (udev->bos) { + len = le16_to_cpu(udev->bos->desc->wTotalLength); + if (len != le16_to_cpu(old_bos->desc->wTotalLength)) + return 1; + if (memcmp(udev->bos->desc, old_bos->desc, len)) + return 1; + } + + /* Since the idVendor, idProduct, and bcdDevice values in the + * device descriptor haven't changed, we will assume the + * Manufacturer and Product strings haven't changed either. + * But the SerialNumber string could be different (e.g., a + * different flash card of the same brand). + */ + if (udev->serial) + serial_len = strlen(udev->serial) + 1; + + len = serial_len; + for (index = 0; index < udev->descriptor.bNumConfigurations; index++) { + old_length = le16_to_cpu(udev->config[index].desc.wTotalLength); + len = max(len, old_length); + } + + buf = kmalloc(len, GFP_NOIO); + if (!buf) + /* assume the worst */ + return 1; + + for (index = 0; index < udev->descriptor.bNumConfigurations; index++) { + old_length = le16_to_cpu(udev->config[index].desc.wTotalLength); + length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf, + old_length); + if (length != old_length) { + dev_dbg(&udev->dev, "config index %d, error %d\n", + index, length); + changed = 1; + break; + } + if (memcmp(buf, udev->rawdescriptors[index], old_length) + != 0) { + dev_dbg(&udev->dev, "config index %d changed (#%d)\n", + index, + ((struct usb_config_descriptor *) buf)-> + bConfigurationValue); + changed = 1; + break; + } + } + + if (!changed && serial_len) { + length = usb_string(udev, udev->descriptor.iSerialNumber, + buf, serial_len); + if (length + 1 != serial_len) { + dev_dbg(&udev->dev, "serial string error %d\n", + length); + changed = 1; + } else if (memcmp(buf, udev->serial, length) != 0) { + dev_dbg(&udev->dev, "serial string changed\n"); + changed = 1; + } + } + + kfree(buf); + return changed; +} + static void hub_port_connect(struct usb_hub *hub, int port1, u16 portstatus, u16 portchange) { @@ -5340,7 +5425,9 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, { struct usb_port *port_dev = hub->ports[port1 - 1]; struct usb_device *udev = port_dev->child; + struct usb_device_descriptor descriptor; int status = -ENODEV; + int retval; dev_dbg(&port_dev->dev, "status %04x, change %04x, %s\n", portstatus, portchange, portspeed(hub, portstatus)); @@ -5361,7 +5448,30 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, if ((portstatus & USB_PORT_STAT_CONNECTION) && udev && udev->state != USB_STATE_NOTATTACHED) { if (portstatus & USB_PORT_STAT_ENABLE) { - status = 0; /* Nothing to do */ + /* + * USB-3 connections are initialized automatically by + * the hostcontroller hardware. Therefore check for + * changed device descriptors before resuscitating the + * device. + */ + descriptor = udev->descriptor; + retval = usb_get_device_descriptor(udev, + sizeof(udev->descriptor)); + if (retval < 0) { + dev_dbg(&udev->dev, + "can't read device descriptor %d\n", + retval); + } else { + if (descriptors_changed(udev, &descriptor, + udev->bos)) { + dev_dbg(&udev->dev, + "device descriptor has changed\n"); + /* for disconnect() calls */ + udev->descriptor = descriptor; + } else { + status = 0; /* Nothing to do */ + } + } #ifdef CONFIG_PM } else if (udev->state == USB_STATE_SUSPENDED && udev->persist_enabled) { @@ -5745,90 +5855,6 @@ void usb_hub_cleanup(void) usb_deregister(&hub_driver); } /* usb_hub_cleanup() */ -static int descriptors_changed(struct usb_device *udev, - struct usb_device_descriptor *old_device_descriptor, - struct usb_host_bos *old_bos) -{ - int changed = 0; - unsigned index; - unsigned serial_len = 0; - unsigned len; - unsigned old_length; - int length; - char *buf; - - if (memcmp(&udev->descriptor, old_device_descriptor, - sizeof(*old_device_descriptor)) != 0) - return 1; - - if ((old_bos && !udev->bos) || (!old_bos && udev->bos)) - return 1; - if (udev->bos) { - len = le16_to_cpu(udev->bos->desc->wTotalLength); - if (len != le16_to_cpu(old_bos->desc->wTotalLength)) - return 1; - if (memcmp(udev->bos->desc, old_bos->desc, len)) - return 1; - } - - /* Since the idVendor, idProduct, and bcdDevice values in the - * device descriptor haven't changed, we will assume the - * Manufacturer and Product strings haven't changed either. - * But the SerialNumber string could be different (e.g., a - * different flash card of the same brand). - */ - if (udev->serial) - serial_len = strlen(udev->serial) + 1; - - len = serial_len; - for (index = 0; index < udev->descriptor.bNumConfigurations; index++) { - old_length = le16_to_cpu(udev->config[index].desc.wTotalLength); - len = max(len, old_length); - } - - buf = kmalloc(len, GFP_NOIO); - if (!buf) - /* assume the worst */ - return 1; - - for (index = 0; index < udev->descriptor.bNumConfigurations; index++) { - old_length = le16_to_cpu(udev->config[index].desc.wTotalLength); - length = usb_get_descriptor(udev, USB_DT_CONFIG, index, buf, - old_length); - if (length != old_length) { - dev_dbg(&udev->dev, "config index %d, error %d\n", - index, length); - changed = 1; - break; - } - if (memcmp(buf, udev->rawdescriptors[index], old_length) - != 0) { - dev_dbg(&udev->dev, "config index %d changed (#%d)\n", - index, - ((struct usb_config_descriptor *) buf)-> - bConfigurationValue); - changed = 1; - break; - } - } - - if (!changed && serial_len) { - length = usb_string(udev, udev->descriptor.iSerialNumber, - buf, serial_len); - if (length + 1 != serial_len) { - dev_dbg(&udev->dev, "serial string error %d\n", - length); - changed = 1; - } else if (memcmp(buf, udev->serial, length) != 0) { - dev_dbg(&udev->dev, "serial string changed\n"); - changed = 1; - } - } - - kfree(buf); - return changed; -} - /** * usb_reset_and_verify_device - perform a USB port reset to reinitialize a device * @udev: device to reset (not in SUSPENDED or NOTATTACHED state) From patchwork Wed Oct 11 22:54:13 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yuxuan Luo X-Patchwork-Id: 1847060 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4S5Smq6NwSz1ypX for ; Thu, 12 Oct 2023 09:56:11 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1qqi7Z-0002eW-9W; Wed, 11 Oct 2023 22:56:05 +0000 Received: from smtp-relay-internal-1.internal ([10.131.114.114] helo=smtp-relay-internal-1.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1qqi61-0001Uw-6N for kernel-team@lists.ubuntu.com; Wed, 11 Oct 2023 22:54:29 +0000 Received: from mail-oi1-f198.google.com (mail-oi1-f198.google.com [209.85.167.198]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-1.canonical.com (Postfix) with ESMTPS id E227F404A1 for ; Wed, 11 Oct 2023 22:54:28 +0000 (UTC) Received: by mail-oi1-f198.google.com with SMTP id 5614622812f47-3ae4af74db8so455387b6e.0 for ; Wed, 11 Oct 2023 15:54:28 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697064867; x=1697669667; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=VY3DNG21M8r2laVHfwUvNmw0G3GY08Ez4OarI14nGVA=; b=j6Izj7bbXL0gbdNYmEWdZBYeDTiDYqv8qYq1olrwrZufaw4sXRK4ft++QDx/mtq727 Jeh8BUSb7m7ylKl5es/zieLq29JhyBi2DCoEyLTseS/uPvn/QboDM2P4FuJtJBqWbFu3 h0ksMEGsrbCiO9S2kNjv6rH/Oi92DPyZoGaJDA7yPV1zkw1+ctbTzBLWCaplcwsjOZ1U pwHv8dKnxPvs/sOhvN6s+bgAeQVfnQJAILxr2RnL3ClzcA7dvSQdI/2r+MShZlAg9R3D K8SHH0OxF/p13pCbq92jkNB8hMp3YQZVrswJWQ03UvYsLhAlsrebFDRJnNh1WsnEzM6a FDoQ== X-Gm-Message-State: AOJu0Yzdj4z9Sdf2STO/92OVCORKh3YVtjBliqpl212Vh5ZFV8LqB35r 6xrzyQ+KIPqPLv4FXUITOfp6UU8g2dCHqSTxkeRxH78LuDYtdr4kg5F9E8wWv1EToxMh+oaKiD+ kBWp0Ps5WpmKyfbxOeeSGtq8cSFVj5PO6Uk0QooeU8q/ov6scZg== X-Received: by 2002:aca:1a04:0:b0:3a7:af4c:2406 with SMTP id a4-20020aca1a04000000b003a7af4c2406mr23005078oia.44.1697064866997; Wed, 11 Oct 2023 15:54:26 -0700 (PDT) X-Google-Smtp-Source: AGHT+IH9bcZ/9tjs+PGHnphc1cKHHakAtFuJs5Fqn/m3HPe9UUBmFe1MJzud4RRz1FQ24znkI4536Q== X-Received: by 2002:aca:1a04:0:b0:3a7:af4c:2406 with SMTP id a4-20020aca1a04000000b003a7af4c2406mr23005067oia.44.1697064866569; Wed, 11 Oct 2023 15:54:26 -0700 (PDT) Received: from cache-ubuntu.hsd1.nj.comcast.net ([2601:86:200:98b0:5308:de8d:b54e:a80e]) by smtp.gmail.com with ESMTPSA id y23-20020ac87097000000b00417db2593bdsm5691098qto.72.2023.10.11.15.54.25 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 15:54:26 -0700 (PDT) From: Yuxuan Luo To: kernel-team@lists.ubuntu.com Subject: [SRU][Focal][PATCH 5/6] USB: core: Change usb_get_device_descriptor() API Date: Wed, 11 Oct 2023 18:54:13 -0400 Message-Id: <20231011225414.50811-9-yuxuan.luo@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231011225414.50811-1-yuxuan.luo@canonical.com> References: <20231011225414.50811-1-yuxuan.luo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Alan Stern The usb_get_device_descriptor() routine reads the device descriptor from the udev device and stores it directly in udev->descriptor. This interface is error prone, because the USB subsystem expects in-memory copies of a device's descriptors to be immutable once the device has been initialized. The interface is changed so that the device descriptor is left in a kmalloc-ed buffer, not copied into the usb_device structure. A pointer to the buffer is returned to the caller, who is then responsible for kfree-ing it. The corresponding changes needed in the various callers are fairly small. Signed-off-by: Alan Stern Link: https://lore.kernel.org/r/d0111bb6-56c1-4f90-adf2-6cfe152f6561@rowland.harvard.edu Signed-off-by: Greg Kroah-Hartman (backported from commit de28e469da75359a2bb8cd8778b78aa64b1be1f4) [yuxuan.luo: ignored the comment conflict in message.c] CVE-2023-37453 Signed-off-by: Yuxuan Luo --- drivers/usb/core/hcd.c | 10 ++++++--- drivers/usb/core/hub.c | 44 ++++++++++++++++++++------------------ drivers/usb/core/message.c | 30 +++++++++++--------------- drivers/usb/core/usb.h | 4 ++-- 4 files changed, 45 insertions(+), 43 deletions(-) diff --git a/drivers/usb/core/hcd.c b/drivers/usb/core/hcd.c index 25fb30fdba99e..d476a6503979d 100644 --- a/drivers/usb/core/hcd.c +++ b/drivers/usb/core/hcd.c @@ -981,6 +981,7 @@ static int register_root_hub(struct usb_hcd *hcd) { struct device *parent_dev = hcd->self.controller; struct usb_device *usb_dev = hcd->self.root_hub; + struct usb_device_descriptor *descr; const int devnum = 1; int retval; @@ -992,13 +993,16 @@ static int register_root_hub(struct usb_hcd *hcd) mutex_lock(&usb_bus_idr_lock); usb_dev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); - retval = usb_get_device_descriptor(usb_dev, USB_DT_DEVICE_SIZE); - if (retval != sizeof usb_dev->descriptor) { + descr = usb_get_device_descriptor(usb_dev); + if (IS_ERR(descr)) { + retval = PTR_ERR(descr); mutex_unlock(&usb_bus_idr_lock); dev_dbg (parent_dev, "can't read %s device descriptor %d\n", dev_name(&usb_dev->dev), retval); - return (retval < 0) ? retval : -EMSGSIZE; + return retval; } + usb_dev->descriptor = *descr; + kfree(descr); if (le16_to_cpu(usb_dev->descriptor.bcdUSB) >= 0x0201) { retval = usb_get_bos_descriptor(usb_dev); diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 61e8944380161..44bc27170e8ce 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -2645,12 +2645,17 @@ int usb_authorize_device(struct usb_device *usb_dev) } if (usb_dev->wusb) { - result = usb_get_device_descriptor(usb_dev, sizeof(usb_dev->descriptor)); - if (result < 0) { + struct usb_device_descriptor *descr; + + descr = usb_get_device_descriptor(usb_dev); + if (IS_ERR(descr)) { + result = PTR_ERR(descr); dev_err(&usb_dev->dev, "can't re-read device descriptor for " "authorization: %d\n", result); goto error_device_descriptor; } + usb_dev->descriptor = *descr; + kfree(descr); } usb_dev->authorized = 1; @@ -4737,7 +4742,7 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, const char *driver_name; bool do_new_scheme; int maxp0; - struct usb_device_descriptor *buf; + struct usb_device_descriptor *buf, *descr; buf = kmalloc(GET_DESCRIPTOR_BUFSIZE, GFP_NOIO); if (!buf) @@ -4970,15 +4975,16 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, usb_ep0_reinit(udev); } - retval = usb_get_device_descriptor(udev, USB_DT_DEVICE_SIZE); - if (retval < (signed)sizeof(udev->descriptor)) { + descr = usb_get_device_descriptor(udev); + if (IS_ERR(descr)) { + retval = PTR_ERR(descr); if (retval != -ENODEV) dev_err(&udev->dev, "device descriptor read/all, error %d\n", retval); - if (retval >= 0) - retval = -ENOMSG; goto fail; } + udev->descriptor = *descr; + kfree(descr); /* * Some superspeed devices have finished the link training process @@ -5095,7 +5101,7 @@ hub_power_remaining(struct usb_hub *hub) static int descriptors_changed(struct usb_device *udev, - struct usb_device_descriptor *old_device_descriptor, + struct usb_device_descriptor *new_device_descriptor, struct usb_host_bos *old_bos) { int changed = 0; @@ -5106,8 +5112,8 @@ static int descriptors_changed(struct usb_device *udev, int length; char *buf; - if (memcmp(&udev->descriptor, old_device_descriptor, - sizeof(*old_device_descriptor)) != 0) + if (memcmp(&udev->descriptor, new_device_descriptor, + sizeof(*new_device_descriptor)) != 0) return 1; if ((old_bos && !udev->bos) || (!old_bos && udev->bos)) @@ -5425,9 +5431,8 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, { struct usb_port *port_dev = hub->ports[port1 - 1]; struct usb_device *udev = port_dev->child; - struct usb_device_descriptor descriptor; + struct usb_device_descriptor *descr; int status = -ENODEV; - int retval; dev_dbg(&port_dev->dev, "status %04x, change %04x, %s\n", portstatus, portchange, portspeed(hub, portstatus)); @@ -5454,23 +5459,20 @@ static void hub_port_connect_change(struct usb_hub *hub, int port1, * changed device descriptors before resuscitating the * device. */ - descriptor = udev->descriptor; - retval = usb_get_device_descriptor(udev, - sizeof(udev->descriptor)); - if (retval < 0) { + descr = usb_get_device_descriptor(udev); + if (IS_ERR(descr)) { dev_dbg(&udev->dev, - "can't read device descriptor %d\n", - retval); + "can't read device descriptor %ld\n", + PTR_ERR(descr)); } else { - if (descriptors_changed(udev, &descriptor, + if (descriptors_changed(udev, descr, udev->bos)) { dev_dbg(&udev->dev, "device descriptor has changed\n"); - /* for disconnect() calls */ - udev->descriptor = descriptor; } else { status = 0; /* Nothing to do */ } + kfree(descr); } #ifdef CONFIG_PM } else if (udev->state == USB_STATE_SUSPENDED && diff --git a/drivers/usb/core/message.c b/drivers/usb/core/message.c index f67b30e4b0c84..b055dfc3a5868 100644 --- a/drivers/usb/core/message.c +++ b/drivers/usb/core/message.c @@ -1038,39 +1038,35 @@ char *usb_cache_string(struct usb_device *udev, int index) } /* - * usb_get_device_descriptor - (re)reads the device descriptor (usbcore) - * @dev: the device whose device descriptor is being updated - * @size: how much of the descriptor to read - * Context: !in_interrupt () + * usb_get_device_descriptor - read the device descriptor + * @udev: the device whose device descriptor should be read * - * Updates the copy of the device descriptor stored in the device structure, - * which dedicates space for this purpose. + * Context: task context, might sleep. * * Not exported, only for use by the core. If drivers really want to read * the device descriptor directly, they can call usb_get_descriptor() with * type = USB_DT_DEVICE and index = 0. * - * This call is synchronous, and may not be used in an interrupt context. - * - * Return: The number of bytes received on success, or else the status code - * returned by the underlying usb_control_msg() call. + * Returns: a pointer to a dynamically allocated usb_device_descriptor + * structure (which the caller must deallocate), or an ERR_PTR value. */ -int usb_get_device_descriptor(struct usb_device *dev, unsigned int size) +struct usb_device_descriptor *usb_get_device_descriptor(struct usb_device *udev) { struct usb_device_descriptor *desc; int ret; - if (size > sizeof(*desc)) - return -EINVAL; desc = kmalloc(sizeof(*desc), GFP_NOIO); if (!desc) - return -ENOMEM; + return ERR_PTR(-ENOMEM); + + ret = usb_get_descriptor(udev, USB_DT_DEVICE, 0, desc, sizeof(*desc)); + if (ret == sizeof(*desc)) + return desc; - ret = usb_get_descriptor(dev, USB_DT_DEVICE, 0, desc, size); if (ret >= 0) - memcpy(&dev->descriptor, desc, size); + ret = -EMSGSIZE; kfree(desc); - return ret; + return ERR_PTR(ret); } /* diff --git a/drivers/usb/core/usb.h b/drivers/usb/core/usb.h index 3ad0ee57e859f..007117f307a65 100644 --- a/drivers/usb/core/usb.h +++ b/drivers/usb/core/usb.h @@ -42,8 +42,8 @@ extern bool usb_endpoint_is_blacklisted(struct usb_device *udev, struct usb_endpoint_descriptor *epd); extern int usb_remove_device(struct usb_device *udev); -extern int usb_get_device_descriptor(struct usb_device *dev, - unsigned int size); +extern struct usb_device_descriptor *usb_get_device_descriptor( + struct usb_device *udev); extern int usb_set_isoch_delay(struct usb_device *dev); extern int usb_get_bos_descriptor(struct usb_device *dev); extern void usb_release_bos_descriptor(struct usb_device *dev); From patchwork Wed Oct 11 22:54:14 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Yuxuan Luo X-Patchwork-Id: 1847057 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@legolas.ozlabs.org Authentication-Results: legolas.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=lists.ubuntu.com (client-ip=185.125.189.65; helo=lists.ubuntu.com; envelope-from=kernel-team-bounces@lists.ubuntu.com; receiver=patchwork.ozlabs.org) Received: from lists.ubuntu.com (lists.ubuntu.com [185.125.189.65]) (using TLSv1.2 with cipher ECDHE-ECDSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by legolas.ozlabs.org (Postfix) with ESMTPS id 4S5SmM6yP0z1ypX for ; Thu, 12 Oct 2023 09:55:47 +1100 (AEDT) Received: from localhost ([127.0.0.1] helo=lists.ubuntu.com) by lists.ubuntu.com with esmtp (Exim 4.86_2) (envelope-from ) id 1qqi6x-0001zx-OY; Wed, 11 Oct 2023 22:55:28 +0000 Received: from smtp-relay-internal-0.internal ([10.131.114.225] helo=smtp-relay-internal-0.canonical.com) by lists.ubuntu.com with esmtps (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.86_2) (envelope-from ) id 1qqi63-0001VD-1f for kernel-team@lists.ubuntu.com; Wed, 11 Oct 2023 22:54:33 +0000 Received: from mail-qt1-f200.google.com (mail-qt1-f200.google.com [209.85.160.200]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by smtp-relay-internal-0.canonical.com (Postfix) with ESMTPS id 6E1873F231 for ; Wed, 11 Oct 2023 22:54:29 +0000 (UTC) Received: by mail-qt1-f200.google.com with SMTP id d75a77b69052e-4180b3527c7so3902311cf.2 for ; Wed, 11 Oct 2023 15:54:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697064868; x=1697669668; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=7ZRav90f4O3zN0ooJWebXX8UdeqWdMb20E/S/J4209I=; b=AVz/xHCJdTtBNycRT935zCohHzLO3i0jrvyHw4Zxq/9UA8sXSPmnP1acl7YKsePL+6 BHo7exj5CWSEEjYrQ7QCQBrE+B3NKK04gDG3K9uHtgOJd8iAq0HZ/v+/cf59vPzJ7YJx 7KyBs7IIx1ZxSV/E8EOt3q6wnNbm5bZWG/lI8tB2rCxekAJjeTGIHeT79jPJlF9yTzoJ jOi+fcDfXIN1lF62E1POSdhaEzfET1K3h/rfuwD+1Avv0vIxPxYZ+zh7How+XatD7eDs yL8eePQNSSRI4MruAo27QKx63GJ6P2tm7U45qZnYXEWVAn10DUm7IrCTc3hLZKsLXqSv rirQ== X-Gm-Message-State: AOJu0Yyx28HaNndbwd6BNXlerh8J4alUgKgdI88SHwLhorj9GE9dJRp/ l6PFpxziWvdAd5xXg+rC+YVv8+Hby7cKPl6IcFAAS+vaxPOPJXsy4Wa6FxnNWMl92Ca5rIvm8kA 4YP46RCUq4gQ8MmVTP/cTHknSnotOrd2+rF28y58HdLj+2cPSJQ== X-Received: by 2002:ac8:5a04:0:b0:405:5092:7dce with SMTP id n4-20020ac85a04000000b0040550927dcemr29231538qta.3.1697064867829; Wed, 11 Oct 2023 15:54:27 -0700 (PDT) X-Google-Smtp-Source: AGHT+IHrmB2Emh3dBHIF2Q9JMYYnF9FjroMYa6IvfkVS8WgDGfO6gB+/9fDKLD4hDaqtlZrGODIlxA== X-Received: by 2002:ac8:5a04:0:b0:405:5092:7dce with SMTP id n4-20020ac85a04000000b0040550927dcemr29231523qta.3.1697064867446; Wed, 11 Oct 2023 15:54:27 -0700 (PDT) Received: from cache-ubuntu.hsd1.nj.comcast.net ([2601:86:200:98b0:5308:de8d:b54e:a80e]) by smtp.gmail.com with ESMTPSA id y23-20020ac87097000000b00417db2593bdsm5691098qto.72.2023.10.11.15.54.26 for (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 15:54:26 -0700 (PDT) From: Yuxuan Luo To: kernel-team@lists.ubuntu.com Subject: [SRU][Focal][PATCH 6/6] USB: core: Fix race by not overwriting udev->descriptor in hub_port_init() Date: Wed, 11 Oct 2023 18:54:14 -0400 Message-Id: <20231011225414.50811-10-yuxuan.luo@canonical.com> X-Mailer: git-send-email 2.34.1 In-Reply-To: <20231011225414.50811-1-yuxuan.luo@canonical.com> References: <20231011225414.50811-1-yuxuan.luo@canonical.com> MIME-Version: 1.0 X-BeenThere: kernel-team@lists.ubuntu.com X-Mailman-Version: 2.1.20 Precedence: list List-Id: Kernel team discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: kernel-team-bounces@lists.ubuntu.com Sender: "kernel-team" From: Alan Stern 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: __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 45bf39f8df7f ("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: syzbot+18996170f8096c6174d0@syzkaller.appspotmail.com Closes: https://lore.kernel.org/linux-usb/000000000000c0ffe505fe86c9ca@google.com/#r Signed-off-by: Alan Stern Cc: Khazhy Kumykov Fixes: 45bf39f8df7f ("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: Greg Kroah-Hartman (cherry picked from commit ff33299ec8bb80cdcc073ad9c506bd79bb2ed20b) CVE-2023-37453 Signed-off-by: Yuxuan Luo --- drivers/usb/core/hub.c | 114 +++++++++++++++++++++++++---------------- 1 file changed, 70 insertions(+), 44 deletions(-) diff --git a/drivers/usb/core/hub.c b/drivers/usb/core/hub.c index 44bc27170e8ce..4007cece638b4 100644 --- a/drivers/usb/core/hub.c +++ b/drivers/usb/core/hub.c @@ -4726,10 +4726,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); @@ -4741,6 +4748,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; @@ -4779,32 +4787,34 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, } oldspeed = udev->speed; - /* 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 - * reported as 0xff in the device descriptor). WUSB1.0[4.8.1]. - */ - switch (udev->speed) { - case USB_SPEED_SUPER_PLUS: - case USB_SPEED_SUPER: - case USB_SPEED_WIRELESS: /* fixed at 512 */ - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512); - break; - case USB_SPEED_HIGH: /* fixed at 64 */ - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); - break; - case USB_SPEED_FULL: /* 8, 16, 32, or 64 */ - /* to determine the ep0 maxpacket size, try to read - * the device descriptor to get bMaxPacketSize0 and - * then correct our initial guess. + 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 + * reported as 0xff in the device descriptor). WUSB1.0[4.8.1]. */ - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); - break; - case USB_SPEED_LOW: /* fixed at 8 */ - udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8); - break; - default: - goto fail; + switch (udev->speed) { + case USB_SPEED_SUPER_PLUS: + case USB_SPEED_SUPER: + case USB_SPEED_WIRELESS: /* fixed at 512 */ + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(512); + break; + case USB_SPEED_HIGH: /* fixed at 64 */ + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); + break; + case USB_SPEED_FULL: /* 8, 16, 32, or 64 */ + /* to determine the ep0 maxpacket size, try to read + * the device descriptor to get bMaxPacketSize0 and + * then correct our initial guess. + */ + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(64); + break; + case USB_SPEED_LOW: /* fixed at 8 */ + udev->ep0.desc.wMaxPacketSize = cpu_to_le16(8); + break; + default: + goto fail; + } } if (udev->speed == USB_SPEED_WIRELESS) @@ -4827,22 +4837,24 @@ 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); - /* Set up TT records, if needed */ - if (hdev->tt) { - udev->tt = hdev->tt; - udev->ttport = hdev->ttport; - } else if (udev->speed != USB_SPEED_HIGH - && hdev->speed == USB_SPEED_HIGH) { - if (!hub->tt.hub) { - dev_err(&udev->dev, "parent hub has no TT\n"); - retval = -EINVAL; - goto fail; + if (initial) { + /* Set up TT records, if needed */ + if (hdev->tt) { + udev->tt = hdev->tt; + udev->ttport = hdev->ttport; + } else if (udev->speed != USB_SPEED_HIGH + && hdev->speed == USB_SPEED_HIGH) { + if (!hub->tt.hub) { + dev_err(&udev->dev, "parent hub has no TT\n"); + retval = -EINVAL; + goto fail; + } + udev->tt = &hub->tt; + udev->ttport = port1; } - udev->tt = &hub->tt; - udev->ttport = port1; } /* Why interleave GET_DESCRIPTOR and SET_ADDRESS this way? @@ -4871,6 +4883,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 */ @@ -4940,6 +4958,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); @@ -4983,7 +5007,10 @@ hub_port_init(struct usb_hub *hub, struct usb_device *udev, int port1, retval); goto fail; } - udev->descriptor = *descr; + if (initial) + udev->descriptor = *descr; + else + *dev_descr = *descr; kfree(descr); /* @@ -5287,7 +5314,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; @@ -5896,7 +5923,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; @@ -5938,7 +5965,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; } @@ -5950,7 +5977,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; }