Re: [PATCH v2] media: usb: hackrf: fix device unregister order in hackrf_probe()

[Nikita Zhandarovich]

Hi, please disregard my patch for following reasons:

1) The issue at hand isn't addressed properly - looking back I haven't
properly identified root cause and my fix is flawed.
2) Even if the approach is correct, I messed up the move of the label
err_v4l2_device_unregister. If video_register_device(&dev->rx_vdev,...)
fails we skip necessary deregistering steps.

I'll attempt to come up with a better solution, thank you for your
patience.

Regards,
Nikita

On 2/13/25 21:26, Nikita Zhandarovich wrote:
> Syzkaller reports [1] a slab-use-after-free error identified by KASAN
> sanitizer and most likely caused by the wrong order of device
> unregister steps once a call to video_register_device() fails.
> 
> Fix aforementioned flaw by first freeing control and reference
> handlers, only then dealing with dev->v4l2_dev() via
> v4l2_device_unregister().
> 
> [1] Syzkaller (partial) report:
> BUG: KASAN: slab-use-after-free in v4l2_release+0x3e2/0x460 drivers/media/v4l2-core/v4l2-dev.c:453
> Read of size 8 at addr ffff8880502e80c8 by task v4l_id/7854
> ...
> Call Trace:
>  <TASK>
>  __dump_stack lib/dump_stack.c:94 [inline]
>  dump_stack_lvl+0x116/0x1f0 lib/dump_stack.c:120
>  print_address_description mm/kasan/report.c:377 [inline]
>  print_report+0xc3/0x620 mm/kasan/report.c:488
>  kasan_report+0xd9/0x110 mm/kasan/report.c:601
>  v4l2_release+0x3e2/0x460 drivers/media/v4l2-core/v4l2-dev.c:453
>  __fput+0x3f6/0xb60 fs/file_table.c:431
>  __fput_sync+0x45/0x50 fs/file_table.c:516
>  __do_sys_close fs/open.c:1567 [inline]
>  __se_sys_close fs/open.c:1552 [inline]
>  __x64_sys_close+0x86/0x100 fs/open.c:1552
>  do_syscall_x64 arch/x86/entry/common.c:52 [inline]
>  do_syscall_64+0xcd/0x250 arch/x86/entry/common.c:83
>  entry_SYSCALL_64_after_hwframe+0x77/0x7f
> ...
> 
> Allocated by task 6058:
>  kasan_save_stack+0x33/0x60 mm/kasan/common.c:47
>  kasan_save_track+0x14/0x30 mm/kasan/common.c:68
>  poison_kmalloc_redzone mm/kasan/common.c:377 [inline]
>  __kasan_kmalloc+0xaa/0xb0 mm/kasan/common.c:394
>  kmalloc_noprof include/linux/slab.h:878 [inline]
>  kzalloc_noprof include/linux/slab.h:1014 [inline]
>  hackrf_probe+0xd1/0x1cf0 drivers/media/usb/hackrf/hackrf.c:1353
>  usb_probe_interface+0x309/0x9d0 drivers/usb/core/driver.c:399
> ...
> 
> Freed by task 6058:
>  kasan_save_stack+0x33/0x60 mm/kasan/common.c:47
>  kasan_save_track+0x14/0x30 mm/kasan/common.c:68
>  kasan_save_free_info+0x3b/0x60 mm/kasan/generic.c:579
>  poison_slab_object mm/kasan/common.c:247 [inline]
>  __kasan_slab_free+0x51/0x70 mm/kasan/common.c:264
>  kasan_slab_free include/linux/kasan.h:230 [inline]
>  slab_free_hook mm/slub.c:2342 [inline]
>  slab_free mm/slub.c:4579 [inline]
>  kfree+0x14f/0x4b0 mm/slub.c:4727
>  hackrf_probe+0x4c9/0x1cf0 drivers/media/usb/hackrf/hackrf.c:1525
>  usb_probe_interface+0x309/0x9d0 drivers/usb/core/driver.c:399
> ...
> 
> Reported-by: syzbot+6b52c2b24e341804a58c@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=6b52c2b24e341804a58c
> Fixes: 8bc4a9ed8504 ("[media] hackrf: add support for transmitter")
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
> ---
> v1 -> v2: fix overlong lines in patch description to remove warnings
> from checkpatch.
> 
>  drivers/media/usb/hackrf/hackrf.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/hackrf/hackrf.c b/drivers/media/usb/hackrf/hackrf.c
> index 0b50de8775a3..bc910b35f605 100644
> --- a/drivers/media/usb/hackrf/hackrf.c
> +++ b/drivers/media/usb/hackrf/hackrf.c
> @@ -1513,12 +1513,12 @@ static int hackrf_probe(struct usb_interface *intf,
>  	return 0;
>  err_video_unregister_device_rx:
>  	video_unregister_device(&dev->rx_vdev);
> -err_v4l2_device_unregister:
> -	v4l2_device_unregister(&dev->v4l2_dev);
>  err_v4l2_ctrl_handler_free_tx:
>  	v4l2_ctrl_handler_free(&dev->tx_ctrl_handler);
>  err_v4l2_ctrl_handler_free_rx:
>  	v4l2_ctrl_handler_free(&dev->rx_ctrl_handler);
> +err_v4l2_device_unregister:
> +	v4l2_device_unregister(&dev->v4l2_dev);
>  err_kfree:
>  	kfree(dev);
>  err:
>