Re: [PATCH v2 07/11] media: meson-ir-tx: Ensure clock is disabled on unbind

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Use-After-Free in the redrat3 probe error path.

Pre-existing issues:
- [High] Incomplete fix for IRQ race condition during remove, leading to kernel panic and use-after-free.
--

--- Patch [7]: [PATCH v2 07/11] media: meson-ir-tx: Ensure clock is disabled on unbind ---
commit 7936d37b9d5412a12d7a610528273a3bb7e7ada2
Author: Sean Young <[email protected]>

media: meson-ir-tx: Ensure clock is disabled on unbind

clk_prepare_enable() needs a call to clk_disable_unprepare() on driver
unbind. Make it devm managed.

> diff --git a/drivers/media/rc/redrat3.c b/drivers/media/rc/redrat3.c
> index 3f828a564e192..b64c6440b062d 100644
> --- a/drivers/media/rc/redrat3.c
> +++ b/drivers/media/rc/redrat3.c

[ ... ]

> @@ -1111,17 +1127,20 @@ static int redrat3_dev_probe(struct usb_interface *intf,
>  	/* might be all we need to do? */
>  	retval = redrat3_enable_detector(rr3);
>  	if (retval < 0)
> -		goto led_free;
> +		goto rc_free;
>  
>  	/* we can register the device now, as it is ready */
>  	usb_set_intfdata(intf, rr3);
>  
>  	return 0;
>  
> +rc_free:
> +	rc_unregister_device(rr3->rc);
>  led_free:
>  	led_classdev_unregister(&rr3->led);
>  redrat_free:
>  	redrat3_delete(rr3, rr3->udev);
> +	rc_free_device(rr3->rc);

[Severity: High]
Can this error path trigger a use-after-free on the rr3 pointer?

When redrat3_delete() is called in the error handling path, it
unconditionally frees the rr3 structure:

drivers/media/rc/redrat3.c:redrat3_delete() {
    ...
    kfree(rr3);
}

Directly after this, rc_free_device(rr3->rc) is executed, which dereferences
the newly freed rr3 pointer to access the rc member.

Should rc_free_device() be called before redrat3_delete() to avoid this?

> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 28e840a7e5b8c..af1ee08ffdbec 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c

[ ... ]

> @@ -374,8 +374,8 @@ static void sunxi_ir_remove(struct platform_device *pdev)
>  	struct sunxi_ir *ir = platform_get_drvdata(pdev);
>  
>  	rc_unregister_device(ir->rc);
> -	rc_free_device(ir->rc);
>  	sunxi_ir_hw_exit(&pdev->dev);
> +	rc_free_device(ir->rc);
>  }

[Severity: High]
This is a pre-existing issue, but does moving rc_free_device() after
sunxi_ir_hw_exit() still leave a race condition that could cause a kernel
panic?

Because sunxi_ir_probe() uses devm_request_irq(), the interrupt remains
active until devres cleanup executes after sunxi_ir_remove() returns. If an
interrupt fires after sunxi_ir_hw_exit() disables the APB clocks but before
devres deregisters the IRQ, sunxi_ir_irq() will execute and read from the
unclocked SUNXI_IR_RXSTA_REG register.

On ARM architectures, accessing this register when the APB clock is disabled
causes a synchronous external abort, resulting in a kernel panic. If the
execution somehow proceeds, the handler would also dereference the newly
freed ir->rc pointer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic