Re: [PATCH v4 09/22] ASoC: apple: Add macaudio machine driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 potential issue(s) to consider:
- [High] ASoC: apple: macaudio: AB-BA deadlock in macaudio_vlimit_update()
- [High] ASoC: apple: macaudio: UAF from canceling works after card teardown
- [High] ASoC: apple: macaudio: zeroing rate in macaudio_dpcm_hw_params()
- [High] ASoC: apple: macaudio: sleeping mutex lock in DMA tasklet via nonatomic links
- [Medium] ASoC: apple: macaudio: leak of DAI link OF nodes on unbind and error
- [Medium] ASoC: apple: macaudio: overwritten error in macaudio_be_assign_tdm()
- [Medium] ASoC: apple: macaudio: timeout race with cancel_delayed_work()
--

commit 2cdf88f52057f61693fcf4754466c5850f0fe004
Author: Martin Povišer <povik+lin@cutebit.org>

ASoC: apple: Add macaudio machine driver

This commit adds an ASoC machine driver for Apple Silicon Macs, linking MCA
I2S ports and multiple speaker/headset codecs as DPCM backends to synthetic
frontends, along with speaker volume safety limiting and jack detection.

> diff --git a/sound/soc/apple/macaudio.c b/sound/soc/apple/macaudio.c
> new file mode 100644
> index 0000000000000..2baabf5f3016b
> --- /dev/null
> +++ b/sound/soc/apple/macaudio.c

[ ... ]

> +static void macaudio_vlimit_update(struct macaudio_snd_data *ma, bool take_rwsem)
> +{
> +	bool unlock = true;
> +	const char *reason;
> +
> +	/* Do nothing if there is no safety configured */
> +	if (!ma->has_safety)
> +		return;
> +
> +	/* Check that the control has been pinged within the timeout */
> +	if (ma->speaker_lock_remain <= 0) {
> +		reason = "Lock timeout";
> +		unlock = false;
> +	}
> +
> +	/*
> +	 * If this has been called from either of the safety interlock
> +	 * workqueues, the core API has not taken controls_rwsem for us.
> +	 * Take the write semaphore since macaudio_vlimit_unlock writes
> +	 * to the kcontrols.
> +	 */
> +	if (take_rwsem)
> +		down_write(&ma->card.snd_card->controls_rwsem);

[Severity: High]
As reported by Sashiko on v3 [1], can this lock ordering in
macaudio_vlimit_update() cause an AB-BA deadlock between the ALSA control
write path and the speaker volume-limit workqueue handlers?

When userspace writes to the Speaker Volume Unlock mixer control via
snd_ctl_elem_write(), the ALSA core acquires controls_rwsem for write before
calling macaudio_slk_put(), which then acquires ma->volume_lock_mutex:

  snd_ctl_elem_write()
    down_write(&card->controls_rwsem)
    macaudio_slk_put()
      guard(mutex)(&ma->volume_lock_mutex)

Conversely, when macaudio_vlimit_enable_timeout(),
macaudio_vlimit_disable_timeout(), or macaudio_vlimit_timeout_work() runs from
workqueue context, each function acquires ma->volume_lock_mutex first and then
calls macaudio_vlimit_update(ma, true), which takes controls_rwsem:

  macaudio_vlimit_timeout_work()
    guard(mutex)(&ma->volume_lock_mutex)
    macaudio_vlimit_update(ma, true)
      down_write(&ma->card.snd_card->controls_rwsem)

If a control write races with either workqueue handler, could the two threads
deadlock waiting on each other's lock?

> +
> +	if (unlock != ma->speaker_volume_unlocked) {
> +		if (unlock) {
> +			dev_info(ma->card.dev, "Speaker volumes unlocked\n");
> +		} else  {
> +			dev_info(ma->card.dev, "Speaker volumes locked: %s\n", reason);
> +			ma->speaker_volume_was_locked = true;
> +		}
> +
> +		macaudio_vlimit_unlock(ma, unlock);
> +		ma->speaker_volume_unlocked = unlock;
> +		snd_ctl_notify(ma->card.snd_card, SNDRV_CTL_EVENT_MASK_VALUE,
> +			       &ma->speaker_lock_kctl->id);
> +	}
> +
> +	if (take_rwsem)
> +		up_write(&ma->card.snd_card->controls_rwsem);
> +}

[ ... ]

