[PATCH v3 0/2] ALSA: usx2y: Bound the PCM buffer by the URB size

[PATCH v3 0/2] ALSA: usx2y: Bound the PCM buffer by the URB size

[Sinan Turunc]

v3 of the usx2y buffer fix, split in two after the review of v2.

Patch 1 replaces the two-packet BUFFER_BYTES bound of v2 with a
two-URB BUFFER_SIZE bound in frames that does not read the endpoint,
and keeps the capture drop guard. Patch 2 adds guards in the playback
prepare path for a URB larger than the buffer and a packet larger than
the playback endpoint's maxpacksize, both pre-existing and unchecked.

Tested on 7.3-rc7 with a USB/IP device presenting the US-122 interface
layout. The playback case of patch 2 reproduces on the unpatched driver
with ordinary 48-frame packets and a 100-frame buffer, which the stock
constraints allow. With both patches, buffers below the bound are
refused at hw_params and 400-frame buffers stream in both directions
with no KASAN report, for S16_LE and, on capture, S24_3LE. The results
are in the patch messages.

Sinan Turunc (2):
  ALSA: usx2y: Require a buffer of two URBs
  ALSA: usx2y: Guard the playback copy

 sound/usb/usx2y/usbusx2yaudio.c | 34 ++++++++++++++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

[PATCH v3 1/2] ALSA: usx2y: Require a buffer of two URBs

[Sinan Turunc]

usx2y_urb_capt_retire() wraps the ring buffer once per packet and
usx2y_urb_play_prepare() once per URB of nr_of_packs() packets of up to
50 frames. Both assume that the data fits in the buffer, and the hw
constraints only require about two 1 ms periods, so a packet larger
than the buffer on capture, or a URB larger than the buffer on playback,
violates that assumption.

Constrain SNDRV_PCM_HW_PARAM_BUFFER_SIZE in snd_usx2y_pcm_open() to at
least two URBs, 2 * nr_of_packs() * 50 frames, for both directions. The
bound is in frames and derived from the driver's own packet limit, so it
does not read the endpoint, whose altsetting is only selected in
prepare. Two URBs rather than one, because the position advances by a
URB on playback and only moves when a URB is less than the whole buffer.
Keep a guard in usx2y_urb_capt_retire() that drops a packet larger than
the buffer, for the case the constraint makes unreachable.

With nrpacks above about 655 (S16_LE) or 436 (S24_3LE) the bound exceeds
buffer_bytes_max, so hw_params fails for those module settings. That is
intended: the driver cannot be safe with a URB larger than half the
largest buffer.

Reproduced on 7.3-rc7 with a USB/IP device presenting the US-122
interface layout and 1024-byte packets into a 512-byte capture buffer:

  BUG: KASAN: out-of-bounds in i_usx2y_urb_complete+0x1285/0x1ac0
  Read of size 18446744073709551104 at addr ffff88800e448800 by task vhci_rx/83

Tested on 7.3-rc7 with the same device, S16_LE at 44100 Hz, nrpacks 4,
so the bound is 400 frames. A 128-frame capture buffer and a 200-frame
playback buffer are refused at hw_params with EINVAL. A 400-frame buffer
streams for 30 s in each direction with the position advancing inside
the buffer and no KASAN report; the capture guard did not fire. The
same 400-frame capture buffer with S24_3LE, which selects altsetting 2
in prepare, streams for 30 s with the same result.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Sinan Turunc <enscrpt@pm.me>
---
v3: bound the buffer in frames by two URBs instead of two packets, so
the playback copy is covered and the bound no longer reads the endpoint
before the altsetting is selected (both from the review of v2). The
open() comment that described playback as wrapping per packet is
corrected. Both directions tested on 7.3-rc7, see above.
v2: constrain the buffer at open instead of clamping in the copy.

 sound/usb/usx2y/usbusx2yaudio.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index a3a0bc13632..ee224b4dfb0 100644
--- a/sound/usb/usx2y/usbusx2yaudio.c
+++ b/sound/usb/usx2y/usbusx2yaudio.c
@@ -51,6 +51,17 @@ static int usx2y_urb_capt_retire(struct snd_usx2y_substream *subs)
 			dev_dbg(&usx2y->dev->dev, "%s: 0 == len ERROR!\n", __func__);
 			continue;
 		}
+		/*
+		 * snd_usx2y_pcm_open() constrains the buffer to hold at least
+		 * two URBs, so this cannot happen for a conforming device.
+		 * Drop the packet rather than exceed the single wrap below.
+		 */
+		if (unlikely(len > runtime->buffer_size)) {
+			dev_err_ratelimited(&usx2y->dev->dev,
+					    "%s: packet of %i frames exceeds %lu-frame buffer, dropping\n",
+					    __func__, len, runtime->buffer_size);
+			continue;
+		}
 
 		/* copy a data chunk */
 		if ((hwptr_done + len) > runtime->buffer_size) {
@@ -888,6 +899,16 @@ static int snd_usx2y_pcm_open(struct snd_pcm_substream *substream)
 	runtime->private_data = subs;
 	subs->pcm_substream = substream;
 	snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_PERIOD_TIME, 1000, 200000);
