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

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