Re: [PATCH v2 2/3] rtc: ftrtc010: use devm clock APIs to fix use-after-disable in remove

[sashiko-bot@kernel.org]

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

New issues:
- [High] Replacing devm_clk_get() with devm_clk_get_enabled() incorrectly turns optional clocks into mandatory ones, breaking probe on platforms where the clocks are missing.
- [High] The probe function erroneously returns success (0) if devm_clk_get_enabled() returns NULL, silently bypassing device initialization.

Pre-existing issues:
- [High] The interrupt handler unconditionally returns IRQ_HANDLED on a shared IRQ line without verifying if the hardware actually generated the interrupt.
- [High] Reading the RTC time registers sequentially without checking for rollovers can result in reading corrupted/inconsistent time values.
--

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

rtc: ftrtc010: use devm clock APIs to fix use-after-disable in remove
    
This commit replaces manual clock management with devm_clk_get_enabled() to
fix a use-after-disable window during driver removal. It also removes the
manual .remove() callback, relying on devres for cleanup.

> diff --git a/drivers/rtc/rtc-ftrtc010.c b/drivers/rtc/rtc-ftrtc010.c
> --- a/drivers/rtc/rtc-ftrtc010.c
> +++ b/drivers/rtc/rtc-ftrtc010.c
[ ... ]
> @@ -120,51 +120,33 @@ static int ftrtc010_rtc_probe(struct platform_device *pdev)
>  		return -ENOMEM;
>  	platform_set_drvdata(pdev, rtc);
>  
> -	rtc->pclk = devm_clk_get(dev, "PCLK");
> -	if (IS_ERR(rtc->pclk)) {
> +	rtc->pclk = devm_clk_get_enabled(dev, "PCLK");
> +	if (IS_ERR_OR_NULL(rtc->pclk)) {
>  		dev_err(dev, "could not get PCLK\n");
> -	} else {
> -		ret = clk_prepare_enable(rtc->pclk);
> -		if (ret) {
> -			dev_err(dev, "failed to enable PCLK\n");
> -			return ret;
> -		}
> +		return PTR_ERR_OR_ZERO(rtc->pclk);

[Severity: High]
Does this change inadvertently make the previously optional PCLK mandatory?

In the original code, if devm_clk_get() returned an error (like -ENOENT),
it logged a message but safely allowed probe to continue. With this patch,
if devm_clk_get_enabled() returns -ENOENT, PTR_ERR_OR_ZERO() will propagate
that error and abort the probe.

Additionally, if the kernel is configured without CONFIG_HAVE_CLK,
devm_clk_get_enabled() returns NULL. In that scenario, IS_ERR_OR_NULL()
evaluates to true, and PTR_ERR_OR_ZERO(NULL) returns 0.

Will this cause the probe function to immediately return success without
actually registering the RTC device or mapping memory?

>  	}
> -	rtc->extclk = devm_clk_get(dev, "EXTCLK");
> -	if (IS_ERR(rtc->extclk)) {
> +	rtc->extclk = devm_clk_get_enabled(dev, "EXTCLK");
> +	if (IS_ERR_OR_NULL(rtc->extclk)) {
>  		dev_err(dev, "could not get EXTCLK\n");
> -	} else {
> -		ret = clk_prepare_enable(rtc->extclk);
> -		if (ret) {
> -			dev_err(dev, "failed to enable EXTCLK\n");
> -			goto err_disable_pclk;
> -		}
> +		return PTR_ERR_OR_ZERO(rtc->extclk);

[Severity: High]
The same issues with optional clock requirement and silent probe bypass on
NULL apply here for EXTCLK.

[ ... ]
>  	rtc_dev->ops = &ftrtc010_rtc_ops;
>  
>  	sec  = readl(rtc->rtc_base + FTRTC010_RTC_SECOND);
>  	min  = readl(rtc->rtc_base + FTRTC010_RTC_MINUTE);
>  	hour = readl(rtc->rtc_base + FTRTC010_RTC_HOUR);
>  	days = readl(rtc->rtc_base + FTRTC010_RTC_DAYS);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for a
hardware clock rollover to corrupt the timestamp being read here?

Since the time registers are read sequentially without any check for
rollover across multiple units, a tick occurring exactly between these
sequential reads could result in erroneously mixing an old higher-order
value with a new lower-order value.

>  
>  	rtc_dev->range_min = (u64)days * 86400 + hour * 3600 + min * 60 + sec;
>  	rtc_dev->range_max = U32_MAX + rtc_dev->range_min;
>  
>  	ret = devm_request_irq(dev, rtc->rtc_irq, ftrtc010_rtc_interrupt,
>  			       IRQF_SHARED, pdev->name, dev);

[Severity: High]
This is a pre-existing issue, but since the interrupt is registered as
IRQF_SHARED, is it safe for ftrtc010_rtc_interrupt() to unconditionally
return IRQ_HANDLED?

If another device sharing this IRQ line asserts an interrupt, returning
IRQ_HANDLED without verifying the interrupt source could prevent the core
IRQ tracking mechanism from detecting and disabling spurious interrupt
storms.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260827080320.3351155-1-liudalin@kylinsec.com.cn?part=2