Re: [PATCH v1 1/1] ASoC: dpcm: prevent snd_soc_dpcm use after free

[Greg KH]

On Fri, Jan 21, 2022 at 11:16:44PM +0000, Will McVicker wrote:
> From: KaiChieh Chuang <kaichieh.chuang@xxxxxxxxxxxx>
> 
> [ Upstream commit a9764869779081e8bf24da07ac040e8f3efcf13a ]
> 
> The dpcm get from fe_clients/be_clients
> may be free before use
> 
> Add a spin lock at snd_soc_card level,
> to protect the dpcm instance.
> The lock may be used in atomic context, so use spin lock.
> 
> Use irq spin lock version,
> since the lock may be used in interrupts.
> 
> possible race condition between
> void dpcm_be_disconnect(
> 	...
> 	list_del(&dpcm->list_be);
> 	list_del(&dpcm->list_fe);
> 	kfree(dpcm);
> 	...
> 
> and
> 	for_each_dpcm_fe()
> 	for_each_dpcm_be*()
> 
> race condition example
> Thread 1:
>     snd_soc_dapm_mixer_update_power()
>         -> soc_dpcm_runtime_update()
>             -> dpcm_be_disconnect()
>                 -> kfree(dpcm);
> Thread 2:
>     dpcm_fe_dai_trigger()
>         -> dpcm_be_dai_trigger()
>             -> snd_soc_dpcm_can_be_free_stop()
>                 -> if (dpcm->fe == fe)
> 
> Excpetion Scenario:
> 	two FE link to same BE
> 	FE1 -> BE
> 	FE2 ->
> 
> 	Thread 1: switch of mixer between FE2 -> BE
> 	Thread 2: pcm_stop FE1
> 
> Exception:
> 
> Unable to handle kernel paging request at virtual address dead0000000000e0
> 
> pc=<> [<ffffff8960e2cd10>] dpcm_be_dai_trigger+0x29c/0x47c
> 	sound/soc/soc-pcm.c:3226
> 		if (dpcm->fe == fe)
> lr=<> [<ffffff8960e2f694>] dpcm_fe_dai_do_trigger+0x94/0x26c
> 
> Backtrace:
> [<ffffff89602dba80>] notify_die+0x68/0xb8
> [<ffffff896028c7dc>] die+0x118/0x2a8
> [<ffffff89602a2f84>] __do_kernel_fault+0x13c/0x14c
> [<ffffff89602a27f4>] do_translation_fault+0x64/0xa0
> [<ffffff8960280cf8>] do_mem_abort+0x4c/0xd0
> [<ffffff8960282ad0>] el1_da+0x24/0x40
> [<ffffff8960e2cd10>] dpcm_be_dai_trigger+0x29c/0x47c
> [<ffffff8960e2f694>] dpcm_fe_dai_do_trigger+0x94/0x26c
> [<ffffff8960e2edec>] dpcm_fe_dai_trigger+0x3c/0x44
> [<ffffff8960de5588>] snd_pcm_do_stop+0x50/0x5c
> [<ffffff8960dded24>] snd_pcm_action+0xb4/0x13c
> [<ffffff8960ddfdb4>] snd_pcm_drop+0xa0/0x128
> [<ffffff8960de69bc>] snd_pcm_common_ioctl+0x9d8/0x30f0
> [<ffffff8960de1cac>] snd_pcm_ioctl_compat+0x29c/0x2f14
> [<ffffff89604c9d60>] compat_SyS_ioctl+0x128/0x244
> [<ffffff8960283740>] el0_svc_naked+0x34/0x38
> [<ffffffffffffffff>] 0xffffffffffffffff
> 
> Signed-off-by: KaiChieh Chuang <kaichieh.chuang@xxxxxxxxxxxx>
> Signed-off-by: Mark Brown <broonie@xxxxxxxxxx>
> [willmcvicker: move spinlock to bottom of struct snd_soc_card]
> Signed-off-by: Will McVicker <willmcvicker@xxxxxxxxxx>
> Cc: stable@xxxxxxxxxxxxxxx # 4.19+
> ---
>  include/sound/soc.h  |  2 ++
>  sound/soc/soc-core.c |  1 +
>  sound/soc/soc-pcm.c  | 40 +++++++++++++++++++++++++++++++++-------
>  3 files changed, 36 insertions(+), 7 deletions(-)

Now queued up, thanks.

greg k-h