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