Re: [PATCH v3 2/2] ALSA: usx2y: Guard the playback copy

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ALSA: usx2y: incomplete packet length validation in usx2y_urb_play_prepare()
--

commit 8f4cd6a52f3d49a81dfc6755062eeb7276e1ee4b
Author: Sinan Turunc <enscrpt@pm.me>

ALSA: usx2y: Guard the playback copy

This commit adds bounds checks in usx2y_urb_play_prepare() to verify that each
packet fits within the playback endpoint's maxpacksize and that the total URB
frame count does not exceed the ring buffer size, returning -EPIPE on a
violation.

> diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
> index ee224b4dfb046..5f241d454012f 100644
> --- a/sound/usb/usx2y/usbusx2yaudio.c
> +++ b/sound/usb/usx2y/usbusx2yaudio.c
> @@ -112,7 +112,8 @@ static int usx2y_urb_play_prepare(struct snd_usx2y_substream *subs,
>  		/* calculate the size of a packet */
>  		counts = cap_urb->iso_frame_desc[pack].actual_length / usx2y->stride;
>  		count += counts;
> -		if (counts < 43 || counts > 50) {
> +		if (counts < 43 || counts > 50 ||
> +		    counts * usx2y->stride > subs->maxpacksize) {
>  			dev_err(&usx2y->dev->dev, "%s: should not be here with counts=%i\n",
>  				__func__, counts);
>  			return -EPIPE;
> @@ -125,6 +126,16 @@ static int usx2y_urb_play_prepare(struct snd_usx2y_substream *subs,
>  		urb->iso_frame_desc[pack].length = cap_urb->iso_frame_desc[pack].actual_length;

[Severity: High]
What happens in usx2y_urb_play_prepare() in sound/usb/usx2y/usbusx2yaudio.c
when i_usx2y_urb_complete() -> usx2y_usbframe_complete() ->
usx2y_urb_play_prepare() -> usx2y_urb_submit() processes a capture URB whose
cap_urb->iso_frame_desc[pack].actual_length is not a multiple of
usx2y->stride?

Because counts is computed via truncating integer division:

	counts = cap_urb->iso_frame_desc[pack].actual_length / usx2y->stride;

any remainder actual_length % usx2y->stride is discarded from counts, and the
new guard only checks the truncated byte count counts * usx2y->stride against
subs->maxpacksize. However, the playback ISO descriptor offsets and lengths
are still populated from the untruncated actual_length:

	urb->iso_frame_desc[pack].offset = pack ?
		urb->iso_frame_desc[pack - 1].offset +
		urb->iso_frame_desc[pack - 1].length :
		0;
	urb->iso_frame_desc[pack].length = cap_urb->iso_frame_desc[pack].actual_length;

Meanwhile, the ring-buffer wrap check, the memcpy() into subs->tmpbuf, the
non-wrapping pointer slice into runtime->dma_area, and
urb->transfer_buffer_length all account for only count * usx2y->stride bytes:

		} else {
			/* set the buffer pointer */
			urb->transfer_buffer = runtime->dma_area + subs->hwptr * usx2y->stride;
			subs->hwptr += count;
			if (subs->hwptr >= runtime->buffer_size)
				subs->hwptr -= runtime->buffer_size;
		}
	} else {
		urb->transfer_buffer = subs->tmpbuf;
	}
	urb->transfer_buffer_length = count * usx2y->stride;

If actual_length has a non-zero remainder modulo usx2y->stride, can this still
read past the end of subs->tmpbuf or runtime->dma_area? For example:

1. When counts * usx2y->stride == subs->maxpacksize and actual_length has
   trailing remainder bytes (such as stride = 4, subs->maxpacksize = 200, and
   actual_length = 203, giving counts = 50), counts * usx2y->stride >
   subs->maxpacksize does not trigger, so urb->iso_frame_desc[pack].length
   (203) exceeds subs->maxpacksize (200) and the cumulative ISO descriptors
   across nr_of_packs() packets exceed the subs->tmpbuf allocation of
   nr_of_packs() * subs->maxpacksize.

2. Even when actual_length <= subs->maxpacksize (such as stride = 4,
   subs->maxpacksize = 200, and actual_length = 195, giving counts = 48 and
   counts * stride = 192), the ISO descriptors in urb->iso_frame_desc[] span
   more bytes than urb->transfer_buffer_length. When subs->hwptr + count ==
   runtime->buffer_size in the non-wrapping branch, only count * usx2y->stride
   bytes remain before the end of runtime->dma_area, so the ISO descriptors
   extend past the end of runtime->dma_area.

Should usx2y_urb_play_prepare() either size urb->iso_frame_desc[pack].length
as counts * usx2y->stride or reject packets where actual_length is not aligned
to usx2y->stride?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104105228.1124-1-enscrpt@pm.me?part=2