Re: [PATCH 5/5] rtc: microcrystal: Make sure clk_init_data is fully initialized

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unmanaged `of_clk_add_provider` combined with managed `devm_clk_register` creates a Use-After-Free and memory leak upon driver unbind.
- [High] Missing timeout in `rv3032_hwmon_read_temp` hardware polling loop can cause an infinite loop.
--

--- Patch [5]: [PATCH 5/5] rtc: microcrystal: Make sure clk_init_data is fully initialized ---
commit c0e71ecfe90533b7e0c9d613fb7e76ed54fcf87d
Author: Geert Uytterhoeven <geert+renesas@glider.be>

rtc: microcrystal: Make sure clk_init_data is fully initialized

This commit fully initializes the clk_init_data structure to avoid subtle
issues when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled. This prevents breakage when drivers are converted to different
methods for specifying parents.

> diff --git a/drivers/rtc/rtc-rv3028.c b/drivers/rtc/rtc-rv3028.c
> index d96f6bb68850d..d25e33d0835ee 100644
> --- a/drivers/rtc/rtc-rv3028.c
> +++ b/drivers/rtc/rtc-rv3028.c
> @@ -816,7 +816,7 @@ static int rv3028_clkout_register_clk(struct rv3028_data *rv3028,
>  {
>  	int ret;
>  	struct clk *clk;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct device_node *node = client->dev.of_node;
>  
>  	ret = regmap_update_bits(rv3028->regmap, RV3028_STATUS,

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this function,
I noticed a potential issue with how the clock provider is registered later in
rv3028_clkout_register_clk():

	clk = devm_clk_register(&client->dev, &rv3028->clkout_hw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

Because of_clk_add_provider() is unmanaged, does this cause a use-after-free
and memory leak when the driver is unbound?

The driver registers a clock provider using the unmanaged
of_clk_add_provider() but uses devm_clk_register() for the clock itself.

When the driver is unbound, devres automatically unwinds devm_clk_register()
and frees the clock structure.

Since there is no remove callback or devres action to call
of_clk_del_provider(), it seems the provider remains in the global
of_clk_providers list with a dangling pointer to the freed clock.

Since driver unbind can be triggered by a privileged user via sysfs or by
hardware hot-unplug, can this lead to a kernel panic if another driver looks
up the clock after the device is unbound?

> diff --git a/drivers/rtc/rtc-rv3032.c b/drivers/rtc/rtc-rv3032.c
> index 6bafdec637aea..687faaef42b25 100644
> --- a/drivers/rtc/rtc-rv3032.c
> +++ b/drivers/rtc/rtc-rv3032.c
> @@ -752,7 +752,7 @@ static int rv3032_clkout_register_clk(struct rv3032_data *rv3032,
>  {
>  	int ret;
>  	struct clk *clk;
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	struct device_node *node = client->dev.of_node;
>  
>  	ret = regmap_update_bits(rv3032->regmap, RV3032_TLSB, RV3032_TLSB_CLKF, 0);

[Severity: High]
This is also a pre-existing issue, but rv3032_clkout_register_clk() appears
to have the same unmanaged provider registration:

	clk = devm_clk_register(&client->dev, &rv3032->clkout_hw);
	if (!IS_ERR(clk))
		of_clk_add_provider(node, of_clk_src_simple_get, clk);

Does this also leave a dangling pointer in the global of_clk_providers list
when devres frees the clock upon unbind?

While looking at drivers/rtc/rtc-rv3032.c, I also noticed another
pre-existing issue in the hwmon code:

drivers/rtc/rtc-rv3032.c:rv3032_hwmon_read_temp() {
    ...
	do {
		prev = temp;

		ret = regmap_bulk_read(rv3032->regmap, RV3032_TLSB, buf, sizeof(buf));
		if (ret)
			return ret;

		temp = sign_extend32(buf[1], 7) << 4;
		temp |= FIELD_GET(RV3032_TLSB_TEMP, buf[0]);
	} while (temp != prev);
    ...
}

[Severity: High]
This is a pre-existing issue, but does this loop lack a timeout or maximum
retry count?

If the I2C hardware enters an unstable state and is faulty or noisy, it could
consistently return varying values, preventing the loop from terminating.

Since this code path is exposed to unprivileged userspace via the hwmon sysfs
attributes, can this be used by unprivileged users to trigger an indefinite
hang on a task?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787241693.git.geert+renesas@glider.be?part=5