Re: [PATCH v9 3/3] clk: en7523: add support for Airoha AN7583 clock

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] clk: uaf and leak on probe error path due to unmanaged clock registration
- [High] clk: en7523: race condition in RMW operations on shared SCU registers
--

commit 8fb64358a2b708163ffd1fa2a6071fd191ad66a7
Author: Christian Marangi <ansuelsmth@gmail.com>

clk: en7523: add support for Airoha AN7583 clock

Add support for Airoha AN7583 clock and reset.

> diff --git a/drivers/clk/clk-en7523.c b/drivers/clk/clk-en7523.c
> --- a/drivers/clk/clk-en7523.c
> +++ b/drivers/clk/clk-en7523.c
> @@ -1088,6 +1287,43 @@ static int en7581_clk_hw_init(struct platform_device *pdev,
>  	return en7581_serdes_phy_register(&pdev->dev);
>  }
>  
> +static int an7583_clk_hw_init(struct platform_device *pdev,
> +			      struct clk_hw_onecell_data *clk_data)
> +{
> +	struct en_clk_priv *priv = platform_get_drvdata(pdev);
> +	struct device *dev = &pdev->dev;
> +	struct regmap *map;
> +	void __iomem *base;
> +	u32 val;
> +	int ret;
> +
> +	map = syscon_regmap_lookup_by_phandle(dev->of_node, "airoha,chip-scu");
> +	if (IS_ERR(map))
> +		return PTR_ERR(map);
> +
> +	base = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(base))
> +		return PTR_ERR(base);
> +
> +	priv->base = base;
> +
> +	en7581_register_clocks(dev, clk_data, map, base);
> +
> +	val = readl(base + REG_RST_CTRL1);
> +	val |= REG_PCIE_HB_RST;
> +	writel(val, base + REG_RST_CTRL1);
> +	val &= ~REG_PCIE_HB_RST;
> +	writel(val, base + REG_RST_CTRL1);
> +
> +	ret = en7581_reset_register(dev, base, an7583_rst_map,
> +				    ARRAY_SIZE(an7583_rst_map),
> +				    en7581_rst_ofs);
> +	if (ret)
> +		return ret;

[Severity: High]
Does this early return path leak the fixed-rate clocks and cause a
use-after-free for the PCIe clock?

an7583_clk_hw_init() calls en7581_register_clocks(), which registers
fixed-rate clocks via clk_hw_register_fixed_rate() and a PCIe clock
via en7523_register_pcie_clk().

Looking at en7523_register_pcie_clk(), it allocates a struct en_clk_gate
using devm_kzalloc(), but registers it using the unmanaged
clk_hw_register().

If en7581_reset_register() fails here and we return early, the probe fails.
The devres framework will automatically free the devm_kzalloc'd
en_clk_gate structure. However, because the clock was not registered
with a managed API (like devm_clk_hw_register()), it remains linked in
the Common Clock Framework.

Can this cause a use-after-free when the framework later accesses this
clock, as well as a memory leak for the fixed-rate clocks?

[ ... ]

> @@ -1241,6 +1477,18 @@ static const struct en_clk_soc_data en7581_data = {
>  	.hw_init = en7581_clk_hw_init,
>  };
>  
> +static const struct en_clk_soc_data an7583_data = {
> +	.base_clks = an7583_base_clks,
> +	/* We increment num_clocks by 1 to account for additional PCIe clock */
> +	.num_clocks = ARRAY_SIZE(an7583_base_clks) + 1,
> +	.pcie_ops = {
> +		.is_enabled = en7581_pci_is_enabled,
> +		.enable = en7581_pci_enable,
> +		.disable = en7581_pci_disable,
> +	},

[Severity: High]
Is there a race condition here between the clock and reset operations
on shared SCU registers?

This struct assigns en7581_pci_enable() to the pcie_ops.enable callback.
Meanwhile, an7583_clk_hw_init() registers reset controls using
an7583_rst_map.

A driver invoking a clock operation could trigger en7581_pci_enable(),
while another execution context invoking a reset operation could trigger
en7523_reset_update().

Both of these operations appear to modify the exact same physical registers
(like REG_PCI_CONTROL/REG_NP_SCU_PCIC at 0x88 and
REG_RESET_CONTROL1/REG_RST_CTRL1 at 0x834) using unprotected readl() and
writel() read-modify-write sequences. Since they belong to different
frameworks, there is no shared lock protecting the state.

Could this cause data corruption of the SCU register state, leading to
lost clock enable/disable or reset assert/deassert updates?

> +	.hw_init = an7583_clk_hw_init,
> +};
> +
>  static const struct en_clk_soc_data en751221_data = {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012115954.92718-1-ansuelsmth@gmail.com?part=3