diff mbox series

rtc: ac100: Fix clock provider use-after-free on probe failure

Message ID AS8P251MB0001A489CC45A591ABBB9BDCC8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM (mailing list archive)
State New
Headers
Series rtc: ac100: Fix clock provider use-after-free on probe failure |

Commit Message

Aamir Ahmed Sept. 5, 2026, 8:41 p.m. UTC 
ac100_rtc_register_clks() registers the RTC-32k clock with
clk_hw_register_fixed_rate() and the clock provider with
of_clk_add_hw_provider(). Neither is device managed, and both are
released only from ac100_rtc_remove(). The driver core does not call
remove() when probe() fails: really_probe() reaches probe_failed below
the device_remove() call and goes straight to releasing the device's
managed resources.

ac100_rtc_probe() ends with

	return devm_rtc_register_device(chip->rtc);

so if that fails after the clocks have been registered, the provider is
left on the global of_clk_providers list holding chip->clk_data, which
was allocated with devm_kzalloc() and is freed while probe() unwinds. A
later lookup on this device tree node then reads freed memory in
of_clk_hw_onecell_get(). Consumers that do exactly that exist in tree:
the wifi power sequence nodes on sun8i-a83t-bananapi-m3 and
sun8i-a83t-cubietruck-plus take <&ac100_rtc 1>, and the sun9i-a80 boards
route osc32k through <&ac100_rtc 0>. The window is narrow, as
devm_rtc_register_device() can only fail with -ENOMEM here, but the
provider is left dangling whenever it does.

The RTC-32k clock is leaked on the same path. Since clk_core_lookup()
searches a global list that is not scoped per device, __clk_register()
rejects the duplicate name with -EEXIST, so the leak also makes a later
probe of the same device fail.

The error path that returns -EINVAL when the ADDA 4M parent clock cannot
be found leaks the RTC-32k clock in the same way. No provider has been
registered at that point, so that one is a leak rather than a
use-after-free.

Register both with devm_clk_hw_register_fixed_rate() and
devm_of_clk_add_hw_provider() so that they are released whenever the
device goes away, on a failed probe as well as on unbind. Devres
releases in reverse order of acquisition, so the provider is removed
before chip->clk_data is freed, and the clkout children are now
unregistered before their parent rather than after it.
ac100_rtc_unregister_clks() and the remove callback then have nothing
left to do and are removed.

Fixes: d00a18a42c14 ("rtc: ac100: Add RTC driver for X-Powers AC100")
Reported-by: Sashiko AI <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-rtc/20260905184936.155E11F00A3A@smtp.kernel.org/
Assisted-by: LLM
Signed-off-by: Aamir Ahmed <elb12345@hotmail.co.uk>
---
This applies on top of "[PATCH] rtc: ac100: Assign .num before accessing
.hws", posted to this list earlier today:

https://lore.kernel.org/linux-rtc/AS8P251MB00013E724A77A355668B6CCEC8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM/

