Re: [PATCH v4 2/3] usb: gadget: composite: Support eUSB2v2 bulk MPS update

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] usb: composite: Data race on bitfields in struct usb_gadget
- [High] usb: composite: eUSB2v2 capability check breaks SuperSpeed device descriptors
- [Medium] usb: composite: Missing capability check before accepting eUSB2v2 feature
- [Medium] usb: composite: Missing fallback for unhandled SET_FEATURE requests breaks delegation
--

commit 6017c856bde8d3fefac3ad153909a02705baae1f
Author: Pawel Laszczak <pawell@cadence.com>

usb: gadget: composite: Support eUSB2v2 bulk MPS update

This commit adds support for eUSB2v2 1024-byte Bulk MPS negotiation to the
Gadget Composite framework.

> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
[ ... ]
> @@ -1807,7 +1808,11 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  				count_configs(cdev, USB_DT_DEVICE);
>  			cdev->desc.bMaxPacketSize0 =
>  				cdev->gadget->ep0->maxpacket;
> -			if (gadget_is_superspeed(gadget)) {
> +
> +			if (gadget->is_eusb2v2) {
> +				cdev->desc.bcdUSB = cpu_to_le16(0x0230);
> +				cdev->desc.bMaxPacketSize0 = 64;
> +			} else if (gadget_is_superspeed(gadget)) {

[Severity: High]
Does this static capability check unconditionally override the descriptor
fields and ignore the current negotiated link speed?

is_eusb2v2 is a static capability flag set by dual-speed UDC drivers at
probe time. If a dual-speed device is plugged into a USB 3.0 port and
enumerates at SuperSpeed:

composite_setup()
    switch (w_value >> 8) {
    case USB_DT_DEVICE:
        if (gadget->is_eusb2v2) {
            ...
        } else if (gadget_is_superspeed(gadget)) {
            ...
        }

The code will incorrectly report bMaxPacketSize0 = 64 instead of the
strictly required value of 9 (representing 2^9 = 512 bytes for SuperSpeed).
Can this break USB 3.x host enumeration for any UDC supporting eUSB2v2?

[ ... ]
> @@ -2006,12 +2011,20 @@ composite_setup(struct usb_gadget *gadget, const struct usb_ctrlrequest *ctrl)
>  	 */
>  	case USB_REQ_CLEAR_FEATURE:
>  	case USB_REQ_SET_FEATURE:
> -		if (!gadget_is_superspeed(gadget))
> -			goto unknown;
> -		if (ctrl->bRequestType != (USB_DIR_OUT | USB_RECIP_INTERFACE))
> -			goto unknown;
>  		switch (w_value) {

[Severity: Medium]
Does this switch block miss a fallback for unhandled SET_FEATURE requests?

Standard requests with w_value that are not explicitly handled in the
switch (for example USB_DEVICE_REMOTE_WAKEUP, USB_DEVICE_TEST_MODE, or
USB_DEVICE_U1_ENABLE) will fall through the switch, hit the outer break,
and return -EOPNOTSUPP directly.

Could this bypass the goto unknown delegation block entirely, breaking
userspace gadget drivers like FunctionFS that rely on intercepting
device-level feature requests by setting the FUNCTIONFS_ALL_CTRL_RECIP flag?

> +		case USB_DEVICE_BULK_MAX_PACKET_UPDATE:
> +			if (ctrl->bRequestType != (USB_DIR_OUT | USB_RECIP_DEVICE))
> +				goto unknown;
> +			if (ctrl->bRequest != USB_REQ_SET_FEATURE)
> +				goto unknown;

[Severity: Medium]
Is a capability check missing before accepting the eUSB2v2 feature?

If a host incorrectly sends this eUSB2-specific feature to a standard
USB 2.0 or 3.0 gadget, the gadget will inappropriately ACK the feature
here instead of STALLing it, because there is no verification of
if (gadget->is_eusb2v2).

Can this cause the host to incorrectly assume 1024-byte bulk packets are
supported and transmit them, leading to hardware babble errors or DMA
overflows on standard UDCs configured for 512-byte max packet sizes?

> +			gadget->eusb2v2_mps_active = 1;
> +			value = 0;
> +			break;

[Severity: High]
Does this bitfield write in interrupt context introduce a data race?

composite_setup() runs in the UDC endpoint 0 interrupt context, and
gadget->eusb2v2_mps_active = 1 is written here without taking a lock that
serializes with process-context writers.

In include/linux/usb/gadget.h, eusb2v2_mps_active shares a bitfield word
with other flags like connected and deactivated:

struct usb_gadget {
    ...
    unsigned connected:1;
    ...
    unsigned is_eusb2v2:1;
    unsigned eusb2v2_mps_active:1;
};

Those other fields are written in process context (for example, in
usb_gadget_connect()) while holding the connect_lock mutex, which does
not disable interrupts.

Since bitfield updates compile down to non-atomic Read-Modify-Write (RMW)
operations on the full word, an interrupt occurring during a process-context
RMW will cause the interrupt's bitfield write to be silently overwritten and
lost, or vice versa. Can this randomly drop the gadget's fundamental
connection state?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261009-eusb2v2-packet-size-v4-0-a505ee17b460@cadence.com?part=2