[PATCH v2 0/6] ASoC: topology: Propagate error appropriately
v1: Check if kstrdup succeeded.
v2: Remove unneeded freeing, which is performed in another place by dobj handlers.
Additionally for functions which have return status which was ignored, perform success checks and handle failures in appropriate way.
Amadeusz Sławiński (6): ASoC: topology: Add missing memory checks ASoC: topology: Check return value of soc_tplg_create_tlv ASoC: topology: Check return value of soc_tplg_*_create ASoC: topology: Check soc_tplg_add_route return value ASoC: topology: Check return value of pcm_new_ver ASoC: topology: Check return value of soc_tplg_dai_config
sound/soc/soc-topology.c | 113 ++++++++++++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 25 deletions(-)
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 ---
v2: Remove unneeded freeing, which is performed in another place by dobj handlers.
sound/soc/soc-topology.c | 62 +++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 13 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 1f81cd2d29cf..434e152ac8ee 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; @@ -1860,6 +1870,14 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, return ret; }
+ 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,15 @@ 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 */
The patch
ASoC: topology: Add missing memory checks
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From abc3caac24501008465fdb55c5e89e16d58d5a3d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Fri, 27 Mar 2020 16:47:24 -0400 Subject: [PATCH] ASoC: topology: Add missing memory checks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
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 Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200327204729.397-2-amadeuszx.slawinski@linux.int... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 62 +++++++++++++++++++++++++++++++--------- 1 file changed, 49 insertions(+), 13 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 87f75edba3dc..73fc304c9aca 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; @@ -1860,6 +1870,14 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg, return ret; }
+ 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,15 @@ 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 */
Function soc_tplg_create_tlv can fail, so we should check if it succeded or not and proceed appropriately.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com ---
v2: Added this patch
sound/soc/soc-topology.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 434e152ac8ee..c3811dd66b68 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -894,7 +894,13 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, unsigned int count, }
/* create any TLV data */ - soc_tplg_create_tlv(tplg, &kc, &mc->hdr); + err = soc_tplg_create_tlv(tplg, &kc, &mc->hdr); + if (err < 0) { + dev_err(tplg->dev, "ASoC: failed to create TLV %s\n", + mc->hdr.name); + kfree(sm); + continue; + }
/* pass control to driver for optional further init */ err = soc_tplg_init_kcontrol(tplg, &kc, @@ -1355,7 +1361,13 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create( }
/* create any TLV data */ - soc_tplg_create_tlv(tplg, &kc[i], &mc->hdr); + err = soc_tplg_create_tlv(tplg, &kc[i], &mc->hdr); + if (err < 0) { + dev_err(tplg->dev, "ASoC: failed to create TLV %s\n", + mc->hdr.name); + kfree(sm); + continue; + }
/* pass control to driver for optional further init */ err = soc_tplg_init_kcontrol(tplg, &kc[i],
The patch
ASoC: topology: Check return value of soc_tplg_create_tlv
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 482db55ae87f3749db05810a38b1d618dfd4407c Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Fri, 27 Mar 2020 16:47:25 -0400 Subject: [PATCH] ASoC: topology: Check return value of soc_tplg_create_tlv MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Function soc_tplg_create_tlv can fail, so we should check if it succeded or not and proceed appropriately.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200327204729.397-3-amadeuszx.slawinski@linux.int... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 73fc304c9aca..f37a72aebb5a 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -894,7 +894,13 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, unsigned int count, }
/* create any TLV data */ - soc_tplg_create_tlv(tplg, &kc, &mc->hdr); + err = soc_tplg_create_tlv(tplg, &kc, &mc->hdr); + if (err < 0) { + dev_err(tplg->dev, "ASoC: failed to create TLV %s\n", + mc->hdr.name); + kfree(sm); + continue; + }
/* pass control to driver for optional further init */ err = soc_tplg_init_kcontrol(tplg, &kc, @@ -1355,7 +1361,13 @@ static struct snd_kcontrol_new *soc_tplg_dapm_widget_dmixer_create( }
/* create any TLV data */ - soc_tplg_create_tlv(tplg, &kc[i], &mc->hdr); + err = soc_tplg_create_tlv(tplg, &kc[i], &mc->hdr); + if (err < 0) { + dev_err(tplg->dev, "ASoC: failed to create TLV %s\n", + mc->hdr.name); + kfree(sm); + continue; + }
/* pass control to driver for optional further init */ err = soc_tplg_init_kcontrol(tplg, &kc[i],
Functions soc_tplg_denum_create, soc_tplg_dmixer_create, soc_tplg_dbytes_create can fail, so their return values should be checked and error should be propagated.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com ---
v2: Added this patch
sound/soc/soc-topology.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index c3811dd66b68..860bced933d6 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1124,6 +1124,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { struct snd_soc_tplg_ctl_hdr *control_hdr; + int ret; int i;
if (tplg->pass != SOC_TPLG_PASS_MIXER) { @@ -1152,25 +1153,30 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, case SND_SOC_TPLG_CTL_RANGE: case SND_SOC_TPLG_DAPM_CTL_VOLSW: case SND_SOC_TPLG_DAPM_CTL_PIN: - soc_tplg_dmixer_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dmixer_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_ENUM: case SND_SOC_TPLG_CTL_ENUM_VALUE: case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE: case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT: case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE: - soc_tplg_denum_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_denum_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_BYTES: - soc_tplg_dbytes_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dbytes_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; default: soc_bind_err(tplg, control_hdr, i); return -EINVAL; } + if (ret < 0) { + dev_err(tplg->dev, "ASoC: invalid control\n"); + return ret; + } + }
return 0;
On 3/27/20 3:47 PM, Amadeusz Sławiński wrote:
Functions soc_tplg_denum_create, soc_tplg_dmixer_create, soc_tplg_dbytes_create can fail, so their return values should be checked and error should be propagated.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
v2: Added this patch
sound/soc/soc-topology.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index c3811dd66b68..860bced933d6 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1124,6 +1124,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { struct snd_soc_tplg_ctl_hdr *control_hdr;
int ret; int i;
if (tplg->pass != SOC_TPLG_PASS_MIXER) {
@@ -1152,25 +1153,30 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, case SND_SOC_TPLG_CTL_RANGE: case SND_SOC_TPLG_DAPM_CTL_VOLSW: case SND_SOC_TPLG_DAPM_CTL_PIN:
soc_tplg_dmixer_create(tplg, 1,le32_to_cpu(hdr->payload_size));
ret = soc_tplg_dmixer_create(tplg, 1,le32_to_cpu(hdr->payload_size)); break;
soc_tplg_denum_create(tplg, 1,le32_to_cpu(hdr->payload_size));
ret = soc_tplg_denum_create(tplg, 1,le32_to_cpu(hdr->payload_size)); break;
soc_tplg_dbytes_create(tplg, 1,le32_to_cpu(hdr->payload_size));
ret = soc_tplg_dbytes_create(tplg, 1,le32_to_cpu(hdr->payload_size)); break;if (ret < 0) {dev_err(tplg->dev, "ASoC: invalid control\n");return ret;}
Sounds good, but this happens in a loop, so would all the memory previously allocated by denum/dbytes/dmixer_create leak, or is it freed automatically somewhere else?
}
return 0;
On 3/27/2020 7:56 PM, Pierre-Louis Bossart wrote:
On 3/27/20 3:47 PM, Amadeusz Sławiński wrote:
Functions soc_tplg_denum_create, soc_tplg_dmixer_create, soc_tplg_dbytes_create can fail, so their return values should be checked and error should be propagated.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
v2: Added this patch
sound/soc/soc-topology.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index c3811dd66b68..860bced933d6 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1124,6 +1124,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { struct snd_soc_tplg_ctl_hdr *control_hdr; + int ret; int i; if (tplg->pass != SOC_TPLG_PASS_MIXER) { @@ -1152,25 +1153,30 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, case SND_SOC_TPLG_CTL_RANGE: case SND_SOC_TPLG_DAPM_CTL_VOLSW: case SND_SOC_TPLG_DAPM_CTL_PIN: - soc_tplg_dmixer_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dmixer_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_ENUM: case SND_SOC_TPLG_CTL_ENUM_VALUE: case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE: case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT: case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE: - soc_tplg_denum_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_denum_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_BYTES: - soc_tplg_dbytes_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dbytes_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; default: soc_bind_err(tplg, control_hdr, i); return -EINVAL; } + if (ret < 0) { + dev_err(tplg->dev, "ASoC: invalid control\n"); + return ret; + }
Sounds good, but this happens in a loop, so would all the memory previously allocated by denum/dbytes/dmixer_create leak, or is it freed automatically somewhere else?
Well, now that error is propagated, snd_soc_tplg_component_remove() should be called by snd_soc_tplg_component_load() in case of errors while parsing. From quick look it seems like it should be able to free it up correctly by calling remove_enum/bytes/mixer.
Thanks, Amadeusz
if (tplg->pass != SOC_TPLG_PASS_MIXER) { @@ -1152,25 +1153,30 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, case SND_SOC_TPLG_CTL_RANGE: case SND_SOC_TPLG_DAPM_CTL_VOLSW: case SND_SOC_TPLG_DAPM_CTL_PIN: - soc_tplg_dmixer_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dmixer_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_ENUM: case SND_SOC_TPLG_CTL_ENUM_VALUE: case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE: case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT: case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE: - soc_tplg_denum_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_denum_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_BYTES: - soc_tplg_dbytes_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dbytes_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; default: soc_bind_err(tplg, control_hdr, i); return -EINVAL; } + if (ret < 0) { + dev_err(tplg->dev, "ASoC: invalid control\n"); + return ret; + }
Sounds good, but this happens in a loop, so would all the memory previously allocated by denum/dbytes/dmixer_create leak, or is it freed automatically somewhere else?
Well, now that error is propagated, snd_soc_tplg_component_remove() should be called by snd_soc_tplg_component_load() in case of errors while parsing. From quick look it seems like it should be able to free it up correctly by calling remove_enum/bytes/mixer.
I am not sure what you meant by 'should be called', if it's a recommendation for a future change or a description of the existing behavior. Just to be clear, are you saying the existing code will take care of this error flow or that a new patch is needed?
Sounds good, but this happens in a loop, so would all the memory previously allocated by denum/dbytes/dmixer_create leak, or is it freed automatically somewhere else?
Well, now that error is propagated, snd_soc_tplg_component_remove() should be called by snd_soc_tplg_component_load() in case of errors while parsing. From quick look it seems like it should be able to free it up correctly by calling remove_enum/bytes/mixer.
I am not sure what you meant by 'should be called', if it's a recommendation for a future change or a description of the existing behavior. Just to be clear, are you saying the existing code will take care of this error flow or that a new patch is needed?
Existing code should handle this properly. No new code is needed.
On 3/30/20 11:10 AM, Amadeusz Sławiński wrote:
Sounds good, but this happens in a loop, so would all the memory previously allocated by denum/dbytes/dmixer_create leak, or is it freed automatically somewhere else?
Well, now that error is propagated, snd_soc_tplg_component_remove() should be called by snd_soc_tplg_component_load() in case of errors while parsing. From quick look it seems like it should be able to free it up correctly by calling remove_enum/bytes/mixer.
I am not sure what you meant by 'should be called', if it's a recommendation for a future change or a description of the existing behavior. Just to be clear, are you saying the existing code will take care of this error flow or that a new patch is needed?
Existing code should handle this properly. No new code is needed.
Sounds good, thanks.
The patch
ASoC: topology: Check return value of soc_tplg_*_create
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 2ae548f30d7f6973388fc3769bb3c2f6fd13652b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Fri, 27 Mar 2020 16:47:26 -0400 Subject: [PATCH] ASoC: topology: Check return value of soc_tplg_*_create MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Functions soc_tplg_denum_create, soc_tplg_dmixer_create, soc_tplg_dbytes_create can fail, so their return values should be checked and error should be propagated.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200327204729.397-4-amadeuszx.slawinski@linux.int... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index f37a72aebb5a..3ada769cf823 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1124,6 +1124,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, struct snd_soc_tplg_hdr *hdr) { struct snd_soc_tplg_ctl_hdr *control_hdr; + int ret; int i;
if (tplg->pass != SOC_TPLG_PASS_MIXER) { @@ -1152,25 +1153,30 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, case SND_SOC_TPLG_CTL_RANGE: case SND_SOC_TPLG_DAPM_CTL_VOLSW: case SND_SOC_TPLG_DAPM_CTL_PIN: - soc_tplg_dmixer_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dmixer_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_ENUM: case SND_SOC_TPLG_CTL_ENUM_VALUE: case SND_SOC_TPLG_DAPM_CTL_ENUM_DOUBLE: case SND_SOC_TPLG_DAPM_CTL_ENUM_VIRT: case SND_SOC_TPLG_DAPM_CTL_ENUM_VALUE: - soc_tplg_denum_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_denum_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; case SND_SOC_TPLG_CTL_BYTES: - soc_tplg_dbytes_create(tplg, 1, - le32_to_cpu(hdr->payload_size)); + ret = soc_tplg_dbytes_create(tplg, 1, + le32_to_cpu(hdr->payload_size)); break; default: soc_bind_err(tplg, control_hdr, i); return -EINVAL; } + if (ret < 0) { + dev_err(tplg->dev, "ASoC: invalid control\n"); + return ret; + } + }
return 0;
Function soc_tplg_add_route can propagate error code from callback, we should check its return value and handle fail in correct way.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com ---
v2: Added this patch
sound/soc/soc-topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 860bced933d6..78d10b9b725b 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1284,7 +1284,9 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg, routes[i]->dobj.index = tplg->index; list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
- soc_tplg_add_route(tplg, routes[i]); + ret = soc_tplg_add_route(tplg, routes[i]); + if (ret < 0) + break;
/* add route, but keep going if some fail */ snd_soc_dapm_add_routes(dapm, routes[i], 1);
The patch
ASoC: topology: Check soc_tplg_add_route return value
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From 6856e887eae3efc0fe56899cb3f969fe063171c5 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Fri, 27 Mar 2020 16:47:27 -0400 Subject: [PATCH] ASoC: topology: Check soc_tplg_add_route return value MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Function soc_tplg_add_route can propagate error code from callback, we should check its return value and handle fail in correct way.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200327204729.397-5-amadeuszx.slawinski@linux.int... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 3ada769cf823..cb43994089de 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1284,7 +1284,9 @@ static int soc_tplg_dapm_graph_elems_load(struct soc_tplg *tplg, routes[i]->dobj.index = tplg->index; list_add(&routes[i]->dobj.list, &tplg->comp->dobj_list);
- soc_tplg_add_route(tplg, routes[i]); + ret = soc_tplg_add_route(tplg, routes[i]); + if (ret < 0) + break;
/* add route, but keep going if some fail */ snd_soc_dapm_add_routes(dapm, routes[i], 1);
Function pcm_new_ver can fail, so we should check it's return value and handle possible error.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com ---
v2: Added this patch
sound/soc/soc-topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 78d10b9b725b..dade27588e51 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2135,7 +2135,9 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg, _pcm = pcm; } else { abi_match = false; - pcm_new_ver(tplg, pcm, &_pcm); + ret = pcm_new_ver(tplg, pcm, &_pcm); + if (ret < 0) + return ret; }
/* create the FE DAIs and DAI links */
The patch
ASoC: topology: Check return value of pcm_new_ver
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From b3677fc3d68dd942c92de52f0bd9dd8b472a40e6 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Fri, 27 Mar 2020 16:47:28 -0400 Subject: [PATCH] ASoC: topology: Check return value of pcm_new_ver MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Function pcm_new_ver can fail, so we should check it's return value and handle possible error.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200327204729.397-6-amadeuszx.slawinski@linux.int... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index cb43994089de..818657b06799 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2135,7 +2135,9 @@ static int soc_tplg_pcm_elems_load(struct soc_tplg *tplg, _pcm = pcm; } else { abi_match = false; - pcm_new_ver(tplg, pcm, &_pcm); + ret = pcm_new_ver(tplg, pcm, &_pcm); + if (ret < 0) + return ret; }
/* create the FE DAIs and DAI links */
Function soc_tplg_dai_config can fail, check for and handle possible failure.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com ---
v2: Added this patch
sound/soc/soc-topology.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index dade27588e51..9d6b266ccef8 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2524,7 +2524,7 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg, { struct snd_soc_tplg_dai *dai; int count; - int i; + int i, ret;
count = le32_to_cpu(hdr->count);
@@ -2539,7 +2539,12 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg, return -EINVAL; }
- soc_tplg_dai_config(tplg, dai); + ret = soc_tplg_dai_config(tplg, dai); + if (ret < 0) { + dev_err(tplg->dev, "ASoC: failed to configure DAI\n"); + return ret; + } + tplg->pos += (sizeof(*dai) + le32_to_cpu(dai->priv.size)); }
The patch
ASoC: topology: Check return value of soc_tplg_dai_config
has been applied to the asoc tree at
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git
All being well this means that it will be integrated into the linux-next tree (usually sometime in the next 24 hours) and sent to Linus during the next merge window (or sooner if it is a bug fix), however if problems are discovered then the patch may be dropped or reverted.
You may get further e-mails resulting from automated or manual testing and review of the tree, please engage with people reporting problems and send followup patches addressing any issues that are reported if needed.
If any updates are required or you are submitting further changes they should be sent as incremental updates against current git, existing patches will not be replaced.
Please add any relevant lists and maintainers to the CCs when replying to this mail.
Thanks, Mark
From dd8e871d4e560eeb8d22af82dde91457ad835a63 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Fri, 27 Mar 2020 16:47:29 -0400 Subject: [PATCH] ASoC: topology: Check return value of soc_tplg_dai_config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Function soc_tplg_dai_config can fail, check for and handle possible failure.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com Link: https://lore.kernel.org/r/20200327204729.397-7-amadeuszx.slawinski@linux.int... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 818657b06799..33e8d189ba2f 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2524,7 +2524,7 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg, { struct snd_soc_tplg_dai *dai; int count; - int i; + int i, ret;
count = le32_to_cpu(hdr->count);
@@ -2539,7 +2539,12 @@ static int soc_tplg_dai_elems_load(struct soc_tplg *tplg, return -EINVAL; }
- soc_tplg_dai_config(tplg, dai); + ret = soc_tplg_dai_config(tplg, dai); + if (ret < 0) { + dev_err(tplg->dev, "ASoC: failed to configure DAI\n"); + return ret; + } + tplg->pos += (sizeof(*dai) + le32_to_cpu(dai->priv.size)); }
On 3/27/20 3:47 PM, Amadeusz Sławiński wrote:
v1: Check if kstrdup succeeded.
v2: Remove unneeded freeing, which is performed in another place by dobj handlers.
Additionally for functions which have return status which was ignored, perform success checks and handle failures in appropriate way.
Amadeusz Sławiński (6): ASoC: topology: Add missing memory checks ASoC: topology: Check return value of soc_tplg_create_tlv ASoC: topology: Check return value of soc_tplg_*_create ASoC: topology: Check soc_tplg_add_route return value ASoC: topology: Check return value of pcm_new_ver ASoC: topology: Check return value of soc_tplg_dai_config
Looks good to me
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
We probably want Ranjani to double-check this series though.
sound/soc/soc-topology.c | 113 ++++++++++++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 25 deletions(-)
On 3/30/2020 6:38 PM, Pierre-Louis Bossart wrote:
On 3/27/20 3:47 PM, Amadeusz Sławiński wrote:
v1: Check if kstrdup succeeded.
v2: Remove unneeded freeing, which is performed in another place by dobj handlers.
Additionally for functions which have return status which was ignored, perform success checks and handle failures in appropriate way.
Amadeusz Sławiński (6): ASoC: topology: Add missing memory checks ASoC: topology: Check return value of soc_tplg_create_tlv ASoC: topology: Check return value of soc_tplg_*_create ASoC: topology: Check soc_tplg_add_route return value ASoC: topology: Check return value of pcm_new_ver ASoC: topology: Check return value of soc_tplg_dai_config
Looks good to me
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
We probably want Ranjani to double-check this series though.
Hi Ranjani, can you take another look, I would like for this to get merged before I forget about it ;)
sound/soc/soc-topology.c | 113 ++++++++++++++++++++++++++++++--------- 1 file changed, 88 insertions(+), 25 deletions(-)
Looks good to me
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
We probably want Ranjani to double-check this series though.
Hi Ranjani, can you take another look, I would like for this to get merged before I forget about it ;)
Ranjani provided her Reviewed-by tag on March 30 - likely our emails crossed.
On 4/8/2020 4:20 PM, Pierre-Louis Bossart wrote:
Looks good to me
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
We probably want Ranjani to double-check this series though.
Hi Ranjani, can you take another look, I would like for this to get merged before I forget about it ;)
Ranjani provided her Reviewed-by tag on March 30 - likely our emails crossed.
That's probably what happened, I only asked, because "double-check" comment above caused me to think it may need another look. Thanks, for confirming it's good.
On Wed, 2020-04-08 at 16:46 +0200, Amadeusz Sławiński wrote:
On 4/8/2020 4:20 PM, Pierre-Louis Bossart wrote:
Looks good to me
Reviewed-by: Pierre-Louis Bossart < pierre-louis.bossart@linux.intel.com>
We probably want Ranjani to double-check this series though.
Hi Ranjani, can you take another look, I would like for this to get merged before I forget about it ;)
Ranjani provided her Reviewed-by tag on March 30 - likely our emails crossed.
That's probably what happened, I only asked, because "double-check" comment above caused me to think it may need another look. Thanks, for confirming it's good.
Sorry for the confusion, Amadeusz/Pierre. Yes, I didnt want to reply and add noise as I had already reviewed the series.
Thanks, Ranjani
On Fri, Mar 27, 2020 at 11:40 AM Amadeusz Sławiński < amadeuszx.slawinski@linux.intel.com> wrote:
v1: Check if kstrdup succeeded.
v2: Remove unneeded freeing, which is performed in another place by dobj handlers.
Additionally for functions which have return status which was ignored, perform success checks and handle failures in appropriate way.
Amadeusz Sławiński (6): ASoC: topology: Add missing memory checks ASoC: topology: Check return value of soc_tplg_create_tlv ASoC: topology: Check return value of soc_tplg_*_create ASoC: topology: Check soc_tplg_add_route return value ASoC: topology: Check return value of pcm_new_ver ASoC: topology: Check return value of soc_tplg_dai_config
Thanks, Amadeusz. LGTM Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
participants (5)
-
Amadeusz Sławiński -
Mark Brown -
Pierre-Louis Bossart -
Ranjani Sridharan -
Sridharan, Ranjani