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

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

[Gyeongtaek Lee]

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(-)

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

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

[Takashi Iwai]

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
> 
> 
>