Re: [PATCH v6 4/5] ASoC: airoha: Add AFE driver for Airoha AN7581

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Unconditional NULL pointer dereference in `mtk_dai_etdm_config` if `snd_soc_dai_set_fmt` is never called.
- [High] Unbalanced error path in `an7581_afe_fe_startup` leaves a dangling substream pointer leading to a Use-After-Free.
- [High] Impossible hardware constraints for capture streams due to a mix-up between frames and bytes.
- [Medium] Repeated, unbounded memory allocations in `mtk_dai_etdm_set_fmt` overwrite the previous pointer without freeing it.
- [Medium] DAI format clock inversion settings are successfully parsed but silently ignored by the hardware configuration.
--

commit e1645bd481353127d1a22ed7f6309ae66d234cd2
Author: Christian Marangi [off-list ref]

ASoC: airoha: Add AFE driver for Airoha AN7581

Add support for the Sound system present on Airoha AN7581 SoC. This is
based on the Mediatek AFE drivers and adds the PCM and the ETDM driver.

[ ... ]

[ ... ]

+static int an7581_afe_fe_startup(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
+	int ret;
+
+	ret = mtk_afe_fe_startup(substream, dai);
+	if (ret < 0)
+		return ret;
+
+	if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
+		ret = snd_pcm_hw_constraint_minmax(runtime,
+						   SNDRV_PCM_HW_PARAM_PERIOD_SIZE,
+						   0x2000, UINT_MAX);

[Severity: High]
Is this constraint using the correct parameter? The 
SNDRV_PCM_HW_PARAM_PERIOD_SIZE parameter is measured in frames, not bytes.

For an 8-channel, 32-bit capture stream (32 bytes per frame), 8192 frames
equals 262,144 bytes, which exceeds the absolute maximum period_bytes_max
of 131,072 bytes. This would cause the ALSA core interval intersection
to fail and return -EINVAL.

Was this intended to be SNDRV_PCM_HW_PARAM_PERIOD_BYTES?

+		if (ret < 0)
+			dev_err(afe->dev, "hw_constraint_minmax failed\n");
+	}
+
+	return ret;
+}

[Severity: High]
If snd_pcm_hw_constraint_minmax() fails here, does this error path leave
a dangling substream pointer?

The earlier call to mtk_afe_fe_startup() saves the substream into the
hardware tracking structure at memif->substream. Because we return
immediately on failure without calling mtk_afe_fe_shutdown(), the ALSA core
will destroy the stream without running the DAI .shutdown callback.

Could this lead to a use-after-free if a shared IRQ fires later and tries
to access the freed memif->substream pointer?

[ ... ]

[ ... ]

[ ... ]

+static int mtk_dai_etdm_config(struct mtk_base_afe *afe,
+			       struct snd_pcm_hw_params *params,
+			       struct snd_soc_dai *dai,
+			       int stream)
+{
+	struct an7581_afe_private *afe_priv = afe->platform_priv;
+	struct mtk_dai_etdm_priv *etdm_data = afe_priv->dai_priv[dai->id];
+	unsigned int rate = params_rate(params);
+	unsigned int etdm_rate = an7581_etdm_rate_transform(afe->dev, rate);
+	unsigned int bit_width = params_width(params);
+	unsigned int mask, mask1;
+	unsigned int val, val1;
+
+	dev_dbg(afe->dev, "%s(), stream %d, rate %u, bitwidth %u\n",
+		__func__, stream, rate, params_width(params));
+
+	/* CON0 */
+	mask = ETDM_SLAVE_MODE | ETDM_BIT_LEN | ETDM_WRD_LEN |
+	       ETDM_FMT | ETDM_CH_NUM;
+	val = FIELD_PREP(ETDM_BIT_LEN, params_width(params) - 1) |
+	      FIELD_PREP(ETDM_WRD_LEN, get_etdm_wlen(bit_width) - 1) |
+	      FIELD_PREP(ETDM_FMT, etdm_data->format) |

[Severity: High]
What happens if a machine driver does not configure the .dai_fmt on the
DAI link?

If the ALSA core never invokes mtk_dai_etdm_set_fmt(), the pointer at
afe_priv->dai_priv[dai->id] remains NULL. When a stream is opened and
mtk_dai_etdm_config() runs, it appears this code will blindly fetch and
dereference etdm_data->format.

Can this cause a kernel panic due to a NULL pointer dereference?

[Severity: Medium]
Are the clock inversion settings being silently ignored here?

The mtk_dai_etdm_set_fmt() function successfully parses the
SND_SOC_DAIFMT_INV_MASK flags and stores them in etdm_data->bck_inv and
etdm_data->lrck_inv. However, they don't seem to be applied when
programming the ETDM_IN1_CON4 or ETDM_OUT1_CON5 registers in this
configuration step.

+	      FIELD_PREP(ETDM_CH_NUM,
+			 get_etdm_ch_fixup(params_channels(params)) - 1);

[ ... ]

+static int mtk_dai_etdm_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
+{
+	struct mtk_base_afe *afe = snd_soc_dai_get_drvdata(dai);
+	struct an7581_afe_private *afe_priv = afe->platform_priv;
+	struct mtk_dai_etdm_priv *etdm_data;
+	void *priv_data;
+
+	priv_data = devm_kzalloc(afe->dev, sizeof(struct mtk_dai_etdm_priv),
+				 GFP_KERNEL);
+	if (!priv_data)
+		return -ENOMEM;
+
+	afe_priv->dai_priv[dai->id] = priv_data;

[Severity: Medium]
Will this cause a memory leak if the DAI format is set multiple times?

A machine driver might configure the format dynamically or multiple times
during stream restarts. Because this unconditionally allocates memory with
devm_kzalloc() and overwrites the shared afe_priv->dai_priv[dai->id]
pointer, it seems previous allocations will remain leaked for the lifespan
of the device.

+	etdm_data = afe_priv->dai_priv[dai->id];
+
+	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908090448.3332-1-ansuelsmth@gmail.com?part=4