Re: [PATCH v5 1/1] ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()

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