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);
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
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
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
-
Aditya Chari S -
Dan Carpenter -
Greg Kroah-Hartman