Re: [PATCH v6 2/4] clk: cix: add sky1 audss clock controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Manual unregistration of composite clocks creates a Use-After-Free vulnerability due to premature freeing of clk_hw pointers.
- [High] Clock operations access hardware registers without runtime resuming the provider device, causing bus faults.
- [High] Forceful suspend in remove() breaks PM guarantees for devres-managed child reset controller device.
- [Medium] Probe and resume error paths leak the PM runtime usage counter and leave the hardware reset deasserted.
- [Medium] Hardware register state is saved to a shared global static array during suspend, causing cross-device memory corruption.
- [Medium] Return value of reset_control_deassert() is ignored during probe.
- [Medium] Clock operations read hardware registers using regmap_read() without checking the return value, risking the use of uninitialized stack variables.
- [Low] Unconditional assignment of the dev_pm_ops structure bypasses dead code elimination when CONFIG_PM is disabled.
--

commit 2a293298bf161f9032ad5d5ba5c782ee20a8e27c
Author: Joakim Zhang <joakim.zhang@cixtech.com>

clk: cix: add sky1 audss clock controller

This commit adds a platform driver for the Cix Sky1 AUDSS CRU. It provides
mux, divider, and gate clocks for the DSP, SRAM, and other audio subsystem
blocks. It also handles the AUDSS NOC reset and initializes an auxiliary
reset device.

> diff --git a/drivers/clk/cix/clk-sky1-audss.c b/drivers/clk/cix/clk-sky1-audss.c
> index 0000000000000..25341be4f5bf6
> --- /dev/null
> +++ b/drivers/clk/cix/clk-sky1-audss.c
> @@ -0,0 +1,1201 @@
[ ... ]
> +static u32 sky1_reg_save[][2] = {
> +	{ INFO_HIFI0,  0 },
> +	{ INFO_CLK_GATE,  0 },
> +	{ INFO_CLK_DIV, 0 },
> +	{ INFO_CLK_MUX, 0 },
> +	{ INFO_MCLK, 0 },
> +};

[Severity: Medium]
Since sky1_reg_save is declared as a global static array, won't multiple
instances of this device overwrite each other's saved register states during
sky1_audss_clk_runtime_suspend()?

