| Message ID | 1551861979-26601-1-git-send-email-kaichieh.chuang@mediatek.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,v2] ASoC: dpcm: prevent snd_soc_dpcm use after free | expand |
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@mediatek.com> > --- > 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; > } >
On Wed, 2019-03-06 at 10:19 +0100, Jaroslav Kysela wrote: > Dne 06. 03. 19 v 9:46 KaiChieh Chuang napsal(a): > > the dpcm get from fe_clients/be_clients > > may be free before use > > > > @@ -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 > The dpcm_lock in this patch is to protect the instance of dpcm, e.g. protect dpcm not to be free while dereference from the be_clients/fe_clients. The lock is not meant to protect the list "be_clients" and "fe_client", e.g. not meant to protect add/remove dpcm from these list. The lock is added only at the places that may have race with dpcm_be_disconnect(), e.g. kfree(dpcm). And note that, many places that call for_each_dpcm_be/fe() cannot use spin_lock, since there are pcm callbacks which can sleep.
On Wed, Mar 06, 2019 at 10:19:38AM +0100, Jaroslav Kysela wrote: > Dne 06. 03. 19 v 9:46 KaiChieh Chuang napsal(a): > > + 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. Yes, the free *needs* to be outside of the spinlocked section - we shouldn't do dynamic memory operations in atomics context.
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); } } @@ -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; }
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@mediatek.com> --- include/sound/soc.h | 2 ++ sound/soc/soc-core.c | 1 + sound/soc/soc-pcm.c | 31 ++++++++++++++++++++++++------- 3 files changed, 27 insertions(+), 7 deletions(-)