diff mbox series

rtc: ac100: Assign .num before accessing .hws

Message ID AS8P251MB00013E724A77A355668B6CCEC8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM (mailing list archive)
State New
Headers
Series rtc: ac100: Assign .num before accessing .hws |

Commit Message

Aamir Ahmed Sept. 5, 2026, 6:38 p.m. UTC 
Commit f316cdff8d67 ("clk: Annotate struct clk_hw_onecell_data with
__counted_by") annotated the hws member of 'struct clk_hw_onecell_data'
with __counted_by, which informs the bounds sanitizer (UBSAN_BOUNDS)
about the number of elements in .hws[], so that it can warn when .hws[]
is accessed out of bounds. As noted in that change, the __counted_by
member must be initialized with the number of elements before the first
array access happens, otherwise there will be a warning from each access
prior to the initialization because the number of elements is zero.
This occurs in ac100_rtc_register_clks() due to .num being assigned only
after every clkout clock has been stored in .hws[]. With
CONFIG_UBSAN_BOUNDS and a compiler that implements __counted_by (GCC
15.1+ or Clang 20.1+), this triggers an array-index-out-of-bounds report
during probe, and with CONFIG_UBSAN_TRAP the first store traps.

Initialize .num with AC100_CLKOUT_NUM, the number of elements .hws[] was
allocated with, right after the allocation. That is the value the loop
counter ends up at on the success path anyway, so the provider's
behaviour is unchanged.

Cc: stable@vger.kernel.org
Fixes: f316cdff8d67 ("clk: Annotate struct clk_hw_onecell_data with __counted_by")
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
Found while auditing the remaining clk_hw_onecell_data users that assign
.num only after touching .hws[], following the fixes already merged for
clk-s2mps11 (3e14c7207a97), exynos-clkout (cf33f0b7df13) and
clk-raspberrypi (6dc445c19050). The audit, the fix and this changelog
were drafted with an LLM assistant and reviewed by hand.

Compile-tested only (W=1, no warnings) on x86_64 with GCC 13.3, with
CONFIG_RTC_DRV_AC100=m forced on the make command line because the
driver has no COMPILE_TEST option. GCC 13.3 does not implement
__counted_by (CC_HAS_COUNTED_BY needs GCC 15.1+ or Clang 20.1+), so the
build only confirms that the change compiles; the sanitizer path was not
exercised. I do not have the hardware, so this is not runtime-tested and
no UBSAN report was captured.

Based on v7.3-rc1.

 drivers/rtc/rtc-ac100.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)


base-commit: 654ae5d73c05bd2943d65636ce6cd0aa46e62f18

Comments

sashiko-bot@kernel.org Sept. 5, 2026, 6:49 p.m. UTC | #1 
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Missing cleanup of OF clock provider and fixed-rate clock on probe failure causes a Use-After-Free and resource leak.
--

commit a37d5a060e39167e75415bce4b97e44f0383fe08
Author: Aamir Ahmed <elb12345@hotmail.co.uk>

rtc: ac100: Assign .num before accessing .hws

This commit assigns .num in struct clk_hw_onecell_data before accessing
its .hws[] elements in ac100_rtc_register_clks(). This fixes a bounds
sanitizer warning triggered by the __counted_by annotation introduced
in a previous commit.

> diff --git a/drivers/rtc/rtc-ac100.c b/drivers/rtc/rtc-ac100.c
> index bba7115ff3ad0..a2f465438fd05 100644
> --- a/drivers/rtc/rtc-ac100.c
> +++ b/drivers/rtc/rtc-ac100.c
[ ... ]
> @@ -360,7 +362,6 @@ static int ac100_rtc_register_clks(struct ac100_rtc_dev *chip)
>  		chip->clk_data->hws[i] = &clk->hw;
>  	}
>  
> -	chip->clk_data->num = i;
>  	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, chip->clk_data);
>  	if (ret)
>  		goto err_unregister_rtc_32k;

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of cleanup
in ac100_rtc_probe() lead to a use-after-free and resource leak?

In ac100_rtc_probe(), ac100_rtc_register_clks() manually registers a
clock provider and a fixed-rate clock which are not devm-managed:

drivers/rtc/rtc-ac100.c:ac100_rtc_probe() {
    ...
	ret = ac100_rtc_register_clks(chip);
	if (ret)
		return ret;

	return devm_rtc_register_device(chip->rtc);
}

If devm_rtc_register_device() fails, the probe function exits and devm
cleans up chip->clk_data. However, since the clock provider is not
unregistered on this error path, it remains active in the Common Clock
Framework with a dangling pointer to the freed chip->clk_data.

Can any subsequent attempt to query a clock via device tree traverse the
provider list, dereference the freed pointer, and trigger a crash?
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-ac100.c b/drivers/rtc/rtc-ac100.c
index bba7115ff3a..a2f465438fd 100644
--- a/drivers/rtc/rtc-ac100.c
+++ b/drivers/rtc/rtc-ac100.c
@@ -317,6 +317,8 @@  static int ac100_rtc_register_clks(struct ac100_rtc_dev *chip)
 	if (!chip->clk_data)
 		return -ENOMEM;
 
+	chip->clk_data->num = AC100_CLKOUT_NUM;
+
 	chip->rtc_32k_clk = clk_hw_register_fixed_rate(chip->dev,
 						       AC100_RTC_32K_NAME,
 						       NULL, 0,
@@ -360,7 +362,6 @@  static int ac100_rtc_register_clks(struct ac100_rtc_dev *chip)
 		chip->clk_data->hws[i] = &clk->hw;
 	}
 
-	chip->clk_data->num = i;
 	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, chip->clk_data);
 	if (ret)
 		goto err_unregister_rtc_32k;