Re: [PATCH v2 09/21] ASoC: apple: Add macaudio machine driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 10 potential issue(s) to consider:
- [High] ALSA: macaudio: UAF in workqueues due to devres and remove cleanup ordering
- [High] ALSA: macaudio: OOB memory read in macaudio_vlimit_unlock due to pointer underflow
- [High] ALSA: macaudio: ABBA deadlock between controls_rwsem and volume_lock_mutex
- [High] ALSA: macaudio: sleeping in atomic context in macaudio_be_trigger
- [High] ALSA: macaudio: invalid hardware state from mutating params in macaudio_dpcm_hw_params
- [Medium] ALSA: macaudio: spurious speaker volume lock due to race condition in macaudio_slk_put
- [Medium] ALSA: macaudio: OF node reference leak in probe error paths and driver unbind
- [Medium] ALSA: macaudio: NULL pointer dereference of DAI widget streams on malformed DT
- [Low] ALSA: macaudio: missing NULL check before pointer dereference in macaudio_vlimit_unlock
- [Low] ALSA: macaudio: data race on ma->bes_active read
--

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

ASoC: apple: Add macaudio machine driver

Apple Silicon Macs have a complex audio subsystem consisting
of an I2S peripheral (MCA) and multiple codecs of various
models and capabilities. Some machines have a basic mono
speaker with hardware downmix, while others have a very
intricate stereo system consisting of multiple codecs and
drivers per L/R channel. Some machines report voice coil
voltage and current information back to the SoC, and others
do not. All machines have a headset jack.

