* Re: [PATCH v5 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()
2021-03-03 6:01 ` [PATCH v5 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() Gyeongtaek Lee
@ 2021-03-03 9:46 ` Takashi Iwai
0 siblings, 0 replies; 2+ messages in thread
From: Takashi Iwai @ 2021-03-03 9:46 UTC (permalink / raw)
To: Gyeongtaek Lee
Cc: alsa-devel, khw0178.kim, 'Kuninori Morimoto', kimty,
lgirdwood, 'Pierre-Louis Bossart', broonie, hmseo, cpgs,
donggyun.ko, s47.kang, pilsun.jang, tkjung
On Wed, 03 Mar 2021 07:01:34 +0100,
Gyeongtaek Lee wrote:
>
> If stop by underrun and DPCM BE disconnection is run simultaneously,
> data abort can be occurred by the sequence below.
>
> CPU0 CPU1
> dpcm_be_dai_trigger(): dpcm_be_disconnect():
>
> for_each_dpcm_be(fe, stream, dpcm) {
>
> spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> list_del(&dpcm->list_be);
> list_del(&dpcm->list_fe);
>
> spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
> kfree(dpcm);
>
> struct snd_soc_pcm_runtime *be = dpcm->be; <-- Accessing freed memory
>
> To prevent this situation, dpcm_lock should be acquired during
> iteration of dpcm list in dpcm_be_dai_trigger().
>
> Signed-off-by: Gyeongtaek Lee <gt82.lee@samsung.com>
> Cc: stable@vger.kernel.org
I don't think this addresses the issues fully.
This change fixes the traversal of broken linked list, but it still
does use-after-free, because the BE object is being freed while it's
executed.
Instead, we need a serialization like your previous patch, but in a
different way than the spinlock that can't be used for nonatomic
case.
thanks,
Takashi
> ---
> include/sound/soc-dpcm.h | 5 ++++
> sound/soc/soc-pcm.c | 59 +++++++++++++++++++++++++++++++++-------
> 2 files changed, 54 insertions(+), 10 deletions(-)
>
> diff --git a/include/sound/soc-dpcm.h b/include/sound/soc-dpcm.h
> index 0f6c50b17bba..599cd6054bc3 100644
> --- a/include/sound/soc-dpcm.h
> +++ b/include/sound/soc-dpcm.h
> @@ -103,6 +103,11 @@ struct snd_soc_dpcm_runtime {
> int trigger_pending; /* trigger cmd + 1 if pending, 0 if not */
> };
>
> +struct snd_soc_dpcm_rtd_list {
> + int num_rtds;
> + struct snd_soc_pcm_runtime *rtds[0];
> +};
> +
> #define for_each_dpcm_fe(be, stream, _dpcm) \
> list_for_each_entry(_dpcm, &(be)->dpcm[stream].fe_clients, list_fe)
>
> diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
> index 14d85ca1e435..ccda0f77e827 100644
> --- a/sound/soc/soc-pcm.c
> +++ b/sound/soc/soc-pcm.c
> @@ -2028,15 +2028,51 @@ static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
> return ret;
> }
>
> +static int dpcm_be_list_create(struct snd_soc_pcm_runtime *fe, int stream,
> + struct snd_soc_dpcm_rtd_list **be_list)
> +{
> + struct snd_soc_dpcm *dpcm;
> + unsigned long flags;
> + int size, i, ret = 0;
> +
> + spin_lock_irqsave(&fe->card->dpcm_lock, flags);
> +
> + size = 0;
> + for_each_dpcm_be(fe, stream, dpcm)
> + size++;
> +
> + *be_list = kzalloc(struct_size(*be_list, rtds, size), GFP_ATOMIC);
> + if (*be_list) {
> + i = 0;
> + for_each_dpcm_be(fe, stream, dpcm)
> + (*be_list)->rtds[i++] = dpcm->be;
> +
> + if (i != size)
> + dev_err(fe->dev, "ASoC: abnormal change in be clients: %d -> %d\n",
> + size, i);
> +
> + (*be_list)->num_rtds = i;
> + } else {
> + ret = -ENOMEM;
> + }
> +
> + spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
> +
> + return ret;
> +}
> +
> int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
> int cmd)
> {
> - struct snd_soc_dpcm *dpcm;
> - int ret = 0;
> + struct snd_soc_dpcm_rtd_list *be_list;
> + int i, ret = 0;
>
> - for_each_dpcm_be(fe, stream, dpcm) {
> + ret = dpcm_be_list_create(fe, stream, &be_list);
> + if (ret < 0)
> + return ret;
>
> - struct snd_soc_pcm_runtime *be = dpcm->be;
> + for (i = 0; i < be_list->num_rtds; i++) {
> + struct snd_soc_pcm_runtime *be = be_list->rtds[i];
> struct snd_pcm_substream *be_substream =
> snd_soc_dpcm_get_substream(be, stream);
>
> @@ -2056,7 +2092,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - return ret;
> + break;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
> break;
> @@ -2066,7 +2102,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - return ret;
> + break;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
> break;
> @@ -2076,7 +2112,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - return ret;
> + break;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
> break;
> @@ -2090,7 +2126,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - return ret;
> + break;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
> break;
> @@ -2103,7 +2139,7 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - return ret;
> + break;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
> break;
> @@ -2116,13 +2152,16 @@ int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
>
> ret = soc_pcm_trigger(be_substream, cmd);
> if (ret)
> - return ret;
> + break;
>
> be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
> break;
> }
> + if (ret < 0)
> + break;
> }
>
> + kfree(be_list);
> return ret;
> }
> EXPORT_SYMBOL_GPL(dpcm_be_dai_trigger);
> --
> 2.21.0
>
>
>
^ permalink raw reply [flat|nested] 2+ messages in thread