Re: [BUG]: drivers: media: dvb-frontends: rtl2832_sdr.c: a dangling pointer may cause double free

[Sean Young <sean@mess.org>]

Hi,

On Sun, Aug 01, 2021 at 03:08:16PM +0800, nil Yi wrote:
> Hi,
> I found there is a dangling pointer in  rtl2832_sdr_alloc_urbs which
> may cause double free in v5.14-rc3
> 
> in rtl2832_sdr_alloc_urbs:
> 
> 379: for (j = 0; j < i; j++)
> 380:    usb_free_urb(dev->urb_list[j]);
> it frees all the urbs but forgets to set the dev->urbs_initialized to
> zero, which will be used in function  rtl2832_sdr_free_urbs:
> 
> 357: for (i = dev->urbs_initialized - 1; i >= 0; i--) {
> 358:     if (dev->urb_list[i]) {
> 359:          dev_dbg(&pdev->dev, "free urb=%d\n", i);
> 360:                /* free the URBs */
> 361:              usb_free_urb(dev->urb_list[i]);
> 362: }
> 363: }
> 364:    dev->urbs_initialized = 0;
> 
> 
> I'm not sure whether this double free would be triggered or not,
> similar issue happened in commit b7f870510384 <media: tm6000: double
> free if usb disconnect while streaming>
> 
> Any feedback would be appreciated, thanks :)

I am not quite sure how this bug could be triggered, since this would involve
rtl2832_sdr_start_streaming() being called and then failing, and then
rtl2832_sdr_stop_streaming() being called even though start failed (I don't
think that happens).

Note that the code around this is pretty ugly; it might be nicer to remove
urbs_initialized and simply rely on the fact the the urb_list[i] entry
is non-null if it needs to be freed. That would be a great patch. :-)

For discussion it is best if you post a patch and then we can discuss the
patch itself rather than "is there a possible way this could go wrong".


Sean