> +static void macaudio_vlimit_disable_timeout(struct macaudio_snd_data *ma)
> +{
> +	ktime_t now;
> +
> +	guard(mutex)(&ma->volume_lock_mutex);
> +
> +	if (!ma->speaker_lock_timeout_enabled) {
> +		return;
> +	}
> +
> +	now = ktime_get();
> +
> +	cancel_delayed_work(&ma->lock_timeout_work);
> +
> +	if (ktime_after(now, ma->speaker_lock_timeout))
> +		ma->speaker_lock_remain = 0;
> +	else if (ma->speaker_lock_remain > 0)
> +		ma->speaker_lock_remain = ktime_sub(ma->speaker_lock_timeout, now);
> +
> +	dev_dbg(ma->card.dev, "Disabling volume limit timeout: %ld us left\n",
> +		(long)ktime_to_us(ma->speaker_lock_remain));
> +
> +	macaudio_vlimit_update(ma, true);
> +
> +	ma->speaker_lock_timeout_enabled = false;
> +}
> +
> +static void macaudio_vlimit_timeout_work(struct work_struct *wrk)
> +{
> +	struct macaudio_snd_data *ma = container_of(to_delayed_work(wrk),
> +						    struct macaudio_snd_data, lock_timeout_work);
> +
> +	guard(mutex)(&ma->volume_lock_mutex);
> +	ma->speaker_lock_remain = 0;
> +	macaudio_vlimit_update(ma, true);
> +}

[Severity: Medium]
As reported by Sashiko on v3 [2], what happens if lock_timeout_work expires
just as macaudio_slk_put() or macaudio_vlimit_disable_timeout() acquires
ma->volume_lock_mutex?

If macaudio_vlimit_timeout_work() has already been dispatched and is waiting
on guard(mutex)(&ma->volume_lock_mutex), the non-synchronous
cancel_delayed_work(&ma->lock_timeout_work) calls in macaudio_slk_put() and
macaudio_vlimit_disable_timeout() will not stop it.

Once macaudio_slk_put() refreshes ma->speaker_lock_remain and releases
ma->volume_lock_mutex (or macaudio_vlimit_disable_timeout() disables the
timeout and releases the mutex), wouldn't macaudio_vlimit_timeout_work()
immediately acquire ma->volume_lock_mutex, unconditionally zero
ma->speaker_lock_remain, and lock the speaker volumes?

[ ... ]

