Re: [PATCH v3 2/2] usb: gadget: gadgetfs: prevent ep0 event counter underflow

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] usb: gadget: gadgetfs: unlocked dev->event copy and event
  loss in ep0_read()
--

commit 7de7074f8592544c9b5457f297c92470cf9e3ac3
Author: Jing Lin <kayano050408@gmail.com>

usb: gadget: gadgetfs: prevent ep0 event counter underflow

This commit adds an event generation counter and clamps the subtraction on
dev->ev_next in ep0_read() to prevent an unsigned counter underflow when
the event queue changes during copy_to_user().

> diff --git a/drivers/usb/gadget/legacy/inode.c b/drivers/usb/gadget/legacy/inode.c
> index 87aff568aa596..6db3b7879c94d 100644
> --- a/drivers/usb/gadget/legacy/inode.c
> +++ b/drivers/usb/gadget/legacy/inode.c
[ ... ]
> @@ -1174,7 +1175,7 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  scan:
>  	/* return queued events right away */
>  	if (dev->ev_next != 0) {
> -		unsigned		i, n;
> +		unsigned int		i, n, generation;
>  
>  		n = len / sizeof (struct usb_gadgetfs_event);
>  		if (dev->ev_next < n)
> @@ -1188,6 +1189,7 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  				break;
>  			}
>  		}
> +		generation = dev->ev_generation;
>  		spin_unlock_irq (&dev->lock);
>  		len = n * sizeof (struct usb_gadgetfs_event);
>  		if (copy_to_user (buf, &dev->event, len))
> @@ -1199,12 +1201,16 @@ ep0_read (struct file *fd, char __user *buf, size_t len, loff_t *ptr)
>  			 * concurrent ep0 readers may lose events.
>  			 */
>  			spin_lock_irq (&dev->lock);
> -			if (dev->ev_next > n) {
> -				memmove(&dev->event[0], &dev->event[n],
> -					sizeof (struct usb_gadgetfs_event)
> -						* (dev->ev_next - n));
> +			if (dev->ev_generation == generation) {
> +				if (dev->ev_next > n) {
> +					memmove(&dev->event[0], &dev->event[n],
> +						sizeof(struct usb_gadgetfs_event)
> +							* (dev->ev_next - n));
> +					dev->ev_next -= n;
> +				} else {
> +					dev->ev_next = 0;
> +				}
>  			}
> -			dev->ev_next -= n;
>  			spin_unlock_irq (&dev->lock);
>  		}
>  		return retval;
> @@ -1252,6 +1258,7 @@ next_event (struct dev_data *dev, enum usb_gadgetfs_event_type type)
>  		fallthrough;
>  	case GADGETFS_CONNECT:
>  		dev->ev_next = 0;
> +		dev->ev_generation++;
>  		break;
>  	case GADGETFS_SETUP:		/* previous request timed out */
>  	case GADGETFS_SUSPEND:		/* same effect */

[Severity: Medium]
Can ep0_read() still drop a newly arrived GADGETFS_SETUP or
GADGETFS_SUSPEND event or race on dev->event[] during copy_to_user()?

next_event() only increments dev->ev_generation on GADGETFS_DISCONNECT and
GADGETFS_CONNECT.  For GADGETFS_SETUP and GADGETFS_SUSPEND, it deduplicates
existing events of the same type in place without bumping
dev->ev_generation:

drivers/usb/gadget/legacy/inode.c:next_event() {
	...
	case GADGETFS_SETUP:		/* previous request timed out */
	case GADGETFS_SUSPEND:		/* same effect */
		/* these events can't be repeated */
		for (i = 0; i != dev->ev_next; i++) {
			if (dev->event [i].type != type)
				continue;
			DBG(dev, "discard old event[%d] %d\n", i, type);
			dev->ev_next--;
	...
}

Because ep0_read() leaves the n events in dev->event[] until after
copy_to_user() returns, a new GADGETFS_SETUP arriving during that window
deduplicates against the old entry still at dev->event[0]:

CPU0                                  CPU1
ep0_read()
  spin_lock_irq(&dev->lock)
  /* event[0] is GADGETFS_SETUP, n = 1 */
  generation = dev->ev_generation
  spin_unlock_irq(&dev->lock)
  copy_to_user(buf, &dev->event, len)
                                      gadgetfs_setup()
                                        spin_lock(&dev->lock)
                                        next_event(dev, GADGETFS_SETUP)
                                          /* discards old event[0] */
                                          /* writes new SETUP at event[0] */
                                          /* dev->ev_next is still 1 */
                                        spin_unlock(&dev->lock)
  spin_lock_irq(&dev->lock)
  if (dev->ev_generation == generation)
    dev->ev_next = 0;  <-- drops the new GADGETFS_SETUP

Dropping the new GADGETFS_SETUP event leaves ep0 control transfers wedged
when gadgetfs_setup() returns USB_GADGET_DELAYED_STATUS and leaves
dev->state out of sync with the queue.  Also, because copy_to_user() reads
directly from dev->event[] without holding dev->lock, concurrent
next_event() calls can overwrite or memmove() entries while they are being
copied to userspace, or a GADGETFS_CONNECT/DISCONNECT purge can write a new
event into dev->event[0] that gets copied by the current read and then left
queued for a duplicate delivery because dev->ev_generation changed.

Dequeuing the n events into a local buffer (N_EVENT * sizeof(struct
usb_gadgetfs_event) is 60 bytes, which fits in tmp[RBUF_SIZE]) and updating
dev->ev_next before dropping dev->lock for copy_to_user() would avoid the
underflow, the unlocked read of dev->event[], and the lost events.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107-b4-gadgetfs-ep0-v3-1-62089dc0ec07@gmail.com?part=2