Re: [PATCH] rtc: ftrtc010: fix clock resource leak on probe failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Mixing devm_rtc_register_device() with manual clock disabling in .remove causes a use-after-disable vulnerability.
- [High] Shared interrupt handler unconditionally returns IRQ_HANDLED without checking hardware registers.
- [High] Integer overflow in `ftrtc010_rtc_read_time` due to 32-bit arithmetic wrapping.
--

commit 7252ea4ab6d10cb6ebbf156a3c7aa20a2dbb7aa7
Author: Liu Dalin <liudalin@kylinsec.com.cn>

rtc: ftrtc010: fix clock resource leak on probe failure

When devm_rtc_register_device() fails, ftrtc010_rtc_probe() returns the error
directly without releasing clocks obtained via clk_prepare_enable(). This
patch captures the return value and jumps to the error handling path to
properly release clocks via clk_disable_unprepare().

> diff --git a/drivers/rtc/rtc-ftrtc010.c b/drivers/rtc/rtc-ftrtc010.c
> index 02608d3784958..f398d6dc85ae4 100644
> --- a/drivers/rtc/rtc-ftrtc010.c
> +++ b/drivers/rtc/rtc-ftrtc010.c
> @@ -181,7 +181,11 @@ static int ftrtc010_rtc_probe(struct platform_device *pdev)
>  	if (unlikely(ret))
>  		goto err_disable_extclk;
>  
> -	return devm_rtc_register_device(rtc_dev);
> +	ret = devm_rtc_register_device(rtc_dev);
> +	if (ret)
> +		goto err_disable_extclk;
> +
> +	return 0;
>  
>  err_disable_extclk:
>  	clk_disable_unprepare(rtc->extclk);

[Severity: High]
This isn't a bug introduced by this patch, but mixing
devm_rtc_register_device() with manual clock disabling in ftrtc010_rtc_remove()
causes a use-after-disable issue.

In ftrtc010_rtc_remove(), the hardware clocks are manually disabled before
devres cleans up the RTC character device:

drivers/rtc/rtc-ftrtc010.c:ftrtc010_rtc_remove() {
    ...
	if (!IS_ERR(rtc->extclk))
		clk_disable_unprepare(rtc->extclk);
	if (!IS_ERR(rtc->pclk))
		clk_disable_unprepare(rtc->pclk);
}

Because the RTC device was registered with devm_rtc_register_device(), it
remains registered until devres cleanup runs after the .remove callback
returns.

Could an unprivileged process with an open file descriptor to /dev/rtc0 issue
an ioctl(RTC_RD_TIME) during this window?  Wouldn't this invoke
ftrtc010_rtc_read_time() which performs MMIO reads on the clock-gated
hardware, resulting in a synchronous external abort and crashing the system?

Should the clocks be managed with devm_clk_get_enabled() or
devm_add_action_or_reset(), tying their lifecycle to devres and eliminating
the need for manual teardown in ftrtc010_rtc_remove()?

[Severity: High]
This is a pre-existing issue, but the shared interrupt handler unconditionally
returns IRQ_HANDLED without checking hardware registers.

The driver explicitly registers the interrupt as shared in
ftrtc010_rtc_probe():

drivers/rtc/rtc-ftrtc010.c:ftrtc010_rtc_probe() {
    ...
	ret = devm_request_irq(dev, rtc->rtc_irq, ftrtc010_rtc_interrupt,
			       IRQF_SHARED, pdev->name, dev);
    ...
}

However, the handler blindly claims the interrupt, masking 'nobody cared'
warnings for the shared line:

drivers/rtc/rtc-ftrtc010.c:ftrtc010_rtc_interrupt() {
	return IRQ_HANDLED;
}

If another device shares this IRQ line and asserts an interrupt, the IRQ core
calls all registered handlers.  Since ftrtc010_rtc_interrupt() unconditionally
returns IRQ_HANDLED without checking device-specific registers, won't the
IRQ core incorrectly assume the interrupt was legitimately handled?

If the actual interrupting device is level-triggered and its driver is
unbound or fails to clear the interrupt, couldn't this cause an endless
interrupt loop that completely freezes the system because the IRQ line
remains asserted and the IRQ core will not disable it?

Should the interrupt handler read a status register to verify if the RTC
actually generated the interrupt and return IRQ_NONE if it did not?

[Severity: High]
This isn't a bug introduced by this patch, but there is an integer overflow in
ftrtc010_rtc_read_time() due to 32-bit arithmetic wrapping.

In ftrtc010_rtc_read_time(), the calculation for time evaluates entirely in
32-bit math:

drivers/rtc/rtc-ftrtc010.c:ftrtc010_rtc_read_time() {
    ...
	u32 days, hour, min, sec, offset;
	timeu64_t time;
    ...
	time = offset + days * 86400 + hour * 3600 + min * 60 + sec;
    ...
}

Since offset, days, hour, min, and sec are all 32-bit unsigned integers,
won't this silently overflow and wrap around U32_MAX before being assigned to
the 64-bit result?

If a privileged user sets the RTC time to a value near or beyond
the year 2106 (U32_MAX seconds), wouldn't the reported time wrap around
incorrectly, causing data corruption and contradicting the driver's
rtc_dev->range_max configuration?

Should one of the operands be cast to u64 before the arithmetic operations?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6A7910FC22A9FEF1+20260827015739.3314298-1-liudalin@kylinsec.com.cn?part=1