Re: [PATCH] media: rc: sunxi-cir: unregister rc device on probe failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The newly added error path leaks the `rc_dev` structure.
- [High] Device registration and IRQ request occur before hardware clocks are enabled, creating a race condition that can panic the kernel.
- [High] Mixing devm-managed IRQ with manual error path cleanup creates a Use-After-Free vulnerability.
- [High] Unvalidated `clock-frequency` device tree property allows a divide-by-zero kernel panic.
--

commit 230e9a371e882d2d3972a39ec6ca89c53afbeaf9
Author: Myeonghun Pak <mhun512@gmail.com>

media: rc: sunxi-cir: unregister rc device on probe failure

This commit updates the error handling in sunxi_ir_probe() to ensure the
RC device is unregistered if a failure occurs after rc_register_device()
succeeds.

While reviewing this commit, I noticed a few potential issues, including
some existing concerns in the probe function.

> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index cb4c56bf0752a..f40f3ea0ac8e5 100644
> --- a/drivers/media/rc/sunxi-cir.c
> +++ b/drivers/media/rc/sunxi-cir.c
> @@ -344,22 +344,26 @@ static int sunxi_ir_probe(struct platform_device *pdev)
>  	ir->irq = platform_get_irq(pdev, 0);
>  	if (ir->irq < 0) {
>  		ret = ir->irq;
> -		goto exit_free_dev;
> +		goto exit_unregister_dev;
>  	}
>  
>  	ret = devm_request_irq(dev, ir->irq, sunxi_ir_irq, 0, SUNXI_IR_DEV, ir);
>  	if (ret) {
>  		dev_err(dev, "failed request irq\n");
> -		goto exit_free_dev;
> +		goto exit_unregister_dev;
>  	}
>  
>  	ret = sunxi_ir_hw_init(dev);

Is there a race condition since rc_register_device() and devm_request_irq()
are called before sunxi_ir_hw_init() enables the hardware clocks?

If an early interrupt fires, or if userspace interacts with the device via
sysfs or lirc (like setting the timeout), could the driver attempt to read
or write memory-mapped registers while the clocks are still disabled?

Also, if sunxi_ir_hw_init() fails, the code jumps to exit_unregister_dev.
Because the IRQ is managed by devm_request_irq(), it remains active until
the probe function completely returns. If the interrupt fires concurrently
while rc_unregister_device() is tearing down the device, could the handler
access freed resources?

>  	if (ret)
> -		goto exit_free_dev;
> +		goto exit_unregister_dev;
>  
>  	dev_info(dev, "initialized sunXi IR driver\n");
>  	return 0;
>  
> +exit_unregister_dev:
> +	rc_unregister_device(ir->rc);
> +	return ret;
> +

Does this error path leak the rc_dev structure?

rc_allocate_device() initializes the structure with a reference count of 1.
While rc_unregister_device() removes the device from the system, it does
not drop this initial reference count.

Would it be necessary to fall through or explicitly call rc_free_device()
here to fully free the memory?

>  exit_free_dev:
>  	rc_free_device(ir->rc);
>  

Additionally, earlier in sunxi_ir_probe(), the driver reads the
clock-frequency property and calculates the rx_resolution:

drivers/media/rc/sunxi-cir.c:sunxi_ir_probe() {
        ...
        of_property_read_u32(dn, "clock-frequency", &b_clk_freq);
        ...
        ir->rc->rx_resolution = (USEC_PER_SEC / (b_clk_freq / 64));
        ...
}

Could this cause a divide-by-zero issue? If the device tree specifies a
clock frequency less than 64 (such as 0), b_clk_freq / 64 would evaluate
to 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260424000000.558-1-mhun512@gmail.com?part=1