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@xxxxxxxxx>
> ---
>  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