[PATCH] usb: fix various gadget panics on 10gbps cabling

[PATCH] usb: fix various gadget panics on 10gbps cabling

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 drivers/usb/gadget/config.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/gadget/config.c b/drivers/usb/gadget/config.c
index 8bb25773b61e..d624f9f57f34 100644
--- a/drivers/usb/gadget/config.c
+++ b/drivers/usb/gadget/config.c
@@ -164,6 +164,10 @@ int usb_assign_descriptors(struct usb_function *f,
 {
 	struct usb_gadget *g = f->config->cdev->gadget;
 
+	/* In most cases this is good enough as a default */
+	if (!ssp)
+		ssp = ss;
+
 	if (fs) {
 		f->fs_descriptors = usb_copy_descriptors(fs);
 		if (!f->fs_descriptors)
-- 
2.32.0.rc1.229.g3e70b5a671-goog

Re: [PATCH] usb: fix various gadget panics on 10gbps cabling

[Greg Kroah-Hartman]

On Mon, Jun 07, 2021 at 09:42:01PM -0700, Maciej Żenczykowski wrote:
> From: Maciej Żenczykowski <maze@google.com>
> 
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>
> ---
>  drivers/usb/gadget/config.c | 4 ++++
>  1 file changed, 4 insertions(+)

I can not take patches without any changelog text at all, sorry.

greg k-h

[PATCH v2] usb: fix various gadget panics on 10gbps cabling

[Maciej Żenczykowski]

From: Maciej Żenczykowski <maze@google.com>

usb_assign_descriptors() is called with 5 parameters,
the last 4 of which are the usb_descriptor_header for:
  full-speed (USB1.1 - 12Mbps [including USB1.0 low-speed @ 1.5Mbps),
  high-speed (USB2.0 - 480Mbps),
  super-speed (USB3.0 - 5Gbps),
  super-speed-plus (USB3.1 - 10Gbps).

The differences between full/high/super-speed descriptors are usually
substantial (due to changes in the maximum usb block size from 64 to 512
to 1024 bytes and other differences in the specs), while the difference
between 5 and 10Gbps descriptors may be as little as nothing
(in many cases the same tuning is simply good enough).

However if a gadget driver calls usb_assign_descriptors() with
a NULL descriptor for super-speed-plus and is then used on a max 10gbps
configuration, the kernel will crash with a null pointer dereference,
when a 10gbps capable device port + cable + host port combination shows up.
(This wouldn't happen if the gadget max-speed was set to 5gbps, but
it of course defaults to the maximum, and there's no real reason to
artificially limit it)

The fix is to simply use the 5gbps descriptor as the 10gbps descriptor,
if a 10gbps descriptor wasn't provided.

Obviously this won't fix the problem if the 5gbps descriptor is also
NULL, but such cases can't be so trivially solved (and any such gadgets
are unlikely to be used with USB3 ports any way).

Cc: Felipe Balbi <balbi@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Signed-off-by: Maciej Żenczykowski <maze@google.com>
---
 drivers/usb/gadget/config.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/drivers/usb/gadget/config.c b/drivers/usb/gadget/config.c
index 8bb25773b61e..05507606b2b4 100644
--- a/drivers/usb/gadget/config.c
+++ b/drivers/usb/gadget/config.c
@@ -164,6 +164,14 @@ int usb_assign_descriptors(struct usb_function *f,
 {
 	struct usb_gadget *g = f->config->cdev->gadget;
 
+	/* super-speed-plus descriptor falls back to super-speed one,
+	 * if such a descriptor was provided, thus avoiding a NULL
+	 * pointer dereference if a 5gbps capable gadget is used with
+	 * a 10gbps capable config (device port + cable + host port)
+	 */
+	if (!ssp)
+		ssp = ss;
+
 	if (fs) {
 		f->fs_descriptors = usb_copy_descriptors(fs);
 		if (!f->fs_descriptors)
-- 
2.32.0.rc1.229.g3e70b5a671-goog

Re: [PATCH v2] usb: fix various gadget panics on 10gbps cabling

[Felipe Balbi]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

Maciej Żenczykowski <zenczykowski@gmail.com> writes:

> From: Maciej Żenczykowski <maze@google.com>
>
> usb_assign_descriptors() is called with 5 parameters,
> the last 4 of which are the usb_descriptor_header for:
>   full-speed (USB1.1 - 12Mbps [including USB1.0 low-speed @ 1.5Mbps),
>   high-speed (USB2.0 - 480Mbps),
>   super-speed (USB3.0 - 5Gbps),
>   super-speed-plus (USB3.1 - 10Gbps).
>
> The differences between full/high/super-speed descriptors are usually
> substantial (due to changes in the maximum usb block size from 64 to 512
> to 1024 bytes and other differences in the specs), while the difference
> between 5 and 10Gbps descriptors may be as little as nothing
> (in many cases the same tuning is simply good enough).
>
> However if a gadget driver calls usb_assign_descriptors() with
> a NULL descriptor for super-speed-plus and is then used on a max 10gbps
> configuration, the kernel will crash with a null pointer dereference,
> when a 10gbps capable device port + cable + host port combination shows up.
> (This wouldn't happen if the gadget max-speed was set to 5gbps, but
> it of course defaults to the maximum, and there's no real reason to
> artificially limit it)
>
> The fix is to simply use the 5gbps descriptor as the 10gbps descriptor,
> if a 10gbps descriptor wasn't provided.
>
> Obviously this won't fix the problem if the 5gbps descriptor is also
> NULL, but such cases can't be so trivially solved (and any such gadgets
> are unlikely to be used with USB3 ports any way).
>
> Cc: Felipe Balbi <balbi@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Signed-off-by: Maciej Żenczykowski <maze@google.com>

nice catch!!

I think this is already in Greg's tree, but in any  case:

Acked-by: Felipe Balbi <balbi@kernel.org>

-- 
balbi

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 511 bytes --]