Re: [alsa-devel] [RFC PATCH v2] ASoC: dpcm: prevent snd_soc_dpcm use after free

[Jaroslav Kysela <perex-/Fr2/VpizcU@public.gmane.org>]

Dne 06. 03. 19 v 9:46 KaiChieh Chuang napsal(a):
> the dpcm get from fe_clients/be_clients
> may be free before use
> 
> 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 off mixer between FE2 -> BE
> 	Thread 2: pcm_stop FE1
> 
> 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.
> 
> 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-NuS5LvNUpcJWk0Htik3J/w@public.gmane.org>
> ---
>  include/sound/soc.h  |  2 ++
>  sound/soc/soc-core.c |  1 +
>  sound/soc/soc-pcm.c  | 31 ++++++++++++++++++++++++-------
>  3 files changed, 27 insertions(+), 7 deletions(-)
> 
> diff --git a/include/sound/soc.h b/include/sound/soc.h
> index eb7db605955b..1e2be35ed36f 100644
> --- a/include/sound/soc.h
> +++ b/include/sound/soc.h
> @@ -1083,6 +1083,8 @@ struct snd_soc_card {
>  	struct mutex mutex;
>  	struct mutex dapm_mutex;
>  
> +	spinlock_t dpcm_lock;
> +
>  	bool instantiated;
>  	bool topology_shortname_created;
>  
> diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
> index 5a5764dba147..d88757659729 100644
> --- a/sound/soc/soc-core.c
> +++ b/sound/soc/soc-core.c
> @@ -2820,6 +2820,7 @@ int snd_soc_register_card(struct snd_soc_card *card)
>  	card->instantiated = 0;
>  	mutex_init(&card->mutex);
>  	mutex_init(&card->dapm_mutex);
> +	spin_lock_init(&card->dpcm_lock);
>  
>  	return snd_soc_bind_card(card);
>  }
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index a5b40e82dea4..f31eaf58e750 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -1294,9 +1294,11 @@ void dpcm_be_disconnect(struct snd_soc_pcm_runtime *fe, int stream)
>  #ifdef CONFIG_DEBUG_FS
>  		debugfs_remove(dpcm->debugfs_state);
>  #endif
> +		spin_lock(&fe->card->dpcm_lock);
>  		list_del(&dpcm->list_be);
>  		list_del(&dpcm->list_fe);
>  		kfree(dpcm);
> +		spin_unlock(&fe->card->dpcm_lock);

The unlock might be moved before kfree(). Also, I don't see the
list_add() spin lock protection in your patch.

				Jaroslav

>  	}
>  }
>  
> @@ -1548,9 +1550,11 @@ void dpcm_clear_pending_state(struct snd_soc_pcm_runtime *fe, int stream)
>  {
>  	struct snd_soc_dpcm *dpcm;
>  
> +	spin_lock(&fe->card->dpcm_lock);
>  	for_each_dpcm_be(fe, stream, dpcm)
>  		dpcm->be->dpcm[stream].runtime_update =
>  						SND_SOC_DPCM_UPDATE_NO;
> +	spin_unlock(&fe->card->dpcm_lock);
>  }
>  
>  static void dpcm_be_dai_startup_unwind(struct snd_soc_pcm_runtime *fe,
> @@ -2640,11 +2644,13 @@ static int dpcm_run_update_startup(struct snd_soc_pcm_runtime *fe, int stream)
>  	dpcm_be_dai_shutdown(fe, stream);
>  disconnect:
>  	/* disconnect any non started BEs */
> +	spin_lock(&fe->card->dpcm_lock);
>  	for_each_dpcm_be(fe, stream, dpcm) {
>  		struct snd_soc_pcm_runtime *be = dpcm->be;
>  		if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
>  				dpcm->state = SND_SOC_DPCM_LINK_STATE_FREE;
>  	}
> +	spin_unlock(&fe->card->dpcm_lock);
>  
>  	return ret;
>  }
> @@ -3220,7 +3226,9 @@ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
>  {
>  	struct snd_soc_dpcm *dpcm;
>  	int state;
> +	int ret = 1;
>  
> +	spin_lock(&fe->card->dpcm_lock);
>  	for_each_dpcm_fe(be, stream, dpcm) {
>  
>  		if (dpcm->fe == fe)
> @@ -3229,12 +3237,15 @@ int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
>  		state = dpcm->fe->dpcm[stream].state;
>  		if (state == SND_SOC_DPCM_STATE_START ||
>  			state == SND_SOC_DPCM_STATE_PAUSED ||
> -			state == SND_SOC_DPCM_STATE_SUSPEND)
> -			return 0;
> +			state == SND_SOC_DPCM_STATE_SUSPEND) {
> +			ret = 0;
> +			break;
> +		}
>  	}
> +	spin_unlock(&fe->card->dpcm_lock);
>  
>  	/* it's safe to free/stop this BE DAI */
> -	return 1;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_free_stop);
>  
> @@ -3247,7 +3258,9 @@ int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
>  {
>  	struct snd_soc_dpcm *dpcm;
>  	int state;
> +	int ret = 1;
>  
> +	spin_lock(&fe->card->dpcm_lock);
>  	for_each_dpcm_fe(be, stream, dpcm) {
>  
>  		if (dpcm->fe == fe)
> @@ -3257,12 +3270,15 @@ int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
>  		if (state == SND_SOC_DPCM_STATE_START ||
>  			state == SND_SOC_DPCM_STATE_PAUSED ||
>  			state == SND_SOC_DPCM_STATE_SUSPEND ||
> -			state == SND_SOC_DPCM_STATE_PREPARE)
> -			return 0;
> +			state == SND_SOC_DPCM_STATE_PREPARE) {
> +			ret = 0
> +			break;
> +		}
>  	}
> +	spin_unlock(&fe->card->dpcm_lock);
>  
>  	/* it's safe to change hw_params */
> -	return 1;
> +	return ret;
>  }
>  EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_params);
>  
> @@ -3328,6 +3344,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
>  		goto out;
>  	}
>  
> +	spin_lock(&fe->card->dpcm_lock);
>  	for_each_dpcm_be(fe, stream, dpcm) {
>  		struct snd_soc_pcm_runtime *be = dpcm->be;
>  		params = &dpcm->hw_params;
> @@ -3348,7 +3365,7 @@ static ssize_t dpcm_show_state(struct snd_soc_pcm_runtime *fe,
>  				params_channels(params),
>  				params_rate(params));
>  	}
> -
> +	spin_unlock(&fe->card->dpcm_lock);
>  out:
>  	return offset;
>  }
> 


-- 
Jaroslav Kysela <perex-/Fr2/VpizcU@public.gmane.org>
Linux Sound Maintainer; ALSA Project; Red Hat, Inc.