Re: [PATCH 3/3] ASoC: tlv320aic32x4: Make sure clk_init_data is fully initialized

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

Pre-existing issues:
- [High] Memory leak and Use-After-Free via unmanaged clk_lookup entries.
- [High] Data race and stack Use-After-Free due to assigning a block-scoped compound literal to a global array.
- [High] Unchecked return values of resource allocations and clock registrations mask initialization failures.
- [High] Hardcoded clock names cause registration failures for multiple codec instances.
- [High] Unvalidated requested clock rate causes a division by zero panic.
- [High] Unvalidated parent clock rate causes a division by zero panic.
--

--- Patch [3]: [PATCH 3/3] ASoC: tlv320aic32x4: Make sure clk_init_data is fully initialized ---
commit bdb202ba7d1e5e85e1733f12689e1c8729d8c7d3
Author: Geert Uytterhoeven <[email protected]>

ASoC: tlv320aic32x4: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need.  However,
not initializing all members may cause subtle issues, which are only
exposed when CONFIG_INIT_STACK_ALL_PATTERN or CONFIG_INIT_STACK_NONE is
enabled.

Make sure all members are fully initialized, to avoid such bugs, and to
prevent future breakage when converting drivers to a different method
for specifying the parents.

> diff --git a/sound/soc/codecs/tlv320aic32x4-clk.c b/sound/soc/codecs/tlv320aic32x4-clk.c
> index deed61650e09d..ad15ce9c60791 100644
> --- a/sound/soc/codecs/tlv320aic32x4-clk.c
> +++ b/sound/soc/codecs/tlv320aic32x4-clk.c
> @@ -451,8 +451,8 @@ static struct aic32x4_clkdesc aic32x4_clkdesc_array[] = {
>  static struct clk *aic32x4_register_clk(struct device *dev,
>  					struct aic32x4_clkdesc *desc)
>  {
> -	struct clk_init_data init;
>  	struct clk_aic32x4 *priv;
> +	struct clk_init_data init = {};
>  	const char *devname = dev_name(dev);
>  
>  	init.ops = desc->ops;

[Severity: High]
This isn't a bug introduced by this patch, but does hardcoding the clock
name directly from the static descriptor (init.name = desc->name) prevent
probing multiple instances of this codec?

If devm_clk_register() is called on a system with multiple codecs, will
it fail with -EEXIST because names like 'pll' and 'codec_clkin' are reused?


While reviewing this file, I noticed a few other pre-existing regressions:

[Severity: High]
This is a pre-existing issue, but is there a clk_lookup entry leaked when
the device is unbound?

sound/soc/codecs/tlv320aic32x4-clk.c:aic32x4_register_clk() {
    ...
	clk_hw_register_clkdev(&priv->hw, desc->name, devname);
	return devm_clk_register(dev, &priv->hw);
}

Since clk_hw_register_clkdev() dynamically allocates a clk_lookup on the
global list without using a devres variant or dropping it in teardown,
will the lookup retain a dangling pointer to the freed priv->hw if probe
fails or the driver is unbound?


[Severity: High]
This isn't a bug introduced by this patch, but is stack memory being written
into the static global aic32x4_clkdesc_array?

sound/soc/codecs/tlv320aic32x4-clk.c:aic32x4_register_clocks() {
    ...
	aic32x4_clkdesc_array[0].parent_names =
			(const char* []) { mclk_name, "bclk", "gpio", "din" };
	aic32x4_clkdesc_array[1].parent_names =
			(const char *[]) { mclk_name, "bclk", "gpio", "pll" };
    ...
}

Because these compound literals are block-scoped (stack allocated), if this
function returns, won't the global array point to dead stack memory, leading
to a use-after-free when the clock framework reads the parent names?


[Severity: High]
This is a pre-existing issue, but are we discarding the return value of
aic32x4_register_clk() and masking probe failures?

sound/soc/codecs/tlv320aic32x4-clk.c:aic32x4_register_clocks() {
    ...
	for (i = 0; i < ARRAY_SIZE(aic32x4_clkdesc_array); ++i)
		aic32x4_register_clk(dev, &aic32x4_clkdesc_array[i]);

	return 0;
}

If aic32x4_register_clk() fails due to low memory or clock name collisions,
will the driver falsely report a successful probe?


[Severity: High]
This isn't a bug introduced by this patch, but could DIV_ROUND_UP() trigger a
division by zero panic?

sound/soc/codecs/tlv320aic32x4-clk.c:clk_aic32x4_div_determine_rate() {
    ...
	divisor = DIV_ROUND_UP(req->best_parent_rate, req->rate);
    ...
}

If a 0 rate is supplied via the Common Clock Framework API, will this
cause a kernel panic?

This same pattern also appears in clk_aic32x4_div_set_rate() where
DIV_ROUND_UP(parent_rate, rate) is called.


[Severity: High]
This is a pre-existing issue, but might do_div() trigger a division by
zero panic?

sound/soc/codecs/tlv320aic32x4-clk.c:clk_aic32x4_pll_calc_muldiv() {
    ...
	multiplier = (u64)rate * settings->p * 10000;
	do_div(multiplier, parent_rate);
    ...
}

If the PLL clock is queried or set when its parent clock is uninitialized or
reporting a 0 Hz rate, will do_div() divide by zero?

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