diff mbox series

ASoC: topology: Add missing memory checks

Message ID 20200320181345.31565-1-amadeuszx.slawinski@linux.intel.com (mailing list archive)
State New, archived
Headers show
Series ASoC: topology: Add missing memory checks | expand

Commit Message

Amadeusz Sławiński March 20, 2020, 6:13 p.m. UTC 
kstrdup is an allocation function and it can fail, so its return value
should be checked and handled appropriately.

In order to check all cases, we need to modify set_stream_info to return
a value, so check that everything went correctly when doing kstrdup().
Later add proper checks and error handlers.

Signed-off-by: Amadeusz Sławiński <amadeuszx.slawinski@linux.intel.com>
---
 sound/soc/soc-topology.c | 65 +++++++++++++++++++++++++++++++---------
 1 file changed, 51 insertions(+), 14 deletions(-)

Comments

Ranjani Sridharan March 21, 2020, 12:18 a.m. UTC | #1 
On Fri, 2020-03-20 at 14:13 -0400, Amadeusz Sławiński wrote:
> kstrdup is an allocation function and it can fail, so its return
> value
> should be checked and handled appropriately.
> 
> In order to check all cases, we need to modify set_stream_info to
> return
> a value, so check that everything went correctly when doing
> kstrdup().
> Later add proper checks and error handlers.
> 
> Signed-off-by: Amadeusz Sławiński <
> amadeuszx.slawinski@linux.intel.com>
> ---
>  sound/soc/soc-topology.c | 65 +++++++++++++++++++++++++++++++-------
> --
>  1 file changed, 51 insertions(+), 14 deletions(-)
> 
> diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
> index 575da6aba807..0bec3ff782c1 100644
> --- a/sound/soc/soc-topology.c
> +++ b/sound/soc/soc-topology.c
> @@ -1766,10 +1766,13 @@ static int soc_tplg_dapm_complete(struct
> soc_tplg *tplg)
>  	return 0;
>  }
>  
> -static void set_stream_info(struct snd_soc_pcm_stream *stream,
> +static int set_stream_info(struct snd_soc_pcm_stream *stream,
>  	struct snd_soc_tplg_stream_caps *caps)
>  {
>  	stream->stream_name = kstrdup(caps->name, GFP_KERNEL);
> +	if (!stream->stream_name)
> +		return -ENOMEM;
> +
>  	stream->channels_min = le32_to_cpu(caps->channels_min);
>  	stream->channels_max = le32_to_cpu(caps->channels_max);
>  	stream->rates = le32_to_cpu(caps->rates);
> @@ -1777,6 +1780,8 @@ static void set_stream_info(struct
> snd_soc_pcm_stream *stream,
>  	stream->rate_max = le32_to_cpu(caps->rate_max);
>  	stream->formats = le64_to_cpu(caps->formats);
>  	stream->sig_bits = le32_to_cpu(caps->sig_bits);
> +
> +	return 0;
>  }
>  
>  static void set_dai_flags(struct snd_soc_dai_driver *dai_drv,
> @@ -1812,20 +1817,29 @@ static int soc_tplg_dai_create(struct
> soc_tplg *tplg,
>  	if (dai_drv == NULL)
>  		return -ENOMEM;
>  
> -	if (strlen(pcm->dai_name))
> +	if (strlen(pcm->dai_name)) {
>  		dai_drv->name = kstrdup(pcm->dai_name, GFP_KERNEL);
> +		if (!dai_drv->name) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +	}
>  	dai_drv->id = le32_to_cpu(pcm->dai_id);
>  
>  	if (pcm->playback) {
>  		stream = &dai_drv->playback;
>  		caps = &pcm->caps[SND_SOC_TPLG_STREAM_PLAYBACK];
> -		set_stream_info(stream, caps);
> +		ret = set_stream_info(stream, caps);
> +		if (ret < 0)
> +			goto err;
>  	}
>  
>  	if (pcm->capture) {
>  		stream = &dai_drv->capture;
>  		caps = &pcm->caps[SND_SOC_TPLG_STREAM_CAPTURE];
> -		set_stream_info(stream, caps);
> +		ret = set_stream_info(stream, caps);
> +		if (ret < 0)
> +			goto err;
>  	}
>  
>  	if (pcm->compress)
> @@ -1835,11 +1849,7 @@ static int soc_tplg_dai_create(struct soc_tplg
> *tplg,
>  	ret = soc_tplg_dai_load(tplg, dai_drv, pcm, NULL);
>  	if (ret < 0) {
>  		dev_err(tplg->comp->dev, "ASoC: DAI loading failed\n");
> -		kfree(dai_drv->playback.stream_name);
> -		kfree(dai_drv->capture.stream_name);
> -		kfree(dai_drv->name);
> -		kfree(dai_drv);
> -		return ret;
> +		goto err;
>  	}
>  
>  	dai_drv->dobj.index = tplg->index;
> @@ -1857,9 +1867,17 @@ static int soc_tplg_dai_create(struct soc_tplg
> *tplg,
>  	if (ret != 0) {
>  		dev_err(dai->dev, "Failed to create DAI widgets %d\n",
> ret);
>  		snd_soc_unregister_dai(dai);
> -		return ret;
> +		goto err;
Hi Amadeusz,

I think this is not needed. Once the dai_drv is added to the dobj_list,
upon a failure here, the tplg components will be removed and this will
be taken care of. So it is safe to just return ret here.
>  	}
>  
> +	return 0;
> +
> +err:
> +	kfree(dai_drv->playback.stream_name);
> +	kfree(dai_drv->capture.stream_name);
> +	kfree(dai_drv->name);
> +	kfree(dai_drv);
> +
>  	return ret;
>  }
>  
> @@ -1916,11 +1934,20 @@ static int soc_tplg_fe_link_create(struct
> soc_tplg *tplg,
>  	if (strlen(pcm->pcm_name)) {
>  		link->name = kstrdup(pcm->pcm_name, GFP_KERNEL);
>  		link->stream_name = kstrdup(pcm->pcm_name, GFP_KERNEL);
> +		if (!link->name || !link->stream_name) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
>  	}
>  	link->id = le32_to_cpu(pcm->pcm_id);
>  
> -	if (strlen(pcm->dai_name))
> +	if (strlen(pcm->dai_name)) {
>  		link->cpus->dai_name = kstrdup(pcm->dai_name,
> GFP_KERNEL);
> +		if (!link->cpus->dai_name) {
> +			ret = -ENOMEM;
> +			goto err;
> +		}
> +	}
>  
>  	link->codecs->name = "snd-soc-dummy";
>  	link->codecs->dai_name = "snd-soc-dummy-dai";
> @@ -2436,13 +2463,17 @@ static int soc_tplg_dai_config(struct
> soc_tplg *tplg,
>  	if (d->playback) {
>  		stream = &dai_drv->playback;
>  		caps = &d->caps[SND_SOC_TPLG_STREAM_PLAYBACK];
> -		set_stream_info(stream, caps);
> +		ret = set_stream_info(stream, caps);
> +		if (ret < 0)
> +			goto err;
>  	}
>  
>  	if (d->capture) {
>  		stream = &dai_drv->capture;
>  		caps = &d->caps[SND_SOC_TPLG_STREAM_CAPTURE];
> -		set_stream_info(stream, caps);
> +		ret = set_stream_info(stream, caps);
> +		if (ret < 0)
> +			goto err;
>  	}
>  
>  	if (d->flag_mask)
> @@ -2454,10 +2485,16 @@ static int soc_tplg_dai_config(struct
> soc_tplg *tplg,
The return value of soc_tplg_dai_config() in soc_tplg_dai_elems_load()
is never checked. So maybe we need a follow-up patch to fix that too?

Thanks,
Ranjani
Amadeusz Sławiński March 27, 2020, 6:16 p.m. UTC | #2 
On 3/21/2020 1:18 AM, Ranjani Sridharan wrote:

(...)

>>   	if (pcm->compress)
>> @@ -1835,11 +1849,7 @@ static int soc_tplg_dai_create(struct soc_tplg
>> *tplg,
>>   	ret = soc_tplg_dai_load(tplg, dai_drv, pcm, NULL);
>>   	if (ret < 0) {
>>   		dev_err(tplg->comp->dev, "ASoC: DAI loading failed\n");
>> -		kfree(dai_drv->playback.stream_name);
>> -		kfree(dai_drv->capture.stream_name);
>> -		kfree(dai_drv->name);
>> -		kfree(dai_drv);
>> -		return ret;
>> +		goto err;
>>   	}
>>   
>>   	dai_drv->dobj.index = tplg->index;
>> @@ -1857,9 +1867,17 @@ static int soc_tplg_dai_create(struct soc_tplg
>> *tplg,
>>   	if (ret != 0) {
>>   		dev_err(dai->dev, "Failed to create DAI widgets %d\n",
>> ret);
>>   		snd_soc_unregister_dai(dai);
>> -		return ret;
>> +		goto err;
> Hi Amadeusz,
> 
> I think this is not needed. Once the dai_drv is added to the dobj_list,
> upon a failure here, the tplg components will be removed and this will
> be taken care of. So it is safe to just return ret here.

Hi,
you are right will do in v2.

(...)
>>   
>>   	if (d->capture) {
>>   		stream = &dai_drv->capture;
>>   		caps = &d->caps[SND_SOC_TPLG_STREAM_CAPTURE];
>> -		set_stream_info(stream, caps);
>> +		ret = set_stream_info(stream, caps);
>> +		if (ret < 0)
>> +			goto err;
>>   	}
>>   
>>   	if (d->flag_mask)
>> @@ -2454,10 +2485,16 @@ static int soc_tplg_dai_config(struct
>> soc_tplg *tplg,
> The return value of soc_tplg_dai_config() in soc_tplg_dai_elems_load()
> is never checked. So maybe we need a follow-up patch to fix that too?
> 

Yes, actually there is few more functions where status is not checked, 
will add checks for them too.

Amadeusz
diff mbox series

Patch

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 575da6aba807..0bec3ff782c1 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1766,10 +1766,13 @@  static int soc_tplg_dapm_complete(struct soc_tplg *tplg)
 	return 0;
 }
 
-static void set_stream_info(struct snd_soc_pcm_stream *stream,
+static int set_stream_info(struct snd_soc_pcm_stream *stream,
 	struct snd_soc_tplg_stream_caps *caps)
 {
 	stream->stream_name = kstrdup(caps->name, GFP_KERNEL);
+	if (!stream->stream_name)
+		return -ENOMEM;
+
 	stream->channels_min = le32_to_cpu(caps->channels_min);
 	stream->channels_max = le32_to_cpu(caps->channels_max);
 	stream->rates = le32_to_cpu(caps->rates);
@@ -1777,6 +1780,8 @@  static void set_stream_info(struct snd_soc_pcm_stream *stream,
 	stream->rate_max = le32_to_cpu(caps->rate_max);
 	stream->formats = le64_to_cpu(caps->formats);
 	stream->sig_bits = le32_to_cpu(caps->sig_bits);
+
+	return 0;
 }
 
 static void set_dai_flags(struct snd_soc_dai_driver *dai_drv,
@@ -1812,20 +1817,29 @@  static int soc_tplg_dai_create(struct soc_tplg *tplg,
 	if (dai_drv == NULL)
 		return -ENOMEM;
 
-	if (strlen(pcm->dai_name))
+	if (strlen(pcm->dai_name)) {
 		dai_drv->name = kstrdup(pcm->dai_name, GFP_KERNEL);
+		if (!dai_drv->name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+	}
 	dai_drv->id = le32_to_cpu(pcm->dai_id);
 
 	if (pcm->playback) {
 		stream = &dai_drv->playback;
 		caps = &pcm->caps[SND_SOC_TPLG_STREAM_PLAYBACK];
-		set_stream_info(stream, caps);
+		ret = set_stream_info(stream, caps);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (pcm->capture) {
 		stream = &dai_drv->capture;
 		caps = &pcm->caps[SND_SOC_TPLG_STREAM_CAPTURE];
-		set_stream_info(stream, caps);
+		ret = set_stream_info(stream, caps);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (pcm->compress)
@@ -1835,11 +1849,7 @@  static int soc_tplg_dai_create(struct soc_tplg *tplg,
 	ret = soc_tplg_dai_load(tplg, dai_drv, pcm, NULL);
 	if (ret < 0) {
 		dev_err(tplg->comp->dev, "ASoC: DAI loading failed\n");
-		kfree(dai_drv->playback.stream_name);
-		kfree(dai_drv->capture.stream_name);
-		kfree(dai_drv->name);
-		kfree(dai_drv);
-		return ret;
+		goto err;
 	}
 
 	dai_drv->dobj.index = tplg->index;
@@ -1857,9 +1867,17 @@  static int soc_tplg_dai_create(struct soc_tplg *tplg,
 	if (ret != 0) {
 		dev_err(dai->dev, "Failed to create DAI widgets %d\n", ret);
 		snd_soc_unregister_dai(dai);
-		return ret;
+		goto err;
 	}
 
+	return 0;
+
+err:
+	kfree(dai_drv->playback.stream_name);
+	kfree(dai_drv->capture.stream_name);
+	kfree(dai_drv->name);
+	kfree(dai_drv);
+
 	return ret;
 }
 
@@ -1916,11 +1934,20 @@  static int soc_tplg_fe_link_create(struct soc_tplg *tplg,
 	if (strlen(pcm->pcm_name)) {
 		link->name = kstrdup(pcm->pcm_name, GFP_KERNEL);
 		link->stream_name = kstrdup(pcm->pcm_name, GFP_KERNEL);
+		if (!link->name || !link->stream_name) {
+			ret = -ENOMEM;
+			goto err;
+		}
 	}
 	link->id = le32_to_cpu(pcm->pcm_id);
 
-	if (strlen(pcm->dai_name))
+	if (strlen(pcm->dai_name)) {
 		link->cpus->dai_name = kstrdup(pcm->dai_name, GFP_KERNEL);
+		if (!link->cpus->dai_name) {
+			ret = -ENOMEM;
+			goto err;
+		}
+	}
 
 	link->codecs->name = "snd-soc-dummy";
 	link->codecs->dai_name = "snd-soc-dummy-dai";
@@ -2436,13 +2463,17 @@  static int soc_tplg_dai_config(struct soc_tplg *tplg,
 	if (d->playback) {
 		stream = &dai_drv->playback;
 		caps = &d->caps[SND_SOC_TPLG_STREAM_PLAYBACK];
-		set_stream_info(stream, caps);
+		ret = set_stream_info(stream, caps);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (d->capture) {
 		stream = &dai_drv->capture;
 		caps = &d->caps[SND_SOC_TPLG_STREAM_CAPTURE];
-		set_stream_info(stream, caps);
+		ret = set_stream_info(stream, caps);
+		if (ret < 0)
+			goto err;
 	}
 
 	if (d->flag_mask)
@@ -2454,10 +2485,16 @@  static int soc_tplg_dai_config(struct soc_tplg *tplg,
 	ret = soc_tplg_dai_load(tplg, dai_drv, NULL, dai);
 	if (ret < 0) {
 		dev_err(tplg->comp->dev, "ASoC: DAI loading failed\n");
-		return ret;
+		goto err;
 	}
 
 	return 0;
+
+err:
+	kfree(dai_drv->playback.stream_name);
+	kfree(dai_drv->capture.stream_name);
+
+	return ret;
 }
 
 /* load physical DAI elements */