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