[ ... ]
> +static void sky1_audss_clk_gate_endisable(struct clk_hw *hw, int enable)
> +{
> +	struct clk_gate *gate = to_clk_gate(hw);
> +	struct sky1_clk_gate *sky1_gate = to_sky1_clk_gate(gate);
> +	int set = gate->flags & CLK_GATE_SET_TO_DISABLE ? 1 : 0;
> +	unsigned long flags = 0;
> +	u32 reg;
> +
> +	set ^= enable;
> +
> +	if (gate->lock)
> +		spin_lock_irqsave(gate->lock, flags);
> +	else
> +		__acquire(gate->lock);
> +
> +	if (gate->flags & CLK_GATE_HIWORD_MASK) {
> +		reg = BIT(gate->bit_idx + 16);
> +		if (set)
> +			reg |= BIT(gate->bit_idx);
> +	} else {
> +		regmap_read(sky1_gate->regmap, sky1_gate->offset, &reg);

[Severity: High]
Does this code access hardware registers while the device might be runtime
suspended?

At the end of sky1_audss_clk_probe(), pm_runtime_put_sync() is called, which
powers off the hardware. If a clock operation like
sky1_audss_clk_gate_endisable() is later invoked (for example, by reading
/sys/kernel/debug/clk/clk_summary), couldn't this cause a bus fault?

[ ... ]
> +static unsigned long sky1_audss_clk_divider_recalc_rate(struct clk_hw *hw,
> +							unsigned long parent_rate)
> +{
> +	struct clk_divider *divider = to_clk_divider(hw);
> +	struct sky1_clk_divider *sky1_div = to_sky1_clk_divider(divider);
> +	unsigned int val;
> +
> +	regmap_read(sky1_div->regmap, sky1_div->offset, &val);
> +	val = val >> divider->shift;

[Severity: Medium]
Can this lead to the use of an uninitialized variable?

If regmap_read() fails, val remains uninitialized, but it is immediately
shifted and used. Should the return value of regmap_read() be checked here
and in sky1_audss_clk_mux_get_parent()?

[ ... ]
> +static int sky1_audss_clk_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	priv->rst_noc = devm_reset_control_get_exclusive(dev, NULL);
> +	if (IS_ERR(priv->rst_noc))
> +		return dev_err_probe(dev, PTR_ERR(priv->rst_noc),
> +				     "failed to get audss noc reset");
> +
> +	reset_control_deassert(priv->rst_noc);

[Severity: Medium]
Is it safe to ignore the return value of reset_control_deassert() here? If
the reset fails to deassert, might subsequent MMIO accesses hang the system?

[ ... ]
> +	for (i = 0; i < devtype_data->clk_cfg_size; i++) {
> +		clk_table[devtype_data->clk_cfg[i].id] =
> +			sky1_audss_clk_register(dev,
[ ... ]
> +		if (IS_ERR(clk_table[devtype_data->clk_cfg[i].id])) {
> +			ret = PTR_ERR(clk_table[devtype_data->clk_cfg[i].id]);
> +			dev_err(dev, "failed to register clock %d, ret:%d\n",
> +				devtype_data->clk_cfg[i].id, ret);
> +			goto fail_array_clk;
> +		}
> +	}
> +
> +	ret = devm_of_clk_add_hw_provider(dev, of_clk_hw_onecell_get, priv->clk_data);
> +	if (ret) {
> +		dev_err(dev, "failed to add clock provider: %d\n", ret);
> +		goto fail_register;
> +	}
[ ... ]
> +fail_register:
> +fail_array_clk:
> +	while (i--)
> +		clk_hw_unregister_composite(clk_table[devtype_data->clk_cfg[i].id]);

[Severity: High]
Could this cause a use-after-free?

Since devm_of_clk_add_hw_provider() is devres-managed, the OF clock provider
will be unregistered automatically after the probe error path completes. If we
manually free the composite clocks here before the provider is unregistered,
couldn't concurrent clock lookups access the freed clk_hw pointers?

[ ... ]
> +fail_fixed_clk:
> +fail_clks_set:
> +	pm_runtime_put_sync(dev);
> +err_pm:
> +	pm_runtime_disable(dev);
> +	return ret;
> +}

[Severity: Medium]
Does this error path leak the PM runtime usage counter?

Earlier in sky1_audss_clk_probe(), pm_runtime_get_noresume() increments the
counter, but the err_pm path does not call pm_runtime_put_noidle(). Also,
should the rst_noc reset be re-asserted here to match the expected
powered-down state?

[ ... ]
> +static void sky1_audss_clk_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
> +	const struct sky1_audss_clks_devtype_data *devtype_data = priv->devtype_data;
> +	int i = 0;
> +
> +	for (i = 0; i < devtype_data->clk_cfg_size; i++)
> +		clk_hw_unregister_composite(priv->clk_data->hws[devtype_data->clk_cfg[i].id]);

[Severity: High]
Similar to the probe error path, does this manual unregistration open a
use-after-free window during unbind, since the devm-registered OF provider is
still active until after sky1_audss_clk_remove() finishes?

> +
> +	if (!pm_runtime_status_suspended(dev))
> +		pm_runtime_force_suspend(dev);
> +
> +	pm_runtime_disable(dev);
> +}

[Severity: High]
Can this forceful suspend break the devres-managed child reset controller?

The child reset controller device created by devm_auxiliary_device_create()
is unregistered after sky1_audss_clk_remove() returns. If we forcefully
suspend the hardware here, wouldn't the child driver access powered-down
registers if it attempts hardware cleanup during its own teardown?

[ ... ]
> +static int __maybe_unused sky1_audss_clk_runtime_resume(struct device *dev)
> +{
> +	struct sky1_audss_clks_priv *priv = dev_get_drvdata(dev);
[ ... ]
> +	ret = reset_control_deassert(priv->rst_noc);
> +	if (ret)
> +		return ret;
> +
> +	ret = sky1_audss_clks_enable(priv);
> +	if (ret) {
> +		dev_err(dev, "failed to enable clocks\n");
> +		return ret;
> +	}

[Severity: Medium]
If sky1_audss_clks_enable() fails, shouldn't we assert priv->rst_noc before
returning, so that the hardware reset state correctly matches the runtime
suspended state?

[ ... ]
> +static struct platform_driver sky1_audss_clk_driver = {
> +	.probe = sky1_audss_clk_probe,
> +	.remove = sky1_audss_clk_remove,
> +	.driver = {
> +		.name = "sky1-audss-clk",
> +		.suppress_bind_attrs = true,
> +		.of_match_table = sky1_audss_clk_of_match,
> +		.pm = &sky1_audss_clk_pm_ops,
> +	},
> +};

[Severity: Low]
Should the assignment to .pm be wrapped with pm_ptr()? Without it, the
sky1_audss_clk_pm_ops structure and its callbacks will not be eliminated as
dead code when CONFIG_PM is disabled.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623070805.211019-1-joakim.zhang@cixtech.com?part=2