[PATCH v2] media: cx231xx: Reject audio buffers smaller than two packets

[PATCH v2] media: cx231xx: Reject audio buffers smaller than two packets

[Sinan Turunc]

cx231xx_audio_isocirq() and cx231xx_audio_bulkirq() split the copy into
the ALSA ring buffer once at the buffer end and wrap hwptr_done_capture
once, which is only correct when one packet holds at most buffer_size
frames. The packet size is max_pkt_size from the device's alternate
setting, while the hw constraints allow 64-byte periods with periods_min
2, so a packet can be larger than the whole buffer.

snd_cx231xx_capture_open() selects the alternate setting before
hw_params, which sets max_pkt_size, so constrain
SNDRV_PCM_HW_PARAM_BUFFER_BYTES there to at least twice max_pkt_size.
Two packets, matching the line6 fix. The bound makes the copy safe, but
it does not make every larger buffer progress: the position is reported
once per URB of 64 packets, so a buffer that divides one URB's data
returns to the same position at every callback. That was already the
case before this patch and is not changed here. Keep a guard in both
callbacks that drops an oversized packet with a ratelimited error, for
the case that should no longer be reachable.

Reproduced on 7.3-rc7 with a USB/IP device presenting a Pixelview
Xcapture interface layout and 1024-byte packets into a 128-byte buffer:

  BUG: KASAN: use-after-free in cx231xx_audio_isocirq+0x2c7/0x780
  Read of size 4294966528 at addr ffff88800e8e0400 by task vhci_rx/82

With this patch a 1024-byte buffer is refused at hw_params; a 2048-byte
buffer is accepted and safe but barely advances (hw_ptr 14336 after 30
s, for the reason above); a 3072-byte buffer streams for 30 s with no
report and with the position moving inside the buffer.

Fixes: e0d3bafd0258 ("V4L/DVB (10954): Add cx231xx USB driver")
Cc: stable@vger.kernel.org
Signed-off-by: Sinan Turunc <enscrpt@pm.me>
---
v2: constrain the buffer at open instead of clamping in the copy. v1
clamped the frame count to buffer_size, which left the ring position
unchanged on every oversized packet and stalled the stream, as the
Sashiko review of the line6 patch pointed out. The bound is two
packets, matching the line6 fix; see the message for what it does and
does not change. The clamp is replaced by a drop guard for the case the
constraint makes unreachable.

 drivers/media/usb/cx231xx/cx231xx-audio.c | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-audio.c b/drivers/media/usb/cx231xx/cx231xx-audio.c
index b24ceef497e..14df45ab011 100644
--- a/drivers/media/usb/cx231xx/cx231xx-audio.c
+++ b/drivers/media/usb/cx231xx/cx231xx-audio.c
@@ -123,6 +123,19 @@ static void cx231xx_audio_isocirq(struct urb *urb)
 			if (!length)
 				continue;
 
+			/*
+			 * snd_cx231xx_capture_open() constrains the buffer to
+			 * hold at least two packets, so this cannot happen for
+			 * a conforming device. Drop the packet rather than
+			 * overrun the single wrap below.
+			 */
+			if (unlikely(length > runtime->buffer_size)) {
+				dev_err_ratelimited(dev->dev,
+						    "audio packet of %d frames exceeds %lu-frame buffer, dropping\n",
+						    length, runtime->buffer_size);
+				continue;
+			}
+
 			oldptr = dev->adev.hwptr_done_capture;
 			if (oldptr + length >= runtime->buffer_size) {
 				unsigned int cnt;
@@ -204,7 +217,7 @@ static void cx231xx_audio_bulkirq(struct urb *urb)
 		runtime = substream->runtime;
 		stride = runtime->frame_bits >> 3;
 
-		if (1) {
+		if (urb->actual_length / stride <= runtime->buffer_size) {
 			unsigned long flags;
 			int length = urb->actual_length /
 				     stride;
@@ -424,6 +437,18 @@ static int snd_cx231xx_capture_open(struct snd_pcm_substream *substream)
 
 	runtime->hw = snd_cx231xx_hw_capture;
 
+	/*
+	 * The URB callbacks wrap the ring buffer once per packet and advance
+	 * the position by the packet, which only works when the buffer holds
+	 * more than one packet. max_pkt_size was set by the alternate setting
+	 * above, so bound the buffer by two packets here.
+	 */
+	ret = snd_pcm_hw_constraint_minmax(runtime,
+					   SNDRV_PCM_HW_PARAM_BUFFER_BYTES,
+					   2 * dev->adev.max_pkt_size, UINT_MAX);
+	if (ret < 0)
+		return ret;
+
 	mutex_lock(&dev->lock);
 	/* inform hardware to start streaming */
 	ret = cx231xx_capture_start(dev, 1, Audio);
-- 
2.56.0