Re: [PATCH v2 7/7] ASoC: Intel: avs: SoundWire PCM BE operations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ASoC: Intel: avs: broken sdw_suspended tracking in pcm_sdw.c
- [High] ASoC: Intel: avs: m_rt leak and UAF in avs_sdw_dai_hw_params()
--

commit 74a0d17712722615078fab6499f5e8443f22cefa
Author: Cezary Rojewski <cezary.rojewski@intel.com>

ASoC: Intel: avs: SoundWire PCM BE operations

This commit implements the back-end PCM operations for SoundWire links in
the Intel AVS driver to support audio data transfer over the SoundWire
interface.

> diff --git a/sound/soc/intel/avs/pcm_sdw.c b/sound/soc/intel/avs/pcm_sdw.c
> new file mode 100644
> index 0000000000000..54caa84fbd75f
> --- /dev/null
> +++ b/sound/soc/intel/avs/pcm_sdw.c

[ ... ]

> +static void avs_sdw_dai_shutdown(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct snd_soc_acpi_mach *mach;
> +	struct snd_soc_dai *codec_dai;
> +	struct avs_dma_data *data;
> +	struct sdw_slave *codec;
> +
> +	codec_dai = snd_soc_rtd_to_codec(rtd, 0);
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +
> +	snd_soc_dai_set_stream(codec_dai, NULL, substream->stream);
> +	sdw_cdns_release_pdi(codec->bus, data->pdi);
> +	sdw_release_stream(data->sdw_stream);
> +	avs_dai_shutdown(substream, dai);
> +}
> +
> +static int avs_sdw_dai_hw_params(struct snd_pcm_substream *substream,
> +				 struct snd_pcm_hw_params *hw_params,
> +				 struct snd_soc_dai *dai)
> +{
> +	struct sdw_stream_config sconfig = {0};
> +	struct sdw_port_config pconfig = {0};
> +	struct snd_soc_acpi_mach *mach;
> +	struct avs_dma_data *data;
> +	struct sdw_cdns_pdi *pdi;
> +	struct sdw_slave *codec;
> +	int ret;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	if (data->path)
> +		return 0;
> +
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +	pdi = data->pdi;
> +	pdi->ch_count = params_channels(hw_params);
> +	pdi->h_ch_num = pdi->ch_count - 1;
> +	pdi->l_ch_num = 0;
> +	sdw_intel_pdi_config(codec->bus, pdi);
> +
> +	if (data->sdw_suspended)
> +		return avs_dai_be_hw_params(substream, hw_params, dai, pdi->intel_alh_id);
> +
> +	sconfig.direction = pdi->dir;
> +	sconfig.ch_count = pdi->ch_count;
> +	sconfig.frame_rate = params_rate(hw_params);
> +	sconfig.type = SDW_STREAM_PCM;
> +	sconfig.bps = snd_pcm_format_width(params_format(hw_params));
> +	pconfig.num = pdi->num;
> +	pconfig.ch_mask = (1 << pdi->ch_count) - 1;
> +
> +	ret = sdw_stream_add_master(codec->bus, &sconfig, &pconfig, 1, data->sdw_stream);
> +	if (ret)
> +		return ret;

[Severity: High]
As reported by Sashiko on v1 [1], can a failure in sdw_stream_add_master()
here leak the struct sdw_master_runtime (m_rt) and leave a dangling pointer to
data->sdw_stream on codec->bus->m_rt_list?

In __soc_pcm_hw_params(), codec DAI hw_params callbacks run before CPU DAI
hw_params callbacks:

__soc_pcm_hw_params()
  -> snd_soc_dai_hw_params(codec_dai)
       -> sdw_stream_add_slave()
            -> sdw_master_rt_alloc()
  -> snd_soc_dai_hw_params(cpu_dai)
       -> avs_sdw_dai_hw_params()
            -> sdw_stream_add_master()

Because sdw_stream_add_slave() already allocated m_rt and linked it onto
data->sdw_stream->master_list and codec->bus->m_rt_list,
sdw_stream_add_master() finds the existing m_rt via sdw_master_rt_find() and
leaves alloc_master_rt as false.

If sdw_master_port_alloc(), sdw_master_rt_config(), sdw_config_stream(), or
sdw_master_port_config() then fails inside sdw_stream_add_master(), m_rt (and
any allocated master ports) is not freed by sdw_stream_add_master(), and
avs_sdw_dai_hw_params() returns early without calling
sdw_stream_remove_master() and with data->path still NULL.

During __soc_pcm_hw_params() error rollback, soc_pcm_hw_clean() calls the
codec DAI hw_free callback (which calls sdw_stream_remove_slave(), freeing
only s_rt and slave ports), while avs_sdw_dai_hw_free() is either skipped or
returns early because data->path is NULL. When avs_sdw_dai_shutdown() later
calls sdw_release_stream(data->sdw_stream), data->sdw_stream is freed while
m_rt remains on codec->bus->m_rt_list with m_rt->stream pointing to freed
memory.

Should avs_sdw_dai_hw_params() call sdw_stream_remove_master() when
sdw_stream_add_master() fails, or should sdw_stream_remove_master() be
guaranteed on teardown even when data->path is NULL?

> +
> +	ret = avs_dai_be_hw_params(substream, hw_params, dai, pdi->intel_alh_id);
> +	if (ret)
> +		sdw_stream_remove_master(codec->bus, data->sdw_stream);
> +	return ret;
> +}
> +
> +static int avs_sdw_dai_hw_free(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_acpi_mach *mach;
> +	struct avs_dma_data *data;
> +	struct sdw_slave *codec;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +	if (!data->path)
> +		return 0;
> +
> +	mach = dev_get_platdata(dai->component->card->dev);
> +	codec = mach->pdata;
> +
> +	avs_path_reset(data->path);
> +	avs_path_free(data->path);
> +	data->path = NULL;
> +	sdw_deprepare_stream(data->sdw_stream);
> +	if (!data->sdw_suspended)
> +		sdw_stream_remove_master(codec->bus, data->sdw_stream);
> +
> +	return 0;
> +}
> +
> +static int avs_sdw_dai_prepare(struct snd_pcm_substream *substream, struct snd_soc_dai *dai)
> +{
> +	struct avs_dma_data *data;
> +	int ret;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +
> +	/*
> +	 * There is no guarantee trigger(RESUME) fires, so reset the flag here.
> +	 * Ensures there is no sdw_master leak should hw_free() occur without resuming.
> +	 */
> +	data->sdw_suspended = false;
> +
> +	ret = sdw_prepare_stream(data->sdw_stream);
> +	if (ret) {
> +		dev_err(dai->dev, "sdw stream prepare failed: %d\n", ret);
> +		return ret;
> +	}
> +
> +	ret = avs_dai_prepare(substream, dai);
> +	if (ret)
> +		sdw_deprepare_stream(data->sdw_stream);
> +	return ret;
> +}
> +
> +static int avs_sdw_dai_trigger(struct snd_pcm_substream *substream, int cmd,
> +			       struct snd_soc_dai *dai)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct avs_dma_data *data;
> +	int ret = 0;
> +
> +	data = snd_soc_dai_get_dma_data(dai, substream);
> +
> +	switch (cmd) {
> +	case SNDRV_PCM_TRIGGER_RESUME:
> +		if (rtd->dai_link->ignore_suspend)
> +			break;
> +		fallthrough;
> +	case SNDRV_PCM_TRIGGER_START:
> +	case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +		ret = sdw_enable_stream(data->sdw_stream);
> +		if (ret)
> +			break;
> +
> +		ret = avs_path_run(data->path, AVS_TPLG_TRIGGER_AUTO);
> +		if (ret)
> +			sdw_disable_stream(data->sdw_stream);
> +		break;
> +
> +	case SNDRV_PCM_TRIGGER_SUSPEND:
> +		if (rtd->dai_link->ignore_suspend)
> +			break;
> +		data->sdw_suspended = true;
> +		fallthrough;

[Severity: High]
As reported by Sashiko on v1 [2], does tracking suspend state via
data->sdw_suspended in avs_sdw_dai_trigger() and clearing it only in
avs_sdw_dai_prepare() break suspend/resume for non-running streams and risk a
use-after-free if resume hw_params fails?

First, during system suspend, snd_pcm_do_suspend() only triggers
SNDRV_PCM_TRIGGER_SUSPEND when snd_pcm_running(substream) is true, and
dpcm_be_dai_trigger() only forwards SNDRV_PCM_TRIGGER_SUSPEND when the BE is
in SND_SOC_DPCM_STATE_START. If an open, configured stream (data->path !=
NULL) is in SNDRV_PCM_STATE_PAUSED, SNDRV_PCM_STATE_PREPARED,
SNDRV_PCM_STATE_SETUP, or SNDRV_PCM_STATE_XRUN at suspend time,
avs_sdw_dai_trigger() is never called and data->sdw_suspended stays false.

When avs_component_suspend() then runs:

avs_component_suspend()
  -> avs_component_suspend_hw_free()
       -> avs_dai_suspend_be_hw_free()
            -> avs_sdw_dai_hw_free()
                 -> sdw_stream_remove_master()

Because data->sdw_suspended is false, avs_sdw_dai_hw_free() calls
sdw_stream_remove_master(), which invokes sdw_master_rt_free() to free both
m_rt and all attached codec slave runtimes (s_rt) and sets
data->sdw_stream->state to SDW_STREAM_RELEASED. On resume,
avs_component_resume() only invokes CPU DAI callbacks (avs_sdw_dai_hw_params()
and avs_sdw_dai_prepare()) without calling the codec DAI hw_params callback.
sdw_stream_add_master() allocates a new m_rt with no slave runtime and leaves
data->sdw_stream->state as SDW_STREAM_RELEASED, causing sdw_prepare_stream()
in avs_sdw_dai_prepare() to fail with -EINVAL and disconnecting the substream.

Second, for a stream that was running at suspend (data->sdw_suspended ==
true), avs_sdw_dai_hw_free() sets data->path = NULL while keeping m_rt on
codec->bus->m_rt_list. On resume, avs_component_resume() runs
avs_component_resume_hw_params() for all BE and FE links before calling
avs_component_resume_prepare(). If avs_dai_be_hw_params() fails in
avs_sdw_dai_hw_params() (leaving data->path NULL) or if FE hw_params fails
before avs_sdw_dai_prepare() is reached (leaving data->sdw_suspended true),
closing the substream causes avs_sdw_dai_hw_free() to skip
sdw_stream_remove_master(), and avs_sdw_dai_shutdown() then frees
data->sdw_stream via sdw_release_stream() while m_rt is still linked on
codec->bus->m_rt_list.

[1] https://lore.kernel.org/r/20261102181042.1922005-8-cezary.rojewski@intel.com
[2] https://lore.kernel.org/r/20261102181042.1922005-8-cezary.rojewski@intel.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134958.2089136-1-cezary.rojewski@intel.com?part=7