> +static int macaudio_dpcm_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 macaudio_snd_data *ma = snd_soc_card_get_drvdata(rtd->card);
> +	struct macaudio_link_props *props = &ma->link_props[rtd->dai_link->id];
> +	struct snd_soc_dai *cpu_dai = snd_soc_rtd_to_cpu(rtd, 0);
> +	struct snd_interval *rate = hw_param_interval(params,
> +						      SNDRV_PCM_HW_PARAM_RATE);
> +	int bclk_ratio = macaudio_get_runtime_bclk_ratio(substream);
> +	int i;
> +
> +	if (props->is_sense) {
> +		/*
> +		 * The sense FE is opened exclusively by speakersafetyd in blocking
> +		 * mode. At the time of opening, this sample rate may be 0 if the
> +		 * speaker BE is not running. This does not matter, as speakersafetyd
> +		 * only cares about the active sample rate exported to the virtual
> +		 * sample rate kcontrol below.
> +		 */
> +		rate->min = rate->max = cpu_dai->symmetric_rate;
> +		return 0;
> +	}
> +
> +	/* Speakers BE */
> +	if (props->is_speakers) {
> +		if (substream->stream == SNDRV_PCM_STREAM_CAPTURE) {
> +			/* Sense PCM: keep the existing BE rate (0 if not already running) */
> +			rate->min = rate->max = cpu_dai->symmetric_rate;
> +
> +			return 0;
> +		}

[Severity: High]
As reported by Mark Brown on v1 [3], can setting rate->min = rate->max =
cpu_dai->symmetric_rate on the Speaker Sense capture paths corrupt the PCM
runtime rate and fail when speaker playback is not active?

First, on the Speaker BE capture path (props->is_speakers &&
substream->stream == SNDRV_PCM_STREAM_CAPTURE), if speaker playback is not
running yet, cpu_dai->symmetric_rate is 0. Overwriting the BE hw_params rate
with 0 causes the codec hw_params callback (such as tas2764_hw_params() or
tas2770_hw_params()) to see params_rate(params) == 0 and fail with -EINVAL.

Second, on the Speaker Sense FE path (props->is_sense), macaudio_fe_hw_params()
passes the FE substream's struct snd_pcm_hw_params directly to
macaudio_dpcm_hw_params(). Because the Sense FE's CPU DAI (mca-pcm-2) is
capture-only and distinct from the Speaker BE CPU DAI, its
cpu_dai->symmetric_rate remains 0 even when speaker playback is running.

When dpcm_fe_dai_hw_params() returns to snd_pcm_hw_params(), doesn't
runtime->rate = params_rate(params) set runtime->rate to 0, triggering
snd_BUG_ON(!rate) in snd_pcm_timer_resolution_change() and a division by zero
in snd_pcm_post_start() at (runtime->buffer_size * HZ) / runtime->rate?

[ ... ]

> +static int macaudio_be_trigger(struct snd_pcm_substream *substream, int cmd)
> +{
> +	struct snd_soc_pcm_runtime *rtd = snd_soc_substream_to_rtd(substream);
> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(rtd->card);
> +	struct macaudio_link_props *props = &ma->link_props[rtd->dai_link->id];
> +
> +	/* Speaker link is marked nonatomic */
> +	if (props->is_speakers && substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
> +		scoped_guard(mutex, &ma->be_link_mutex) {

[Severity: High]
As reported by Ajay Kumar Nandam on v2 [4], does marking the Secondary FE,
Speaker Sense FE, and Speaker BE DAI links as nonatomic cause sleeping in
atomic context during DMA completion?

In macaudio_fe_links and macaudio_parse_of(), .nonatomic = 1 is set on these
DAI links, which causes soc_new_pcm() to set pcm->nonatomic = true. However,
the MCA platform driver (sound/soc/apple/mca.c) uses generic dmaengine PCM
with the Apple ADMAC controller (drivers/dma/apple-admac.c), which invokes
dmaengine_pcm_dma_complete() from admac_chan_tasklet() in softirq context:

  admac_chan_tasklet() [softirq]
    dmaengine_pcm_dma_complete()
      snd_pcm_period_elapsed()
        _snd_pcm_stream_lock_irqsave()
          mutex_lock(&substream->self_group.mutex)

Furthermore, if an XRUN or drain stop occurs inside
snd_pcm_period_elapsed_under_stream_lock(), it triggers the PCM stop path from
that same tasklet, which calls macaudio_be_trigger() and acquires
ma->be_link_mutex in softirq context.

Should the DAI links remain atomic and macaudio_be_trigger() avoid sleeping
locks, or how should this interact with ADMAC's tasklet completion path?

> +			switch (cmd) {
> +			case SNDRV_PCM_TRIGGER_START:
> +			case SNDRV_PCM_TRIGGER_RESUME:
> +			case SNDRV_PCM_TRIGGER_PAUSE_RELEASE:
> +				ma->bes_active |= BIT(rtd->dai_link->id);
> +				break;
> +			case SNDRV_PCM_TRIGGER_SUSPEND:
> +			case SNDRV_PCM_TRIGGER_PAUSE_PUSH:
> +			case SNDRV_PCM_TRIGGER_STOP:
> +				ma->bes_active &= ~BIT(rtd->dai_link->id);
> +				break;
> +			default:
> +				return -EINVAL;
> +			}
> +		}
> +
> +		if (ma->has_safety)
> +			schedule_work(&ma->lock_update_work);
> +	}
> +
> +	return 0;
> +}

[ ... ]

> +static int macaudio_be_assign_tdm(struct snd_soc_pcm_runtime *rtd)
> +{
> +	struct snd_soc_card *card = rtd->card;
> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
> +	struct macaudio_link_props *props = &ma->link_props[rtd->dai_link->id];
> +	struct snd_soc_dai *dai;
> +	unsigned int mask;
> +	int nslots, ret, i;
> +
> +	if (!props->tdm_mask)
> +		return 0;
> +
> +	mask = props->tdm_mask;
> +	nslots = __fls(mask) + 1;
> +
> +	if (rtd->dai_link->num_codecs == 1) {
> +		dai = snd_soc_rtd_to_codec(rtd, 0);
> +		ret = snd_soc_dai_set_tdm_slot(dai, mask, 0, nslots, MACAUDIO_SLOTWIDTH);
> +
> +		/*
> +		 * Headphones get a pass on -ENOTSUPP (see the comment
> +		 * around bclk_ratio value for primary FE).
> +		 */
> +		if (ret == -ENOTSUPP && props->is_headphones)
> +			return 0;
> +
> +		if (props->codecs[0].idle_mode) {
> +			ret = snd_soc_dai_set_tdm_idle(dai, props->codecs[0].idle_mask,
> +						       0, props->codecs[0].idle_mode, 0);

[Severity: Medium]
In macaudio_be_assign_tdm(), when rtd->dai_link->num_codecs == 1 and
snd_soc_dai_set_tdm_slot() returns an error other than -ENOTSUPP on headphones,
shouldn't this return ret before checking props->codecs[0].idle_mode?

As written, if props->codecs[0].idle_mode is set, the error in ret from
snd_soc_dai_set_tdm_slot() is overwritten by snd_soc_dai_set_tdm_idle().

[ ... ]

> +static void macaudio_devm_release(void *data)
> +{
> +	struct macaudio_snd_data *ma = data;
> +
> +	cancel_work_sync(&ma->lock_update_work);
> +	cancel_delayed_work_sync(&ma->lock_timeout_work);
> +}
> +
> +static int macaudio_snd_platform_probe(struct platform_device *pdev)
> +{

[ ... ]

> +	INIT_WORK(&data->lock_update_work, macaudio_vlimit_update_work);
> +	INIT_DELAYED_WORK(&data->lock_timeout_work, macaudio_vlimit_timeout_work);
> +
> +	ret = devm_add_action(dev, macaudio_devm_release, data);
> +	if (ret)
> +		return ret;
> +
> +	ret = devm_snd_soc_register_card(dev, &data->card);
> +	if (ret) {
> +		for_each_card_prelinks(&data->card, i, link) {
> +			snd_soc_of_put_dai_link_codecs(link);
> +			snd_soc_of_put_dai_link_cpus(link);
> +		}
> +
> +		return ret;
> +	}

[Severity: High]
As reported by Sashiko on v3 [5], can registering macaudio_devm_release()
before devm_snd_soc_register_card() lead to a use-after-free or NULL pointer
dereference of ma->card.snd_card and ma->speaker_lock_kctl in workqueue
context during card teardown?

Because devres actions run in reverse order on driver unbind (and
devm_snd_soc_bind_card() registers devm_card_bind_release() when the card
binds), soc_cleanup_card_resources() runs before macaudio_devm_release()
cancels lock_update_work and lock_timeout_work. Similarly, if a component
device unbinds while macaudio stays bound, snd_soc_unbind_card() runs
soc_cleanup_card_resources() without triggering macaudio_devm_release() at all.

During soc_cleanup_card_resources(), snd_card_disconnect_sync() stops active
PCM streams, which calls macaudio_be_trigger() with SNDRV_PCM_TRIGGER_STOP and
schedules ma->lock_update_work. soc_cleanup_card_resources() then frees all
kcontrols (including ma->speaker_lock_kctl), frees card->snd_card, and sets
card->snd_card = NULL.

If lock_update_work or lock_timeout_work executes before
macaudio_devm_release() runs, won't macaudio_vlimit_update() and
macaudio_vlimit_unlock() dereference ma->card.snd_card and
ma->speaker_lock_kctl after they have been freed?

[Severity: Medium]
As reported by Sashiko on v3 [6], does macaudio_snd_platform_probe() leak the
backend CPU and CODEC device_node references acquired in
macaudio_parse_of_component() when the driver is later unbound or if
devm_add_action() fails?

In macaudio_snd_platform_probe(), snd_soc_of_put_dai_link_codecs() and
snd_soc_of_put_dai_link_cpus() are only called when
devm_snd_soc_register_card() fails, and are not called in
macaudio_devm_release() or on the devm_add_action() error path.

[1] https://lore.kernel.org/r/20261018-macaudio-v3-8-abdff7741dea@gmail.com
[2] https://lore.kernel.org/r/20261018-macaudio-v3-8-abdff7741dea@gmail.com
[3] https://lore.kernel.org/r/arJMrsl45g2_bLPB@sirena.co.uk
[4] https://lore.kernel.org/r/a166844a-ba68-46b2-b37b-111ce54dbba3@oss.qualcomm.com
[5] https://lore.kernel.org/r/20261018-macaudio-v3-8-abdff7741dea@gmail.com
[6] https://lore.kernel.org/r/20261018-macaudio-v3-8-abdff7741dea@gmail.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103-macaudio-v4-0-637484efe46d@gmail.com?part=9