diff mbox series

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

Message ID 20260424000000.558-1-mhun512@gmail.com (mailing list archive)
State New
Headers
Series media: rc: sunxi-cir: unregister rc device on probe failure |

Commit Message

박명훈 May 13, 2026, 7:11 a.m. UTC 
From: Myeonghun Pak <mhun512@gmail.com>

After rc_register_device() succeeds, later probe failures must undo the
registration with rc_unregister_device(). The current error path jumps to
the allocation cleanup label and only calls rc_free_device(), leaving the
rc device registration and resources created by rc_register_device()
behind.

Add a registered-device unwind label for the IRQ lookup, IRQ request, and
hardware initialization failure paths. Keep rc_free_device() for failures
before rc_register_device() succeeds.

Fixes: b4e3e59fb59c ("[media] rc: add sunxi-ir driver")
Cc: stable@vger.kernel.org
Co-developed-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Ijae Kim <ae878000@gmail.com>
Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
---
 drivers/media/rc/sunxi-cir.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

Comments

Jernej Škrabec May 13, 2026, 8:23 p.m. UTC | #1 
Dne sreda, 13. maj 2026 ob 09:11:03 Srednjeevropski poletni čas je 박명훈 napisal(a):
> From: Myeonghun Pak <mhun512@gmail.com>
> 
> After rc_register_device() succeeds, later probe failures must undo the
> registration with rc_unregister_device(). The current error path jumps to
> the allocation cleanup label and only calls rc_free_device(), leaving the
> rc device registration and resources created by rc_register_device()
> behind.
> 
> Add a registered-device unwind label for the IRQ lookup, IRQ request, and
> hardware initialization failure paths. Keep rc_free_device() for failures
> before rc_register_device() succeeds.
> 
> Fixes: b4e3e59fb59c ("[media] rc: add sunxi-ir driver")
> Cc: stable@vger.kernel.org
> Co-developed-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Ijae Kim <ae878000@gmail.com>
> Signed-off-by: Myeonghun Pak <mhun512@gmail.com>
> ---
>  drivers/media/rc/sunxi-cir.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
> index 92ef4e7c6f..cc64a68dfe 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);
>  	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;

This return should not be here.  Device must be freed too.

Best regards,
Jernej

> +
>  exit_free_dev:
>  	rc_free_device(ir->rc);
>  
>
sashiko-bot@kernel.org May 14, 2026, 2:37 a.m. UTC | #2 
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.
diff mbox series

Patch

diff --git a/drivers/media/rc/sunxi-cir.c b/drivers/media/rc/sunxi-cir.c
index 92ef4e7c6f..cc64a68dfe 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);
 	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;
+
 exit_free_dev:
 	rc_free_device(ir->rc);