Re: [PATCH] ASoC: topology: Add missing memory checks

[Ranjani Sridharan <ranjani.sridharan@linux.intel.com>]

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