diff mbox series

[4/6] media: rtl2832_sdr: Return queued buffers on start_streaming() failure

Message ID d10ab7f1bb6b4ee1760967f1957191a341b08354.1778518085.git.vebohr@gmail.com (mailing list archive)
State New
Headers
Series media: vb2: Return queued buffers from start_streaming() on error |

Commit Message

Valery Borovsky May 11, 2026, 5:12 p.m. UTC 
The vb2 framework hands buffers to the driver via buf_queue() before
calling start_streaming().  If start_streaming() returns an error
without first returning those buffers via vb2_buffer_done(),
vb2_start_streaming() fires WARN_ON(owned_by_drv_count) and the queued
buffers leak.

rtl2832_sdr_start_streaming() had multiple error paths that hit this
trap: two direct early returns (-ENODEV, -ERESTARTSYS), plus six
`goto err` paths covering subdev s_power, tuner setup, ADC setup,
stream-buffer allocation, urb allocation, and urb submission failures.
None of them returned the queued buffers.

The original function had no distinct success exit and fell straight
through into the err label, which previously only did mutex_unlock and
"return ret".  Adding queued-buffer cleanup at err must therefore be
paired with an explicit success return; otherwise every successful
start would also drain the buffer queue and kill streaming.  Add that
success return, then add rtl2832_sdr_cleanup_queued_bufs() at the err
label and before each early return.

The cleanup helper takes a vb2_buffer_state argument so that the
start_streaming error paths can pass VB2_BUF_STATE_QUEUED (as
expected by userspace on start_streaming failure) while stop_streaming
keeps its existing VB2_BUF_STATE_ERROR semantics.

This mirrors the uvcvideo fix in commit 4cf3b6fd54eb ("media: uvcvideo:
Return queued buffers on start_streaming() failure").

The err label still does not roll back power_ctrl(), frontend_ctrl(),
the POWER_ON flag, or stream/URB allocations that may have happened
before the failing step.  Those are pre-existing leaks of a different
class and are not addressed here.

Fixes: 771138920eaf ("[media] rtl2832_sdr: Realtek RTL2832 SDR driver module")
Cc: stable@vger.kernel.org
Signed-off-by: Valery Borovsky <vebohr@gmail.com>
---
 drivers/media/dvb-frontends/rtl2832_sdr.c | 19 ++++++++++++++-----
 1 file changed, 14 insertions(+), 5 deletions(-)

Comments

sashiko-bot@kernel.org May 13, 2026, 12:10 a.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Unbounded memory leak and Out-Of-Bounds read/crash due to unrolled allocations on start_streaming failure.
- [High] Silent DMA memory leak on device disconnect due to cleared `udev` pointer.
--

commit b46c390ec17dc3b72b2306b1cffbddd15df567b7
Author: Valery Borovsky <vebohr@gmail.com>

media: rtl2832_sdr: Return queued buffers on start_streaming() failure

This commit fixes a memory leak of queued vb2 buffers when
rtl2832_sdr_start_streaming() returns an error. It adds explicit buffer
cleanup using a new helper function on the error paths.

> diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
> index 422d1a7b5456e..c564485e3bbb5 100644
> --- a/drivers/media/dvb-frontends/rtl2832_sdr.c
> +++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
> @@ -855,11 +856,15 @@ static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
>  
>  	dev_dbg(&pdev->dev, "\n");
>  
> -	if (!dev->udev)
> +	if (!dev->udev) {
> +		rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
>  		return -ENODEV;
> +	}

[ ... ]

> @@ -900,7 +905,11 @@ static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
>  	if (ret)
>  		goto err;
>  
> +	mutex_unlock(&dev->v4l2_lock);
> +	return 0;
> +
>  err:
> +	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
>  	mutex_unlock(&dev->v4l2_lock);
>  
>  	return ret;

This isn't a bug introduced by this patch, but if rtl2832_sdr_submit_urbs()
fails and jumps to this error path, do we leak the newly allocated dma stream
buffers and urbs?

If userspace repeatedly attempts to start the stream via the streamon ioctl,
rtl2832_sdr_alloc_stream_bufs() unconditionally resets dev->buf_num = 0 and
overwrites dev->buf_list[], permanently leaking the dma memory.

Furthermore, rtl2832_sdr_alloc_urbs() does not reset dev->urbs_initialized;
it blindly increments it on each successful urb allocation. On a second
attempt, dev->urbs_initialized could exceed the array bounds. Later, when
rtl2832_sdr_free_urbs() is called, could it iterate from
dev->urbs_initialized - 1 down to 0, reading out-of-bounds past the
dev->urb_list[] array and passing garbage pointers to usb_free_urb(),
potentially causing a panic?

[ ... ]

> @@ -920,7 +929,7 @@ static void rtl2832_sdr_stop_streaming(struct vb2_queue *vq)
>  	rtl2832_sdr_kill_urbs(dev);
>  	rtl2832_sdr_free_urbs(dev);
>  	rtl2832_sdr_free_stream_bufs(dev);

This isn't a bug introduced by this patch, but if dev->udev is cleared to
null by the disconnect handler during device removal, does this cause a
silent dma memory leak?

When the user-space application eventually closes its file descriptor after a
device unplug, it triggers the vb2 release paths and calls
rtl2832_sdr_stop_streaming() which calls rtl2832_sdr_free_stream_bufs().

Because dev->udev is already null, usb_free_coherent(dev->udev, ...) will
immediately abort without freeing or unmapping the memory. Does this
guarantee that all dma stream buffers are permanently leaked every time the
sdr dongle is unplugged while streaming?

> -	rtl2832_sdr_cleanup_queued_bufs(dev);
> +	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_ERROR);
>  	rtl2832_sdr_unset_adc(dev);
>  
>  	/* sleep tuner */
diff mbox series

