ASoC: dpcm: acquire dpcm_lock in dpcm_do_trigger()

Commit Message

Gyeongtaek Lee Dec. 2, 2020, 7:26 a.m. UTC

If stop by underrun and DPCM BE disconnection is run simultaneously,
data abort can be occurred by the sequence below.

/* In core X, running dpcm_be_dai_trigger() */
for_each_dpcm_be(fe, stream, dpcm) {
/* In core Y, running dpcm_be_disconnect() */
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);
/* In core X, running dpcm_be_dai_trigger() */
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_do_trigger().

Signed-off-by: Gyeongtaek Lee <[e-mail]>
Cc: [e-mail]
---
 sound/soc/soc-pcm.c | 62 ++++++++++++++++++++++++++++++++-------------
 1 file changed, 44 insertions(+), 18 deletions(-)


base-commit: fa02fcd94b0c8dff6cc65714510cf25ad194b90d

Comments

Kuninori Morimoto Dec. 2, 2020, 10:33 p.m. UTC | #1

Hi Gyeongtaek

Thank you for your patch

> If stop by underrun and DPCM BE disconnection is run simultaneously,
> data abort can be occurred by the sequence below.
> 
> /* In core X, running dpcm_be_dai_trigger() */
> for_each_dpcm_be(fe, stream, dpcm) {
> /* In core Y, running dpcm_be_disconnect() */
> 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);
> /* In core X, running dpcm_be_dai_trigger() */
> struct snd_soc_pcm_runtime *be = dpcm->be;   <== Accessing freed memory

It is easy to read/understand if this code has alignment.

> To prevent this situation, dpcm_lock should be acquired during
> iteration of dpcm list in dpcm_do_trigger().
> 
> Signed-off-by: Gyeongtaek Lee <[e-mail]>
> Cc: [e-mail]
> ---

Is this bugfix patch for some recent commit ?
If so, having Fixes: tag is helpful.

Thank you for your help !!

Best regards
---
Kuninori Morimoto

Gyeongtaek Lee Dec. 3, 2020, 1:06 a.m. UTC | #2

On 03 Dec 2020 07:33:03 +0900, Kuninori Morimoto wrote:
>Hi Gyeongtaek
>
>Thank you for your patch
>
>> If stop by underrun and DPCM BE disconnection is run simultaneously,
>> data abort can be occurred by the sequence below.
>> 
>> /* In core X, running dpcm_be_dai_trigger() */
>> for_each_dpcm_be(fe, stream, dpcm) {
>> /* In core Y, running dpcm_be_disconnect() */
>> 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);
>> /* In core X, running dpcm_be_dai_trigger() */
>> struct snd_soc_pcm_runtime *be = dpcm->be;   <== Accessing freed memory
>
>It is easy to read/understand if this code has alignment.
>
>> To prevent this situation, dpcm_lock should be acquired during
>> iteration of dpcm list in dpcm_do_trigger().
>> 
>> Signed-off-by: Gyeongtaek Lee <[e-mail]>
>> Cc: [e-mail]
>> ---
>
>Is this bugfix patch for some recent commit ?
>If so, having Fixes: tag is helpful.
>
>Thank you for your help !!
Thank you for your review.
I'll resend the patch v2 after fixing the patch as 
your review comment.

Thanks again.
Gyeongtaek Lee
>
>Best regards
>---
>Kuninori Morimoto
>

Patch

diff --git a/sound/soc/soc-pcm.c b/sound/soc/soc-pcm.c
index dcab9527ba3d..7c5d950a8628 100644
--- a/sound/soc/soc-pcm.c
+++ b/sound/soc/soc-pcm.c
@@ -2073,6 +2073,9 @@  static int dpcm_fe_dai_hw_params(struct snd_pcm_substream *substream,
 	return ret;
 }
 
