Re: [PATCH 10/16] ASoC: omap3pandora: Rewrite sound card driver as a platform driver with DT

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [Critical] Device Tree ABI breakage due to removal of fallback machine-compatible matching and strict requirement for new DT nodes/properties.
- [High] Double-decrement of sample rate user count when `grab_sample_rate()` fails.
- [High] `relax_sample_rate()` incorrectly excludes hardware's minimum and maximum sample rates and permanently erases constraints applied by other components.
- [High] Static global `snd_soc_card` structure reuse causes list corruption on multiple probes and dangling pointer Use-After-Free.
- [High] Calling `mutex_destroy()` synchronously in `.remove` while using devm-based sound card registration creates a Use-After-Free vulnerability.
- [Medium] Missing `mutex_destroy()` on early probe error paths.
- [Low] Unused variable `codec_dai` in `omap3pandora_common_hw_params`.
--

--- Patch [10]: [PATCH 10/16] ASoC: omap3pandora: Rewrite sound card driver as a platform driver with DT ---
commit 8781d4ef4d1eb0b2da4ee910133603e631f2f4a9
Author: Stefan Leichter <sle85276@gmx.de>

ASoC: omap3pandora: Rewrite sound card driver as a platform driver with DT

This change converts the omap3pandora sound card driver to a standard
platform driver matched via Device Tree. It also moves code into the
PCM1773 codec driver and splits capture/playback operations.

> diff --git a/sound/soc/ti/omap3pandora.c b/sound/soc/ti/omap3pandora.c
> index 6c9c184cd9d6f..d3509473f5df0 100644
> --- a/sound/soc/ti/omap3pandora.c
> +++ b/sound/soc/ti/omap3pandora.c

[ ... ]

