* [PATCH] ASoC: topology: Add missing memory checks
@ 2020-03-20 18:13 Amadeusz Sławiński
2020-03-21 0:18 ` Ranjani Sridharan
0 siblings, 1 reply; 3+ messages in thread
From: Amadeusz Sławiński @ 2020-03-20 18:13 UTC (permalink / raw)
To: Liam Girdwood, Mark Brown, Takashi Iwai
Cc: alsa-devel, Amadeusz Sławiński
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;
}
+ 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 */
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: topology: Add missing memory checks
2020-03-20 18:13 [PATCH] ASoC: topology: Add missing memory checks Amadeusz Sławiński
@ 2020-03-21 0:18 ` Ranjani Sridharan
2020-03-27 18:16 ` Amadeusz Sławiński
0 siblings, 1 reply; 3+ messages in thread
From: Ranjani Sridharan @ 2020-03-21 0:18 UTC (permalink / raw)
To: Amadeusz Sławiński, Liam Girdwood, Mark Brown, Takashi Iwai
Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] ASoC: topology: Add missing memory checks
2020-03-21 0:18 ` Ranjani Sridharan
@ 2020-03-27 18:16 ` Amadeusz Sławiński
0 siblings, 0 replies; 3+ messages in thread
From: Amadeusz Sławiński @ 2020-03-27 18:16 UTC (permalink / raw)
To: Ranjani Sridharan, Liam Girdwood, Mark Brown, Takashi Iwai; +Cc: alsa-devel
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2020-03-27 18:18 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-03-20 18:13 [PATCH] ASoC: topology: Add missing memory checks Amadeusz Sławiński
2020-03-21 0:18 ` Ranjani Sridharan
2020-03-27 18:16 ` Amadeusz Sławiński
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.