[PATCH] greybus: usb: fix response buffer size for fixed-length hub requests

[PATCH] greybus: usb: fix response buffer size for fixed-length hub requests

[Aditya Chari S]

hub_control() sized the Greybus operation's response buffer purely
off the caller-supplied wLength. However, per the USB hub class spec,
GetHubDescriptor, GetHubStatus, and GetPortStatus have a response
length that is fixed by the request type itself, and can be larger
than (or independent of) whatever wLength the USB core happens to
pass down. This could under-allocate the response buffer for these
request types.

Special-case these three request types so the response buffer is
always large enough for the data the module will actually send back,
matching the equivalent handling in usbcore's rh_call_control(). Fall
back to wLength for all other request types, as before.

Compile-tested with 'make M=drivers/staging/greybus C=1', including
sparse, with no warnings. checkpatch --strict is also clean.

Note: this driver's hub_control() is currently unreachable at runtime
since gb_usb_probe() unconditionally disables USB support pending
separate USB core changes. As Project Ara hardware is no longer
available, this change has not been tested on physical hardware or
with a Greybus module; it was verified only by compilation, sparse,
and code review against the equivalent logic in usbcore.

Signed-off-by: Aditya Chari S <adi25charis@gmail.com>
---
 drivers/staging/greybus/usb.c | 31 +++++++++++++++++++++++++++----
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
index 475f24f20..ea6c0800c 100644
--- a/drivers/staging/greybus/usb.c
+++ b/drivers/staging/greybus/usb.c
@@ -105,8 +105,29 @@ static int hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
 	size_t response_size;
 	int ret;
 