> -static int omap3pandora_hw_params(struct snd_pcm_substream *substream,
> -	struct snd_pcm_hw_params *params)
> +static int omap3pandora_common_hw_params(struct snd_pcm_substream *substream,
> +					 struct snd_pcm_hw_params *params)
>  {
>  	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
>  	struct snd_soc_dai *codec_dai = snd_soc_rtd_to_codec(rtd, 0);

[Severity: Low]
Is codec_dai used anywhere in this function now? It looks like the usage
was moved to omap3pandora_capture_hw_params(), leaving this variable unused
and potentially causing build warnings.

[ ... ]

> +static inline void relax_sample_rate(struct snd_pcm_substream *substream)
> +{
> +	const struct snd_interval default_sample_rate_range = {
> +		.min = substream->runtime->hw.rate_min,
> +		.max = substream->runtime->hw.rate_max,
> +		.openmin = 1,
> +		.openmax = 1,
> +	};
> +
> +	*constrs_interval(&substream->runtime->hw_constraints,
> +			  SNDRV_PCM_HW_PARAM_RATE) =
> +		default_sample_rate_range;
> +}

[Severity: High]
Does setting openmin and openmax to 1 permanently exclude the hardware's
exact minimum and maximum supported rates? It seems this would block rates
like exactly 8000 Hz or 96000 Hz from being selected, as these flags make
the interval mathematically open.

Also, does overwriting the hw_constraints interval directly via
pointer assignment permanently erase other constraints that might have
been applied by the CPU DAI or Codec DAI?

[ ... ]

> +static int omap3pandora_playback_hw_params(struct snd_pcm_substream *substream,
> +					   struct snd_pcm_hw_params *params)
> +{

[ ... ]

> +	if (!ctx->playback_stream) {
> +		ctx->playback_stream = substream;
> +		ret = grab_sample_rate(ctx, substream, params);
> +		if (ret)
> +			return ret;
> +	}

[Severity: High]
Could this lead to a double-decrement of the sample rate user count?

If grab_sample_rate() fails due to a sample rate mismatch, it will call
release_sample_rate() and decrement the count. However, since
ctx->playback_stream is assigned before grab_sample_rate(), the ALSA core's
subsequent call to omap3pandora_playback_hw_free() will see
ctx->playback_stream as non-NULL and call release_sample_rate() a second
time.

The same pattern appears in omap3pandora_capture_hw_params(). Should the
stream pointer assignment happen after grab_sample_rate() succeeds, or
should it be cleared in the error path?

[ ... ]

>  /* SoC card */
>  static struct snd_soc_card snd_soc_card_omap3pandora = {
> -	.name = "omap3pandora",
> -	.owner = THIS_MODULE,
> -	.dai_link = omap3pandora_dai,
> -	.num_links = ARRAY_SIZE(omap3pandora_dai),
> -
> -	.dapm_widgets = omap3pandora_dapm_widgets,
> -	.num_dapm_widgets = ARRAY_SIZE(omap3pandora_dapm_widgets),
> -	.dapm_routes = omap3pandora_map,
> -	.num_dapm_routes = ARRAY_SIZE(omap3pandora_map),
> +	.name        = "omap3pandora",
> +	.owner       = THIS_MODULE,
> +	.dai_link    = omap3pandora_dai,
> +	.num_links   = ARRAY_SIZE(omap3pandora_dai),
> +	.dapm_widgets     = omap3pandora_local_widgets,
> +	.num_dapm_widgets = ARRAY_SIZE(omap3pandora_local_widgets),
>  };
>  
> -static struct platform_device *omap3pandora_snd_device;
> -
> -static int __init omap3pandora_soc_init(void)
> +static int omap3pandora_probe(struct platform_device *pdev)
>  {
>  	int ret;
> +	struct device *dev = &pdev->dev;
> +	struct snd_soc_card *card = &snd_soc_card_omap3pandora;
> +	struct omap3pandora_sound *ctx;
>  
> +	ctx = devm_kzalloc(dev, sizeof(*ctx), GFP_KERNEL);
> +	if (!ctx)
>  		return -ENOMEM;
>  
> +	mutex_init(&ctx->sample_rate_lock);
>  
> +	ctx->amp_gpio = devm_gpiod_get(dev, "amp", GPIOD_OUT_LOW);
> +	if (IS_ERR(ctx->amp_gpio)) {
> +		dev_err(dev, "cannot find amplifier gpio\n");
> +		return PTR_ERR(ctx->amp_gpio);
>  	}
>  
> +	ctx->amp_regulator = devm_regulator_get(dev, "amp");
> +	if (IS_ERR(ctx->amp_regulator)) {
> +		ret = PTR_ERR(ctx->amp_regulator);
> +		dev_err(dev, "Failed to request regulator for amplifier power: %d\n", ret);
> +		return ret;
>  	}

[Severity: Medium]
Do these early error returns leak the lockdep tracking for the mutex?

Since mutex_init() is called before these checks, shouldn't they use goto
err_mutex instead of returning directly?

[ ... ]

> +	if (dev->of_node) {
> +		ret = snd_soc_of_parse_card_name(card, "label");
> +		if (ret)
> +			card->name = "omap3pandora";
> +
> +		ret = snd_soc_of_parse_audio_simple_widgets(card, "widgets");

[Severity: High]
Does modifying the static global snd_soc_card_omap3pandora structure
in probe() create issues if the driver is unbound and rebound? 

Functions like snd_soc_of_parse_audio_simple_widgets() allocate memory
with devm_kcalloc() and assign it to the card structure. If unbound, devres
frees the memory, leaving dangling pointers in the static structure.
Furthermore, probing multiple times could corrupt the global linked list
by calling list_add() on the same card->list node twice.

> +		if (ret) {
> +			dev_err(dev, "Failed to parse DAPM widgets: %d\n", ret);
> +			goto err_mutex;
> +		}
> +
> +		ret = snd_soc_of_parse_audio_routing(card, "routing");
> +		if (ret) {
> +			dev_err(dev, "Failed to parse audio routing: %d\n", ret);
> +			goto err_mutex;
> +		}
>  	}

[Severity: Critical]
Does this strict property check break compatibility with older Device Trees?

The previous driver automatically instantiated the sound card based on the
root machine compatible string. By unconditionally requiring the new widgets
and routing properties and failing if they are absent, will users booting a
new kernel with an older DT lose audio functionality completely?

> +	snd_soc_card_set_drvdata(card, ctx);
> +
> +	ret = devm_snd_soc_register_card(dev, card);
> +	if (ret) {
> +		dev_err(dev, "Failed to register sound card: %d\n", ret);
> +		goto err_mutex;
>  	}
>  
>  	return 0;
>  
> +err_mutex:
> +	mutex_destroy(&ctx->sample_rate_lock);
>  	return ret;
>  }
>  
> -module_init(omap3pandora_soc_init);
>  
> -static void __exit omap3pandora_soc_exit(void)
> +static void omap3pandora_remove(struct platform_device *pdev)
>  {
> -	regulator_put(omap3pandora_dac_reg);
> -	platform_device_unregister(omap3pandora_snd_device);
> +	struct snd_soc_card *card = platform_get_drvdata(pdev);
> +	struct omap3pandora_sound *ctx = snd_soc_card_get_drvdata(card);
> +
> +	mutex_destroy(&ctx->sample_rate_lock);
>  }

[Severity: High]
Can destroying the mutex here lead to a use-after-free or lockdep panic?

The sound card is registered using devm_snd_soc_register_card(), which
defers unregistration to the devres cleanup phase. This runs after the
omap3pandora_remove() function returns.

If we call mutex_destroy() synchronously in .remove, does this leave a
window where the sound card is still fully active and exposed to userspace,
but its internal lock has been destroyed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1783749722.git.hns@goldelico.com?part=10