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