linux-kernel - Re: [PATCH] ASoC: soc-card: Fix missing locking in snd_soc_card_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <878r33hf81.wl-tiwai@suse.de>
Date: Thu, 29 Feb 2024 09:00:46 +0100
From: Takashi Iwai <tiwai@...e.de>
To: Richard Fitzgerald <rf@...nsource.cirrus.com>
Cc: <broonie@...nel.org>,
	<shengjiu.wang@...il.com>,
	<Xiubo.Lee@...il.com>,
	<linux-sound@...r.kernel.org>,
	<alsa-devel@...a-project.org>,
	<linux-kernel@...r.kernel.org>,
	<linuxppc-dev@...ts.ozlabs.org>,
	<patches@...nsource.cirrus.com>
Subject: Re: [PATCH] ASoC: soc-card: Fix missing locking in snd_soc_card_get_kcontrol()

On Wed, 21 Feb 2024 13:37:10 +0100,
Richard Fitzgerald wrote:
> 
> snd_soc_card_get_kcontrol() must be holding a read lock on
> card->controls_rwsem while walking the controls list.
> 
> Compare with snd_ctl_find_numid().
> 
> The existing function is renamed snd_soc_card_get_kcontrol_locked()
> so that it can be called from contexts that are already holding
> card->controls_rwsem (for example, control get/put functions).
> 
> There are few direct or indirect callers of
> snd_soc_card_get_kcontrol(), and most are safe. Three require
> changes, which have been included in this patch:
> 
> codecs/cs35l45.c:
>   cs35l45_activate_ctl() is called from a control put() function so
>   is changed to call snd_soc_card_get_kcontrol_locked().
> 
> codecs/cs35l56.c:
>   cs35l56_sync_asp1_mixer_widgets_with_firmware() is called from
>   control get()/put() functions so is changed to call
>   snd_soc_card_get_kcontrol_locked().
> 
> fsl/fsl_xcvr.c:
>   fsl_xcvr_activate_ctl() is called from three places, one of which
>   already holds card->controls_rwsem:
>   1. fsl_xcvr_mode_put(), a control put function, which will
>      already be holding card->controls_rwsem.
>   2. fsl_xcvr_startup(), a DAI startup function.
>   3. fsl_xcvr_shutdown(), a DAI shutdown function.
> 
>   To fix this, fsl_xcvr_activate_ctl() has been changed to call
>   snd_soc_card_get_kcontrol_locked() so that it is safe to call
>   directly from fsl_xcvr_mode_put().
>   The fsl_xcvr_startup() and fsl_xcvr_shutdown() functions have been
>   changed to take a read lock on card->controls_rsem() around calls
>   to fsl_xcvr_activate_ctl(). While this is not very elegant, it
>   keeps the change small, to avoid this patch creating a large
>   collateral churn in fsl/fsl_xcvr.c.
> 
> Analysis of other callers of snd_soc_card_get_kcontrol() is that
> they do not need any changes, they are not holding card->controls_rwsem
> when they call snd_soc_card_get_kcontrol().
> 
> Direct callers of snd_soc_card_get_kcontrol():
>   fsl/fsl_spdif.c: fsl_spdif_dai_probe() - DAI probe function
>   fsl/fsl_micfil.c: voice_detected_fn() - IRQ handler
> 
> Indirect callers via soc_component_notify_control():
>   codecs/cs42l43: cs42l43_mic_shutter() - IRQ handler
>   codecs/cs42l43: cs42l43_spk_shutter() - IRQ handler
>   codecs/ak4118.c: ak4118_irq_handler() - IRQ handler
>   codecs/wm_adsp.c: wm_adsp_write_ctl() - not currently used
> 
> Indirect callers via snd_soc_limit_volume():
>   qcom/sc8280xp.c: sc8280xp_snd_init() - DAIlink init function
>   ti/rx51.c: rx51_aic34_init() - DAI init function
> 
> I don't have hardware to test the fsl/*, qcom/sc828xp.c, ti/rx51.c
> and ak4118.c changes.
> 
> Backport note:
> The fsl/, qcom/, cs35l45, cs35l56 and cs42l43 callers were added
> since the Fixes commit so won't all be present on older kernels.
> 
> Signed-off-by: Richard Fitzgerald <rf@...nsource.cirrus.com>
> Fixes: 209c6cdfd283 ("ASoC: soc-card: move snd_soc_card_get_kcontrol() to soc-card")
> ---
> It would be great if people could test the fsl/, qcom/, ti/rx51 and ak4418
> drivers.

This fix itself looks correct, and I merged Mark's PR now.

But in general, it'd be better to use snd_ctl_find_id() and
snd_ctl_find_id_unlocked() if possible.  Those standard APIs can use
the fast Xarray lookup, and especially for the case like many ASoC
drivers that expose hundreds of kcontrols, the performance gain
becomes significant.

I see that there is no snd_ctl_find_mixer_id_unlocked() variant, but
it should be trivial to add.


thanks,

Takashi