-	/* FIXME: handle unspecified lengths */
-	response_size = sizeof(*response) + wLength;
+	/*
+	 * Several USB hub class requests have a response length that is
+	 * fixed by the request type rather than by wLength (which may be
+	 * zero, or otherwise smaller than the actual data the module will
+	 * return). Make sure we always allocate enough room for those.
+	 */
+	switch (typeReq) {
+	case GetHubDescriptor:
+		response_size = sizeof(*response) +
+				 sizeof(struct usb_hub_descriptor);
+		break;
+	case GetHubStatus:
+		response_size = sizeof(*response) +
+				 sizeof(struct usb_hub_status);
+		break;
+	case GetPortStatus:
+		response_size = sizeof(*response) +
+				 sizeof(struct usb_port_status);
+		break;
+	default:
+		response_size = sizeof(*response) + wLength;
+		break;
+	}
 
 	operation = gb_operation_create(dev->connection,
 					GB_USB_TYPE_HUB_CONTROL,
@@ -126,10 +147,12 @@ static int hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
 	if (ret)
 		goto out;
 
-	if (wLength) {
+	if (response_size > sizeof(*response)) {
+		size_t data_size = response_size - sizeof(*response);
+
 		/* Greybus core has verified response size */
 		response = operation->response->payload;
-		memcpy(buf, response->buf, wLength);
+		memcpy(buf, response->buf, data_size);
 	}
 out:
 	gb_operation_put(operation);
-- 
2.53.0

Re: [PATCH] greybus: usb: fix response buffer size for fixed-length hub requests

[Greg Kroah-Hartman]

On Mon, Aug 03, 2026 at 06:32:48AM +0530, Aditya Chari S wrote:
> hub_control() sized the Greybus operation's response buffer purely
> off the caller-supplied wLength. However, per the USB hub class spec,
> GetHubDescriptor, GetHubStatus, and GetPortStatus have a response
> length that is fixed by the request type itself, and can be larger
> than (or independent of) whatever wLength the USB core happens to
> pass down. This could under-allocate the response buffer for these
> request types.
> 
> Special-case these three request types so the response buffer is
> always large enough for the data the module will actually send back,
> matching the equivalent handling in usbcore's rh_call_control(). Fall
> back to wLength for all other request types, as before.
> 
> Compile-tested with 'make M=drivers/staging/greybus C=1', including
> sparse, with no warnings. checkpatch --strict is also clean.
> 
> Note: this driver's hub_control() is currently unreachable at runtime
> since gb_usb_probe() unconditionally disables USB support pending
> separate USB core changes. As Project Ara hardware is no longer
> available, this change has not been tested on physical hardware or
> with a Greybus module; it was verified only by compilation, sparse,
> and code review against the equivalent logic in usbcore.
> 
> Signed-off-by: Aditya Chari S <adi25charis@gmail.com>
> ---
>  drivers/staging/greybus/usb.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)

Cool, what tool was used to find this?  Did you forget the Assisted-by:
tag?

thanks,

greg k-h

Re: [PATCH] greybus: usb: fix response buffer size for fixed-length hub requests

[Dan Carpenter]

On Mon, Aug 03, 2026 at 06:32:48AM +0530, Aditya Chari S wrote:
> hub_control() sized the Greybus operation's response buffer purely
> off the caller-supplied wLength. However, per the USB hub class spec,
> GetHubDescriptor, GetHubStatus, and GetPortStatus have a response
> length that is fixed by the request type itself, and can be larger
> than (or independent of) whatever wLength the USB core happens to
> pass down. This could under-allocate the response buffer for these
> request types.

What is the impact?  Does it truncate the buffer or does it lead to
a buffer overflow?  Presumably Greg knows off the top of his head
since he maintains USB, but I'm not familiar with it.  Which function
does the memcpy() so I can check?

> 
> Special-case these three request types so the response buffer is
> always large enough for the data the module will actually send back,
> matching the equivalent handling in usbcore's rh_call_control(). Fall
> back to wLength for all other request types, as before.
> 

It sort of matches rh_call_control() except GetPortStatus here is
always 8 where in rh_call_control() it's sometimes 4, sometimes 8.

> Compile-tested with 'make M=drivers/staging/greybus C=1', including
> sparse, with no warnings. checkpatch --strict is also clean.
> 
> Note: this driver's hub_control() is currently unreachable at runtime
> since gb_usb_probe() unconditionally disables USB support pending
> separate USB core changes. As Project Ara hardware is no longer
> available, this change has not been tested on physical hardware or
> with a Greybus module; it was verified only by compilation, sparse,
> and code review against the equivalent logic in usbcore.

We disabled USB support in commit a96493560cd1 ("greybus: usb: disable
protocol driver") which was 11 year ago now.  I almost doubt it's ever
going to be fixed.  People are probably using out of tree versions of
this driver.

> 
> Signed-off-by: Aditya Chari S <adi25charis@gmail.com>
> ---
>  drivers/staging/greybus/usb.c | 31 +++++++++++++++++++++++++++----
>  1 file changed, 27 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> index 475f24f20..ea6c0800c 100644
> --- a/drivers/staging/greybus/usb.c
> +++ b/drivers/staging/greybus/usb.c
> @@ -105,8 +105,29 @@ static int hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
>  	size_t response_size;
>  	int ret;
>  
> -	/* FIXME: handle unspecified lengths */

So the story here we are using AI to try to address this FIXME...

We are still assigning:

	request->wLength = cpu_to_le16(wLength);

Where is that used?  This kind of gets back to the impact question.
In rh_call_control() we allocate the buffer with:

        /*
         * tbuf should be at least as big as the
         * USB hub descriptor.
         */
        tbuf_size = max_t(u16, sizeof(struct usb_hub_descriptor), wLength);
        tbuf = kzalloc(tbuf_size, mem_flags);

struct usb_hub_descriptor is larger than the other two sizes.  Here, now,
we have changed it to allocate a buffer which is potentially smaller
than request->wLength.  Is that an issue?  Greg probably knows the answer
off the top of his head again for this but I'm a USB newbie.

> -	if (wLength) {
> +	if (response_size > sizeof(*response)) {
> +		size_t data_size = response_size - sizeof(*response);
> +
>  		/* Greybus core has verified response size */

Where does this verification happen?

>  		response = operation->response->payload;
> -		memcpy(buf, response->buf, wLength);
> +		memcpy(buf, response->buf, data_size);
>  	}

regards,
dan carpenter

Re: [PATCH] greybus: usb: fix response buffer size for fixed-length hub requests

[Aditya Chari S]

Hi Dan,

Before I go through your questions point by point, I want to flag
something that affects the confidence of the whole patch.

My rationale for hardcoding fixed sizes per request type
(GetHubDescriptor/GetHubStatus/GetPortStatus) was based on reasoning
from rh_call_control() alone, without checking how the actual Project
Ara module-side firmware behaves. I went and found the firmware at
github.com/projectara/, and the relevant file at
github.com/projectara/nuttx/blob/master/nuttx/drivers/greybus/usb.c,
gb_usb_hub_control(). It doesn't support the assumption I was making.
The module allocates its own response buffer purely from wLength and
passes wLength straight through to the hardware call:

	response = gb_operation_alloc_response(operation,
						sizeof(*response) + wLength);
	...
	status = device_usb_hcd_hub_control(usbdev, typeReq, wValue,
					     wIndex, (char*) response->buf,
					     wLength);

So it's deferential to wLength on both ends. I don't see evidence here
that the module would ever send back more data than wLength implied,
which was the scenario I described as the "impact" of the original
FIXME.

Given that, and given this driver has been unreachable at runtime for
11 years per commit a96493560cd1, I'd rather get your read on whether
this is worth pursuing further before I go answer the rest of your
review and send a v2. Happy to keep digging if you think it's still
worthwhile, or fine leaving the FIXME as-is otherwise.

Thanks,
Aditya