Re: [PATCH 2/2] mmc: sdhci-of-arasan: Make sure clk_init_data is fully initialized

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

Pre-existing issues:
- [High] Another driver in the same subsystem (meson-mx-sdio.c) suffers from the exact same uninitialized struct clk_init_data issue that this patch fixes.
- [High] The driver registers multiple clock providers for the same device node but only deletes one on teardown, leading to a leaked provider struct, a leaked device node reference, and a dangling UAF pointer in the global clock providers list.
--

--- Patch [2]: [PATCH 2/2] mmc: sdhci-of-arasan: Make sure clk_init_data is fully initialized ---
commit 34a49f083df630f25a24052b358249a2c48b6055
Author: Geert Uytterhoeven <[email protected]>

mmc: sdhci-of-arasan: Make sure clk_init_data is fully initialized

The clk_init_data structure contains mutually-exclusive members for
specifying clock parents. Uninitialized members can cause issues exposed by
certain stack initialization configs. This commit zeroes the struct to prevent
bugs and future breakage.

[Severity: High]
This isn't a bug introduced by this patch, but I noticed another driver in
the same subsystem suffers from the exact same uninitialized struct
clk_init_data issue that this patch fixes.

In drivers/mmc/host/meson-mx-sdio.c:meson_mx_mmc_register_clks():

	struct clk_init_data init;
	...
	init.name = devm_kasprintf(host->controller->parent, ...);
	init.ops = &meson_mx_mmc_cfg_div_ops;
	init.flags = 0;
	...

Does this also need zero-initialization to avoid leaving fields like
parent_data and parent_hws as stack garbage, which could lead to memory
corruption during device probe?

> diff --git a/drivers/mmc/host/sdhci-of-arasan.c b/drivers/mmc/host/sdhci-of-arasan.c
> index 0a4b4696bc01c..aec617aa8e0ca 100644
> --- a/drivers/mmc/host/sdhci-of-arasan.c
> +++ b/drivers/mmc/host/sdhci-of-arasan.c
[ ... ]
> @@ -1638,8 +1638,8 @@ sdhci_arasan_register_sampleclk(struct sdhci_arasan_data *sdhci_arasan,
>  				struct device *dev)
>  {
>  	struct sdhci_arasan_clk_data *clk_data = &sdhci_arasan->clk_data;
> +	struct clk_init_data sampleclk_init = {};
>  	struct device_node *np = dev->of_node;

[Severity: High]
This is a pre-existing issue, but I noticed a potential leak and dangling
pointer hazard when tearing down these clock providers.

If #clock-cells > 0, this driver registers both the sdcardclk and sampleclk
providers for the same dev->of_node via of_clk_add_provider().

During unbind, sdhci_arasan_unregister_sdclk() calls:

	of_clk_del_provider(dev->of_node);

Since of_clk_del_provider() stops after deleting the first matching provider
it finds (which would be sampleclk), does this leave the sdcardclk provider
on the global list? 

When the underlying clock is freed by devm, any subsequent DT lookup on this
node might fetch the dangling provider and return a freed struct clk pointer.
Could we be leaking a provider and device node reference here?

> -	struct clk_init_data sampleclk_init;
>  	const char *parent_clk_name;
>  	int ret;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2

_______________________________________________
linux-amlogic mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-amlogic