| Message ID | 1935972447.41614751502383.JavaMail.epsvc@epcpadp3 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger() | expand |
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 > > >
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);
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 --- include/sound/soc-dpcm.h | 5 ++++ sound/soc/soc-pcm.c | 59 +++++++++++++++++++++++++++++++++------- 2 files changed, 54 insertions(+), 10 deletions(-)