git format-patch emitted a prerequisite-patch-id for it below. I left
out the stable tag, both because the two live triggers are an order-0
allocation failure and a device tree that does not match the binding,
and because the merged fixes of this same shape carried a Fixes: tag
only: c7a639dac8e4 ("rtc: jz4740: Make sure clock provider gets
removed") and 9c48a5368504 ("rtc: pcf8563: fix clock provider leak on
unbind"). Happy to add it if you disagree.

On the choice of devm_of_clk_add_hw_provider(): it does not use
dev->of_node directly but goes through get_clk_provider_node(), which
substitutes the parent's node when the device's own node has no
#clock-cells. That substitution is inert here, since the binding
requires #clock-cells on the x-powers,ac100-rtc node and all six in-tree
boards set it, so the provider is still registered on the same node as
before.

The -EINVAL path is not reachable with any in-tree device tree: every
board gives the codec node clock-output-names and points the rtc node at
it, so of_clk_get_parent_name() always returns a name. It is fixed here
because the conversion covers it, not because I can trigger it.

The problem was pointed out by the Sashiko AI reviewer in reply to that
patch. I verified it against drivers/base/dd.c, drivers/clk/clk.c and
drivers/rtc/class.c before writing this.

chip->rtc_32k_clk is now only assigned and never read. Turning it into a
local and dropping the struct member would be a sensible follow-up, but
it is a separate cleanup so I left it out.

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. I do not have the hardware, so this
is not runtime-tested and neither error path was exercised. The fix and
this changelog were drafted with an LLM assistant and reviewed by hand.

 drivers/rtc/rtc-ac100.c | 36 +++++++-----------------------------
 1 file changed, 7 insertions(+), 29 deletions(-)


base-commit: 654ae5d73c05bd2943d65636ce6cd0aa46e62f18
prerequisite-patch-id: 1474421ba7fe25ac3d750fdfdda9bd4209a85869
diff mbox series

Patch

diff --git a/drivers/rtc/rtc-ac100.c b/drivers/rtc/rtc-ac100.c
index a2f465438fd..c579c4bf557 100644
--- a/drivers/rtc/rtc-ac100.c
+++ b/drivers/rtc/rtc-ac100.c
@@ -319,10 +319,10 @@  static int ac100_rtc_register_clks(struct ac100_rtc_dev *chip)
 
 	chip->clk_data->num = AC100_CLKOUT_NUM;
 
-	chip->rtc_32k_clk = clk_hw_register_fixed_rate(chip->dev,
-						       AC100_RTC_32K_NAME,
-						       NULL, 0,
-						       AC100_RTC_32K_RATE);
+	chip->rtc_32k_clk = devm_clk_hw_register_fixed_rate(chip->dev,
+							    AC100_RTC_32K_NAME,
+							    NULL, 0,
+							    AC100_RTC_32K_RATE);
 	if (IS_ERR(chip->rtc_32k_clk)) {
 		ret = PTR_ERR(chip->rtc_32k_clk);
 		dev_err(chip->dev, "Failed to register RTC-32k clock: %d\n",
@@ -356,28 +356,14 @@  static int ac100_rtc_register_clks(struct ac100_rtc_dev *chip)
 		if (ret) {
 			dev_err(chip->dev, "Failed to register clk '%s': %d\n",
 				init.name, ret);
-			goto err_unregister_rtc_32k;
+			return ret;
 		}
 
 		chip->clk_data->hws[i] = &clk->hw;
 	}
 
-	ret = of_clk_add_hw_provider(np, of_clk_hw_onecell_get, chip->clk_data);
-	if (ret)
-		goto err_unregister_rtc_32k;
-
-	return 0;
-
-err_unregister_rtc_32k:
-	clk_unregister_fixed_rate(chip->rtc_32k_clk->clk);
-
-	return ret;
-}
-
-static void ac100_rtc_unregister_clks(struct ac100_rtc_dev *chip)
-{
-	of_clk_del_provider(chip->dev->of_node);
-	clk_unregister_fixed_rate(chip->rtc_32k_clk->clk);
+	return devm_of_clk_add_hw_provider(chip->dev, of_clk_hw_onecell_get,
+					   chip->clk_data);
 }
 
 /*
@@ -615,13 +601,6 @@  static int ac100_rtc_probe(struct platform_device *pdev)
 	return devm_rtc_register_device(chip->rtc);
 }
 
-static void ac100_rtc_remove(struct platform_device *pdev)
-{
-	struct ac100_rtc_dev *chip = platform_get_drvdata(pdev);
-
-	ac100_rtc_unregister_clks(chip);
-}
-
 static const struct of_device_id ac100_rtc_match[] = {
 	{ .compatible = "x-powers,ac100-rtc" },
 	{ },
@@ -630,7 +609,6 @@  MODULE_DEVICE_TABLE(of, ac100_rtc_match);
 
 static struct platform_driver ac100_rtc_driver = {
 	.probe		= ac100_rtc_probe,
-	.remove		= ac100_rtc_remove,
 	.driver		= {
 		.name		= "ac100-rtc",
 		.of_match_table	= of_match_ptr(ac100_rtc_match),