Re: [PATCH RFC 13/23] ASoC: qcom: lpass: support mapping I2S ports to multiple LPAIFs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Medium] ASoC: qcom: lpass: NULL deref in lpass_cpu_daiops_probe() on
  unprobed LPAIF
--

commit 4c412765d7dfc4828c0e1ec1c2ada16e697ee595
Author: Neil Armstrong <neil.armstrong@linaro.org>

ASoC: qcom: lpass: support mapping I2S ports to multiple LPAIFs

This commit adds an optional lpaif_mi2s_desc descriptor to struct
lpass_variant so MI2S ports can be mapped to and probed across multiple
LPAIF instances.

> diff --git a/sound/soc/qcom/lpass-cpu.c b/sound/soc/qcom/lpass-cpu.c
> index f8b9b2ec90553..411eecd9cbb93 100644
> --- a/sound/soc/qcom/lpass-cpu.c
> +++ b/sound/soc/qcom/lpass-cpu.c
> @@ -1079,6 +1079,8 @@ static void of_lpass_cpu_parse_dai_data(struct device *dev,
>  			data->mi2s_capture_sd_mode[port_id] =
>  				of_lpass_cpu_parse_sd_lines(dev, node,
>  						    "qcom,capture-sd-lines");
> +
> +			set_bit(port_id, &data->lpaif_port_enable);
>  		} else {
>  			dev_err(dev, "invalid dai id: %d\n", id);
>  			continue;
> @@ -1121,12 +1123,33 @@ static int lpass_cpu_init_lpaif(struct platform_device *pdev, int idx,
>  	struct regmap_config cfg = lpass_cpu_regmap_config;
>  	struct device *dev = &pdev->dev;
>  	void __iomem *base;
> -	int ret;
> +	int ret, i;
> +
> +	/* Only probe an LPAIF if one of the port is described in DT */
> +	if (v->mi2s_desc) {
> +		const struct lpaif_mi2s_desc *desc = &v->mi2s_desc[idx];
> +		bool enabled = false;
> +
> +		for (i = 0; i < desc->port_desc_count; ++i) {
> +			unsigned int port_id = mi2s_port_id(desc->port_desc[i].id);
> +
> +			if (test_bit(port_id, &drvdata->lpaif_port_enable)) {
> +				enabled = true;
> +				break;
> +			}
> +		}
> +
> +		if (!enabled)
> +			return 0;
> +	}

[ ... ]

> @@ -1283,8 +1320,30 @@ int asoc_qcom_lpass_cpu_platform_probe(struct platform_device *pdev)
>  
>  		port_id = mi2s_port_id(variant->dai_driver[i].id);
>  
> -		drvdata->mi2s_lpaif[port_id] = &drvdata->lpaif[0];
> -		drvdata->mi2s_intf[port_id] = port_id;
> +		if (variant->mi2s_desc) {
> +			unsigned int lpaif_idx, lpaif_port;
> +			const struct lpaif_mi2s_desc *desc;
> +
> +			for (lpaif_idx = 0; lpaif_idx < variant->num_mi2s_desc; ++lpaif_idx) {
> +				desc = &variant->mi2s_desc[lpaif_idx];
> +
> +				for (lpaif_port = 0;
> +				     lpaif_port < desc->port_desc_count;
> +				     ++lpaif_port) {
> +					if (desc->port_desc[lpaif_port].id !=
> +							variant->dai_driver[i].id)
> +						continue;
> +
> +					drvdata->mi2s_lpaif[port_id] =
> +						&drvdata->lpaif[lpaif_idx];
> +					drvdata->mi2s_intf[port_id] =
> +						desc->port_desc[lpaif_port].intf;
> +				}
> +			}
> +		} else {
> +			drvdata->mi2s_lpaif[port_id] = &drvdata->lpaif[0];
> +			drvdata->mi2s_intf[port_id] = port_id;
> +		}

[Severity: Medium]
Can this cause a NULL pointer dereference in lpass_cpu_daiops_probe() when
a sound card links to an MI2S DAI whose LPAIF was skipped by
lpass_cpu_init_lpaif()?

of_lpass_cpu_parse_dai_data() only sets lpaif_port_enable for MI2S ports
that have an explicit child node under the lpass-cpu DT node, while
leaving child nodes optional when using the default 8-channel SD line
configuration.  When lpass_cpu_init_lpaif() returns 0 early because
lpaif_port_enable is not set for any port on that LPAIF, lpaif->drvdata,
lpaif->map and lpaif->i2sctl remain NULL, and lpass_platform_init_lpaif()
likewise skips initializing lpaif->rd_dmactl and lpaif->wr_dmactl.

However, asoc_qcom_lpass_cpu_platform_probe() still points
drvdata->mi2s_lpaif[port_id] at &drvdata->lpaif[lpaif_idx] (or leaves it
NULL if the DAI is not listed in variant->mi2s_desc) and registers all
variant->dai_driver DAIs.  Unlike the platform suspend/resume paths,
lpass_cpu_daiops_probe() and lpass_platform_pcmops_open() do not check
whether the LPAIF was initialized before dereferencing lpaif and passing
lpaif->map to regmap_write():

sound/soc/qcom/lpass-cpu.c:lpass_cpu_daiops_probe() {
	struct lpass_data *drvdata = snd_soc_dai_get_drvdata(dai);
	unsigned int port_id = mi2s_port_id(dai->driver->id);
	struct lpass_lpaif *lpaif = drvdata->mi2s_lpaif[port_id];
	int ret;

	/* ensure audio hardware is disabled */
	ret = regmap_write(lpaif->map,
			LPAIF_I2SCTL_REG(drvdata->variant, drvdata->mi2s_intf[port_id]), 0);
    ...
}

Checking !lpaif || !lpaif->drvdata in lpass_cpu_daiops_probe() and
returning -ENODEV would prevent probing an uninitialized LPAIF.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113-topic-sm8650-el2-v1-0-5183f6aab9fc@linaro.org?part=13