+static int dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
+		struct snd_soc_pcm_runtime *be, int stream);
+
 static int dpcm_do_trigger(struct snd_soc_dpcm *dpcm,
 		struct snd_pcm_substream *substream, int cmd)
 {
@@ -2092,8 +2095,10 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 			       int cmd)
 {
 	struct snd_soc_dpcm *dpcm;
+	unsigned long flags;
 	int ret = 0;
 
+	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
 	for_each_dpcm_be(fe, stream, dpcm) {
 
 		struct snd_soc_pcm_runtime *be = dpcm->be;
@@ -2113,7 +2118,7 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 
 			ret = dpcm_do_trigger(dpcm, be_substream, cmd);
 			if (ret)
-				return ret;
+				break;
 
 			be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
 			break;
@@ -2123,7 +2128,7 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 
 			ret = dpcm_do_trigger(dpcm, be_substream, cmd);
 			if (ret)
-				return ret;
+				break;
 
 			be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
 			break;
@@ -2133,7 +2138,7 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 
 			ret = dpcm_do_trigger(dpcm, be_substream, cmd);
 			if (ret)
-				return ret;
+				break;
 
 			be->dpcm[stream].state = SND_SOC_DPCM_STATE_START;
 			break;
@@ -2142,12 +2147,12 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 			    (be->dpcm[stream].state != SND_SOC_DPCM_STATE_PAUSED))
 				continue;
 
-			if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+			if (!dpcm_can_be_free_stop(fe, be, stream))
 				continue;
 
 			ret = dpcm_do_trigger(dpcm, be_substream, cmd);
 			if (ret)
-				return ret;
+				break;
 
 			be->dpcm[stream].state = SND_SOC_DPCM_STATE_STOP;
 			break;
@@ -2155,12 +2160,12 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 			if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
 				continue;
 
-			if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+			if (!dpcm_can_be_free_stop(fe, be, stream))
 				continue;
 
 			ret = dpcm_do_trigger(dpcm, be_substream, cmd);
 			if (ret)
-				return ret;
+				break;
 
 			be->dpcm[stream].state = SND_SOC_DPCM_STATE_SUSPEND;
 			break;
@@ -2168,17 +2173,20 @@  int dpcm_be_dai_trigger(struct snd_soc_pcm_runtime *fe, int stream,
 			if (be->dpcm[stream].state != SND_SOC_DPCM_STATE_START)
 				continue;
 
-			if (!snd_soc_dpcm_can_be_free_stop(fe, be, stream))
+			if (!dpcm_can_be_free_stop(fe, be, stream))
 				continue;
 
 			ret = dpcm_do_trigger(dpcm, be_substream, cmd);
 			if (ret)
-				return ret;
+				break;
 
 			be->dpcm[stream].state = SND_SOC_DPCM_STATE_PAUSED;
 			break;
 		}
+		if (ret)
+			break;
 	}
+	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
 
 	return ret;
 }
@@ -2916,10 +2924,9 @@  static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
 	struct snd_soc_dpcm *dpcm;
 	int state;
 	int ret = 1;
-	unsigned long flags;
 	int i;
 
-	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+	lockdep_assert_held(&fe->card->dpcm_lock);
 	for_each_dpcm_fe(be, stream, dpcm) {
 
 		if (dpcm->fe == fe)
@@ -2933,17 +2940,12 @@  static int snd_soc_dpcm_check_state(struct snd_soc_pcm_runtime *fe,
 			}
 		}
 	}
-	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
 
 	/* it's safe to do this BE DAI */
 	return ret;
 }
 
-/*
- * We can only hw_free, stop, pause or suspend a BE DAI if any of it's FE
- * are not running, paused or suspended for the specified stream direction.
- */
-int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
+static int dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
 		struct snd_soc_pcm_runtime *be, int stream)
 {
 	const enum snd_soc_dpcm_state state[] = {
@@ -2954,6 +2956,23 @@  int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
 
 	return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
 }
+
+/*
+ * We can only hw_free, stop, pause or suspend a BE DAI if any of it's FE
+ * are not running, paused or suspended for the specified stream direction.
+ */
+int snd_soc_dpcm_can_be_free_stop(struct snd_soc_pcm_runtime *fe,
+		struct snd_soc_pcm_runtime *be, int stream)
+{
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+	ret =  dpcm_can_be_free_stop(fe, be, stream);
+	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+
+	return ret;
+}
 EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_free_stop);
 
 /*
@@ -2963,6 +2982,9 @@  EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_free_stop);
 int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
 		struct snd_soc_pcm_runtime *be, int stream)
 {
+	unsigned long flags;
+	int ret;
+
 	const enum snd_soc_dpcm_state state[] = {
 		SND_SOC_DPCM_STATE_START,
 		SND_SOC_DPCM_STATE_PAUSED,
@@ -2970,6 +2992,10 @@  int snd_soc_dpcm_can_be_params(struct snd_soc_pcm_runtime *fe,
 		SND_SOC_DPCM_STATE_PREPARE,
 	};
 
-	return snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
+	spin_lock_irqsave(&fe->card->dpcm_lock, flags);
+	ret = snd_soc_dpcm_check_state(fe, be, stream, state, ARRAY_SIZE(state));
+	spin_unlock_irqrestore(&fe->card->dpcm_lock, flags);
+
+	return ret;
 }
 EXPORT_SYMBOL_GPL(snd_soc_dpcm_can_be_params);