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

[sashiko-bot@kernel.org]

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 */

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1778518085.git.vebohr@gmail.com?part=4