Re: [PATCH] rtc: atcrtc100: cancel alarm work on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] CRASH: NULL pointer dereference and uninitialized memory execution. The alarm IRQ is requested prematurely in probe before the required structures are initialized.
- [High] LEAK AND UAF: The probe function's error path for devm_rtc_register_device() fails to clean up newly configured wake capabilities and the workqueue.
--

commit 913e1e349945d1e573a4b95047a974a86f29a566
Author: Fan Wu <fanwu01@zju.edu.cn>

rtc: atcrtc100: cancel alarm work on remove

This commit adds a remove callback to the atcrtc100 RTC driver to properly
clean up resources. It clears the wake IRQ, disables wakeup, frees the
interrupt, and cancels the alarm work to prevent use-after-free and -EEXIST
issues on rebinding.

> diff --git a/drivers/rtc/rtc-atcrtc100.c b/drivers/rtc/rtc-atcrtc100.c
> index 9808fc2c5a492..3c7313218da51 100644
> --- a/drivers/rtc/rtc-atcrtc100.c
> +++ b/drivers/rtc/rtc-atcrtc100.c
> @@ -337,6 +337,16 @@ static int atcrtc_probe(struct platform_device *pdev)
>  	return devm_rtc_register_device(atcrtc_dev->rtc_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak
resources or risk a use-after-free?

If devm_rtc_register_device() fails in atcrtc_probe(), it returns the error
immediately:

atcrtc_probe() {
    ...
    return devm_rtc_register_device(atcrtc_dev->rtc_dev);
}

However, earlier in atcrtc_probe(), device_init_wakeup() and
dev_pm_set_wake_irq() are called, which are not devres-managed. Do these wake
configurations leak on this error path?

Also, if the alarm IRQ fired and scheduled rtc_work, wouldn't returning without
cancel_work_sync() leave the work pending? Once devres frees atcrtc_dev,
couldn't this lead to a use-after-free when the system workqueue eventually
executes the uncancelled work?

[Severity: High]
This is also a pre-existing issue, but is there a race condition when the alarm
IRQ is requested earlier in atcrtc_probe()?

The IRQ is requested via devm_request_irq() before atcrtc_dev->rtc_dev is
allocated and before INIT_WORK() initializes rtc_work:

atcrtc_probe() {
    ...
    ret = devm_request_irq(&pdev->dev, atcrtc_dev->alarm_irq, ...);
    ...
    atcrtc_dev->rtc_dev = devm_rtc_allocate_device(&pdev->dev);
    ...
    INIT_WORK(&atcrtc_dev->rtc_work, atcrtc_alarm_clear);
}

If the hardware interrupt fires immediately after devm_request_irq(), the
handler atcrtc_alarm_isr() will execute:

atcrtc_alarm_isr() {
    ...
    schedule_work(&rtc->rtc_work);
    rtc_update_irq(rtc->rtc_dev, 1, RTC_AF | RTC_IRQF);
    ...
}

Will this cause schedule_work() to operate on uninitialized memory and
rtc_update_irq() to dereference a NULL pointer? Should the IRQ request be moved
until after these structures are fully initialized?

>  }
>  
> +static void atcrtc_remove(struct platform_device *pdev)
> +{
> +	struct atcrtc_dev *atcrtc_dev = platform_get_drvdata(pdev);
> +
> +	dev_pm_clear_wake_irq(&pdev->dev);
> +	device_init_wakeup(&pdev->dev, false);
> +	devm_free_irq(&pdev->dev, atcrtc_dev->alarm_irq, atcrtc_dev);
> +	cancel_work_sync(&atcrtc_dev->rtc_work);
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909152651.712614-1-fanwu01@zju.edu.cn?part=1