+	/*
+	 * Capture wraps the ring buffer once per packet and playback once
+	 * per URB of nr_of_packs() packets of at most 50 frames (see
+	 * usx2y_urb_play_prepare()). Both need the buffer to hold more than
+	 * one URB, and the position only advances when a URB is less than
+	 * the whole buffer, so require two URBs. The bound is in frames and
+	 * does not depend on the altsetting, which is selected in prepare.
+	 */
+	snd_pcm_hw_constraint_minmax(runtime, SNDRV_PCM_HW_PARAM_BUFFER_SIZE,
+				     2 * nr_of_packs() * 50, UINT_MAX);
 	return 0;
 }
 
-- 
2.56.0

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

[Sinan Turunc]

usx2y_urb_play_prepare() sums the packet lengths of a whole URB, copies
that many frames with a single wrap, and sizes each playback packet from
the capture packet's actual_length. Two assumptions are unchecked: that
the URB fits in the ring buffer, and that a packet fits in the playback
endpoint's maxpacksize, from which tmpbuf is allocated.

Return -EPIPE, as the existing packet-size check does, when the URB is
larger than the buffer or a packet is larger than maxpacksize. The first
case is prevented by the buffer bound in the previous patch and this is
the guard for it; the second depends only on the device.

The first case is reachable on the unpatched driver with ordinary
packets: on 7.3-rc7, a USB/IP device presenting the US-122 interface
layout and sending 48-frame packets, with a 100-frame playback buffer
(two 50-frame periods, inside the stock constraints), KASAN reports:

  BUG: KASAN: out-of-bounds in i_usx2y_urb_complete+0x79b/0x1ac0
  Read of size 18446744073709551280 at addr ffff88800e6b02e0 by task vhci_rx/80

With both patches, the same device and a 200-frame playback buffer is
refused at hw_params with EINVAL, and a 400-frame buffer plays for 30 s
with the position advancing inside the buffer, no KASAN report and
neither guard message. The maxpacksize check is not exercised: the
emulated device reports the same wMaxPacketSize on both endpoints.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Sinan Turunc <enscrpt@pm.me>
---
v3: new patch, split out of the v3 draft; the review of v2 found both
unchecked assumptions. The buffer case is confirmed on the unpatched
driver and the guards tested on 7.3-rc7, see above.

 sound/usb/usx2y/usbusx2yaudio.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/sound/usb/usx2y/usbusx2yaudio.c b/sound/usb/usx2y/usbusx2yaudio.c
index ee224b4dfb0..5f241d45401 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;
 	}
 	if (atomic_read(&subs->state) >= STATE_PRERUNNING) {
+		/*
+		 * The copy below wraps once, which needs count <= buffer_size.
+		 * snd_usx2y_pcm_open() guarantees it; guard anyway.
+		 */
+		if (unlikely(count > runtime->buffer_size)) {
+			dev_err_ratelimited(&usx2y->dev->dev,
+					    "%s: URB of %i frames exceeds %lu-frame buffer\n",
+					    __func__, count, runtime->buffer_size);
+			return -EPIPE;
+		}
 		if (subs->hwptr + count > runtime->buffer_size) {
 			/* err, the transferred area goes over buffer boundary.
 			 * copy the data to the temp buffer.
-- 
2.56.0

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

[sashiko-bot]

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

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

[Sinan Turunc]

> 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?

Example 1 (a descriptor of 203 bytes on a 200-byte endpoint) does not
reach the host controller: usb_submit_urb() compares every ISO
descriptor length with the endpoint maximum and returns -EMSGSIZE
(drivers/usb/core/urb.c, the number_of_packets loop), and for the
full-speed devices this driver serves that maximum is maxpacksize. The
driver then stops the streams on the submit error.

Example 2 is valid and pre-existing. The descriptors are filled from the
raw actual_length while transfer_buffer_length and both copies use
count * stride, so a capture packet that is not a whole number of frames
makes the descriptors add up to more than the buffer, by at most
stride - 1 bytes per packet. The excess is read by the host controller
and sent to the device; the driver itself copies only whole frames.

v4 sizes each descriptor as counts * stride, so the descriptors add up
to transfer_buffer_length and, with the maxpacksize check, each stays
within the endpoint maximum, which also removes example 1 without
relying on the core check.

I went through every value in this file's data paths that the device
controls (packet status, capture and playback actual_length, start_frame
and wMaxPacketSize) and where each is bounded. The descriptor length
was the only one left unchecked after v3. With an emulated device
sending 195-byte capture packets, the v3 descriptors add up to 780
bytes for 768 bytes of data and USB/IP's completion check refuses the
URB; with v4 every descriptor is 192 bytes and the stream runs.

Thanks,
Sinan