> diff --git a/sound/soc/apple/macaudio.c b/sound/soc/apple/macaudio.c
> index 0000000000000..36a62d88bb18b
> --- /dev/null
> +++ b/sound/soc/apple/macaudio.c
> @@ -0,0 +1,1699 @@
[ ... ]
> +static void macaudio_vlimit_unlock(struct macaudio_snd_data *ma, bool unlock)
> +{
> +	int ret, max;
> +	struct snd_kcontrol *kctl;
> +	const char *name = volume_control_names[ma->cfg->amp];
> +	int namelen = strlen(name);
> +
> +	if (!name) {
> +		WARN_ON_ONCE(1);
> +		return;
> +	}

[Severity: Low]
Does this code dereference name before checking if it is NULL? The call
to strlen(name) happens just before if (!name).

[ ... ]
> +	/* The caller has already taken the rw semaphore as necessary */
> +	list_for_each_entry(kctl, &ma->card.snd_card->controls, list) {
> +		const char *suffix = kctl->id.name + strlen(kctl->id.name) - namelen;
> +
> +		if (strncmp(suffix, name, namelen))
> +			continue;

[Severity: High]
Can this arithmetic underflow if the kcontrol name is shorter than
namelen? Since strlen() returns an unsigned size_t, subtracting a
larger namelen would result in a very large positive offset, leading
to an out-of-bounds memory read in strncmp().

[ ... ]
> +static void macaudio_vlimit_update(struct macaudio_snd_data *ma, bool take_rwsem)
> +{
[ ... ]
> +	/*
> +	 * 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]
Could this lead to an ABBA deadlock? When a user interacts with the
"Speaker Volume Unlock" kcontrol, the ALSA core takes controls_rwsem
before calling macaudio_slk_put(), which then acquires
volume_lock_mutex.

Here, the worker thread macaudio_vlimit_timeout_work acquires
volume_lock_mutex first and then attempts to acquire controls_rwsem
via macaudio_vlimit_update().

[ ... ]
> +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]
Will this blindly zeroize the timeout if it executes concurrently with a
userspace ping? If macaudio_slk_put() is actively updating the timeout
but the worker has already fired and is waiting on volume_lock_mutex,
the non-synchronous cancel_delayed_work() in macaudio_slk_put()
won't stop it. The worker will then wake up, set
ma->speaker_lock_remain = 0, and lock the volume.

[ ... ]
> +static void macaudio_vlimit_update_work(struct work_struct *wrk)
> +{
> +	struct macaudio_snd_data *ma = container_of(wrk,
> +						    struct macaudio_snd_data, lock_update_work);
> +
> +	if (ma->bes_active)

[Severity: Low]
Is this a data race? ma->bes_active is read here without any locking or
READ_ONCE(), while macaudio_be_trigger() concurrently modifies it.

[ ... ]
> +static int macaudio_parse_of(struct macaudio_snd_data *ma)
> +{
[ ... ]
> +		for (be_index = 0; be_index < num_bes; be_index++) {
> +			/*
> +			 * Set initial link name to be overwritten by a BE-specific
> +			 * name later so that we can use at least use the provisional
> +			 * name in error messages.
> +			 */
> +			link->name = link_name;
> +
> +			ret = macaudio_parse_of_be_dai_link(ma, link, be_index,
> +							    ncodecs_per_cpu, cpu, codec);
> +			if (ret)
> +				goto err_free;
> +
> +			link_props->is_speakers = speakers;
> +			link_props->is_headphones = !speakers;
> +
> +			link_props->codecs = devm_kcalloc(dev, ncodecs_per_cpu,
> +							  sizeof(struct ma_codec_idle), GFP_KERNEL);
> +			if (!link_props->codecs)
> +				return -ENOMEM;

[Severity: Medium]
Does this directly return without cleaning up the OF node references?
Bypassing the err_free label here skips the cleanup logic, leaving the
references acquired in macaudio_parse_of_component() leaked.

Additionally, when taking the err_free path from within this loop,
num_links hasn't been incremented yet for the current iteration, so the
nodes for the currently failing link might not be released.

[ ... ]
> +static int macaudio_dpcm_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params)
> +{
[ ... ]
> +	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]
Does setting rate->min = rate->max = cpu_dai->symmetric_rate mutate
the hardware parameters structure directly during the hw_params phase?
If the speaker BE is not running and symmetric_rate is 0, this forces
the rate to 0 Hz, which may cause downstream hardware drivers to reject
it with -EINVAL and break the stream initialization. Should constraints
like this be applied in the startup callback instead?

[ ... ]
> +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];
> +
> +	guard(mutex)(&ma->be_link_mutex);

[Severity: High]
Can this attempt to acquire a sleeping lock while in an atomic context?
Because these DAI links are not marked with nonatomic = 1, the ASoC and
ALSA cores invoke the .trigger callback with the PCM substream spinlock
held and IRQs disabled.

[ ... ]
> +static int macaudio_add_backend_dai_route(struct snd_soc_card *card, struct snd_soc_dai *dai,
> +					  bool is_speakers)
> +{
> +	struct snd_soc_dapm_route routes[2];
> +	struct snd_soc_dapm_route *r;
> +	int nroutes = 0;
> +	int ret;
> +
> +	memset(routes, 0, sizeof(routes));
> +
> +	dev_dbg(card->dev, "adding routes for '%s'\n", dai->name);
> +
> +	r = &routes[nroutes++];
> +	if (is_speakers)
> +		r->source = "Speaker Playback";
> +	else
> +		r->source = "Headphone Playback";
> +	r->sink = dai->stream[SNDRV_PCM_STREAM_PLAYBACK].widget->name;

[Severity: Medium]
What happens if the configured device tree specifies a capture-only DAI?
Without verifying that the playback stream widget is non-NULL before
dereferencing its name, a malformed device tree could trigger a kernel
panic during device probe.

[ ... ]
> +static int macaudio_slk_put(struct snd_kcontrol *kcontrol, struct snd_ctl_elem_value *uvalue)
> +{
> +	struct snd_soc_card *card = snd_kcontrol_chip(kcontrol);
> +	struct macaudio_snd_data *ma = snd_soc_card_get_drvdata(card);
> +
> +	guard(mutex)(&ma->volume_lock_mutex);

[Severity: High]
Could this result in an ABBA deadlock? As mentioned earlier, this function
is called with controls_rwsem held by the core, and takes
volume_lock_mutex. The background worker acquires volume_lock_mutex
first and then attempts to acquire controls_rwsem.

[ ... ]
> +	cancel_delayed_work(&ma->lock_timeout_work);
> +
> +	ma->speaker_lock_remain = ms_to_ktime(SPEAKER_LOCK_TIMEOUT);
> +	ma->speaker_lock_timeout = ktime_add(ktime_get(), ma->speaker_lock_remain);
> +	macaudio_vlimit_update(ma, false);

[Severity: Medium]
Will this non-synchronous cancel_delayed_work() fail to stop the worker
if it is already running and waiting on volume_lock_mutex? This ties into
the race condition discussed in macaudio_vlimit_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);
> +
> +	return devm_snd_soc_register_card(dev, card);
> +}

[Severity: High]
Does registering the card via devres create a use-after-free window
during unbind? The devres cleanup (including unregistering the card)
runs after macaudio_snd_platform_remove() completes. This leaves a
window where ALSA callbacks can still fire and re-queue workitems on
the workqueues that were cancelled during remove.

[ ... ]
> +static void macaudio_snd_platform_remove(struct platform_device *pdev)
> +{
> +	struct macaudio_snd_data *ma = dev_get_drvdata(&pdev->dev);
> +
> +	cancel_work_sync(&ma->lock_update_work);
> +	cancel_delayed_work_sync(&ma->lock_timeout_work);
> +}

[Severity: High]
Since the sound card is still fully active here (due to devres), can the
work items get re-queued by ongoing ALSA callbacks right after these sync
cancels? When devres later frees macaudio_snd_data, any re-queued work
would execute on freed memory.

[Severity: Medium]
Does this remove handler also leak the OF node references acquired during
probe? It appears to omit the necessary calls to
snd_soc_of_put_dai_link_codecs() and snd_soc_of_put_dai_link_cpus().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-macaudio-v2-0-7b47ed6335a8@gmail.com?part=9