Re: [PATCH v2 4/4] ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Mutation of a global static template array with instance-specific data breaks the device driver model and enables data races.
- [Medium] Missing cleanup of global template array on error path, causing a dangling pointer.
--

commit 0aa6e4b317dd7227cb1e1e58ed456735772485cc
Author: Valerio Setti <[email protected]>

ASoC: meson: aiu: use aiu-formatter-i2s to format I2S output data

Create a new DAPM widget for the "I2S formatter" and place it on the path
between the FIFO and output DAI interface. This also removes the I2S output
formatting code from aiu-encoder-i2s since it's now handled by the new
aiu-formatter-i2s widget.

> diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c
> index f2890111c1d2c..64ace4d25d92c 100644
> --- a/sound/soc/meson/aiu.c
> +++ b/sound/soc/meson/aiu.c
> @@ -29,13 +29,22 @@ static SOC_ENUM_SINGLE_DECL(aiu_spdif_encode_sel_enum, AIU_I2S_MISC,
>  static const struct snd_kcontrol_new aiu_spdif_encode_mux =
>  	SOC_DAPM_ENUM("SPDIF Buffer Src", aiu_spdif_encode_sel_enum);
>  
> -static const struct snd_soc_dapm_widget aiu_cpu_dapm_widgets[] = {
> -	SND_SOC_DAPM_MUX("SPDIF SRC SEL", SND_SOC_NOPM, 0, 0,
> -			 &aiu_spdif_encode_mux),
> +#define AIU_WIDGET_SPDIF_SRC_SEL	0
> +#define AIU_WIDGET_I2S_FORMATTER	1
> +
> +static struct snd_soc_dapm_widget aiu_cpu_dapm_widgets[] = {

[Severity: High]
Does removing the const qualifier here and modifying the global template array
during aiu_probe() break the device driver model?

If multiple instances of this device are probed, won't they overwrite each
other's instance pointers in this shared array, potentially causing data races
or a use-after-free?

> +	[AIU_WIDGET_SPDIF_SRC_SEL] =
> +		SND_SOC_DAPM_MUX("SPDIF SRC SEL", SND_SOC_NOPM, 0, 0,
> +				 &aiu_spdif_encode_mux),
> +	[AIU_WIDGET_I2S_FORMATTER] =
> +		SND_SOC_DAPM_PGA_E("I2S Formatter", SND_SOC_NOPM, 0, 0, NULL, 0,
> +				   gx_formatter_event,
> +				   (SND_SOC_DAPM_PRE_PMU | SND_SOC_DAPM_PRE_PMD)),
>  };

[ ... ]

> @@ -282,6 +296,14 @@ static int aiu_probe(struct platform_device *pdev)
>  	if (ret)
>  		return ret;
>  
> +	/* Allocate the aiu-formatter into its widget */
> +	ret = gx_formatter_create(dev, &aiu_cpu_dapm_widgets[AIU_WIDGET_I2S_FORMATTER],
> +				  &aiu_formatter_i2s_drv, map);
> +	if (ret) {
> +		dev_err(dev, "Failed to allocate aiu formatter\n");
> +		goto err;
> +	}
> +
>  	/* Register the cpu component of the aiu */
>  	ret = snd_soc_register_component(dev, &aiu_cpu_component,
>  					 aiu_cpu_dai_drv,

[Severity: Medium]
If snd_soc_register_component() fails, should this path use "goto err" instead
of returning directly?

Looking at the rest of aiu_probe():

	ret = snd_soc_register_component(dev, &aiu_cpu_component,
					 aiu_cpu_dai_drv,
					 ARRAY_SIZE(aiu_cpu_dai_drv));
	if (ret) {
		dev_err(dev, "Failed to register cpu component\n");
		return ret;
	}

Since gx_formatter_create() allocates memory and assigns it to the global
array, returning directly skips the gx_formatter_free() call at the err label.
The devres framework will free the memory, but wouldn't this leave a dangling
pointer in the global aiu_cpu_dapm_widgets array?

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

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