Patch

diff --git a/drivers/media/dvb-frontends/rtl2832_sdr.c b/drivers/media/dvb-frontends/rtl2832_sdr.c
index 422d1a7b5456..c564485e3bbb 100644
--- a/drivers/media/dvb-frontends/rtl2832_sdr.c
+++ b/drivers/media/dvb-frontends/rtl2832_sdr.c
@@ -399,7 +399,8 @@  static int rtl2832_sdr_alloc_urbs(struct rtl2832_sdr_dev *dev)
 }
 
 /* Must be called with vb_queue_lock hold */
-static void rtl2832_sdr_cleanup_queued_bufs(struct rtl2832_sdr_dev *dev)
+static void rtl2832_sdr_cleanup_queued_bufs(struct rtl2832_sdr_dev *dev,
+					    enum vb2_buffer_state state)
 {
 	struct platform_device *pdev = dev->pdev;
 	unsigned long flags;
@@ -413,7 +414,7 @@  static void rtl2832_sdr_cleanup_queued_bufs(struct rtl2832_sdr_dev *dev)
 		buf = list_entry(dev->queued_bufs.next,
 				struct rtl2832_sdr_frame_buf, list);
 		list_del(&buf->list);
-		vb2_buffer_done(&buf->vb.vb2_buf, VB2_BUF_STATE_ERROR);
+		vb2_buffer_done(&buf->vb.vb2_buf, state);
 	}
 	spin_unlock_irqrestore(&dev->queued_bufs_lock, flags);
 }
@@ -855,11 +856,15 @@  static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
 
 	dev_dbg(&pdev->dev, "\n");
 
-	if (!dev->udev)
+	if (!dev->udev) {
+		rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 		return -ENODEV;
+	}
 
-	if (mutex_lock_interruptible(&dev->v4l2_lock))
+	if (mutex_lock_interruptible(&dev->v4l2_lock)) {
+		rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 		return -ERESTARTSYS;
+	}
 
 	if (d->props->power_ctrl)
 		d->props->power_ctrl(d, 1);
@@ -900,7 +905,11 @@  static int rtl2832_sdr_start_streaming(struct vb2_queue *vq, unsigned int count)
 	if (ret)
 		goto err;
 
+	mutex_unlock(&dev->v4l2_lock);
+	return 0;
+
 err:
+	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_QUEUED);
 	mutex_unlock(&dev->v4l2_lock);
 
 	return ret;
@@ -920,7 +929,7 @@  static void rtl2832_sdr_stop_streaming(struct vb2_queue *vq)
 	rtl2832_sdr_kill_urbs(dev);
 	rtl2832_sdr_free_urbs(dev);
 	rtl2832_sdr_free_stream_bufs(dev);
-	rtl2832_sdr_cleanup_queued_bufs(dev);
+	rtl2832_sdr_cleanup_queued_bufs(dev, VB2_BUF_STATE_ERROR);
 	rtl2832_sdr_unset_adc(dev);
 
 	/* sleep tuner */