[PATCH] ASoC: topology: Perform component check upfront
Function soc_tplg_dbytes_create(), calls soc_tplg_init_kcontrol() to perform additional driver specific initialization. While soc_tplg_init_kcontrol() ensures that component is valid before invoking ops->control_load, there is no such check at the end of soc_tplg_dbytes_create() where list_add() is used.
Also in quite a few places, there is reference of tplg->comp->dapm or tplg->comp->card, without any checks for tplg->comp.
In consequence of the above this may lead to referencing NULL pointer.
This allows for removal of now unnecessary checks.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-topology.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 575da6aba807..66958c57d884 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -251,7 +251,7 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg, { int ret = 0;
- if (tplg->comp && tplg->ops && tplg->ops->vendor_load) + if (tplg->ops && tplg->ops->vendor_load) ret = tplg->ops->vendor_load(tplg->comp, tplg->index, hdr); else { dev_err(tplg->dev, "ASoC: no vendor load callback for ID %d\n", @@ -283,7 +283,7 @@ static int soc_tplg_vendor_load(struct soc_tplg *tplg, static int soc_tplg_widget_load(struct soc_tplg *tplg, struct snd_soc_dapm_widget *w, struct snd_soc_tplg_dapm_widget *tplg_w) { - if (tplg->comp && tplg->ops && tplg->ops->widget_load) + if (tplg->ops && tplg->ops->widget_load) return tplg->ops->widget_load(tplg->comp, tplg->index, w, tplg_w);
@@ -295,7 +295,7 @@ static int soc_tplg_widget_load(struct soc_tplg *tplg, static int soc_tplg_widget_ready(struct soc_tplg *tplg, struct snd_soc_dapm_widget *w, struct snd_soc_tplg_dapm_widget *tplg_w) { - if (tplg->comp && tplg->ops && tplg->ops->widget_ready) + if (tplg->ops && tplg->ops->widget_ready) return tplg->ops->widget_ready(tplg->comp, tplg->index, w, tplg_w);
@@ -307,7 +307,7 @@ static int soc_tplg_dai_load(struct soc_tplg *tplg, struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm, struct snd_soc_dai *dai) { - if (tplg->comp && tplg->ops && tplg->ops->dai_load) + if (tplg->ops && tplg->ops->dai_load) return tplg->ops->dai_load(tplg->comp, tplg->index, dai_drv, pcm, dai);
@@ -318,7 +318,7 @@ static int soc_tplg_dai_load(struct soc_tplg *tplg, static int soc_tplg_dai_link_load(struct soc_tplg *tplg, struct snd_soc_dai_link *link, struct snd_soc_tplg_link_config *cfg) { - if (tplg->comp && tplg->ops && tplg->ops->link_load) + if (tplg->ops && tplg->ops->link_load) return tplg->ops->link_load(tplg->comp, tplg->index, link, cfg);
return 0; @@ -327,7 +327,7 @@ static int soc_tplg_dai_link_load(struct soc_tplg *tplg, /* tell the component driver that all firmware has been loaded in this request */ static void soc_tplg_complete(struct soc_tplg *tplg) { - if (tplg->comp && tplg->ops && tplg->ops->complete) + if (tplg->ops && tplg->ops->complete) tplg->ops->complete(tplg->comp); }
@@ -684,7 +684,7 @@ EXPORT_SYMBOL_GPL(snd_soc_tplg_widget_bind_event); static int soc_tplg_init_kcontrol(struct soc_tplg *tplg, struct snd_kcontrol_new *k, struct snd_soc_tplg_ctl_hdr *hdr) { - if (tplg->comp && tplg->ops && tplg->ops->control_load) + if (tplg->ops && tplg->ops->control_load) return tplg->ops->control_load(tplg->comp, tplg->index, k, hdr);
@@ -1174,7 +1174,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, static int soc_tplg_add_route(struct soc_tplg *tplg, struct snd_soc_dapm_route *route) { - if (tplg->comp && tplg->ops && tplg->ops->dapm_route_load) + if (tplg->ops && tplg->ops->dapm_route_load) return tplg->ops->dapm_route_load(tplg->comp, tplg->index, route);
@@ -2564,7 +2564,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, }
/* pass control to component driver for optional further init */ - if (tplg->comp && tplg->ops && tplg->ops->manifest) + if (tplg->ops && tplg->ops->manifest) ret = tplg->ops->manifest(tplg->comp, tplg->index, _manifest);
if (!abi_match) /* free the duplicated one */ @@ -2736,6 +2736,10 @@ int snd_soc_tplg_component_load(struct snd_soc_component *comp, struct soc_tplg tplg; int ret;
+ /* component needs to exist to keep and reference data while parsing */ + if (!comp) + return -EINVAL; + /* setup parsing context */ memset(&tplg, 0, sizeof(tplg)); tplg.fw = fw;
On Thu, Mar 12, 2020 at 3:14 AM Amadeusz Sławiński < amadeuszx.slawinski@linux.intel.com> wrote:
Function soc_tplg_dbytes_create(), calls soc_tplg_init_kcontrol() to perform additional driver specific initialization. While soc_tplg_init_kcontrol() ensures that component is valid before invoking ops->control_load, there is no such check at the end of soc_tplg_dbytes_create() where list_add() is used.
Also in quite a few places, there is reference of tplg->comp->dapm or tplg->comp->card, without any checks for tplg->comp.
In consequence of the above this may lead to referencing NULL pointer.
This allows for removal of now unnecessary checks.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
sound/soc/soc-topology.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 575da6aba807..66958c57d884 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -251,7 +251,7 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg, { int ret = 0;
if (tplg->comp && tplg->ops && tplg->ops->vendor_load)
if (tplg->ops && tplg->ops->vendor_load) ret = tplg->ops->vendor_load(tplg->comp, tplg->index, hdr); else { dev_err(tplg->dev, "ASoC: no vendor load callback for ID
%d\n", @@ -283,7 +283,7 @@ static int soc_tplg_vendor_load(struct soc_tplg *tplg, static int soc_tplg_widget_load(struct soc_tplg *tplg, struct snd_soc_dapm_widget *w, struct snd_soc_tplg_dapm_widget *tplg_w) {
if (tplg->comp && tplg->ops && tplg->ops->widget_load)
if (tplg->ops && tplg->ops->widget_load) return tplg->ops->widget_load(tplg->comp, tplg->index, w, tplg_w);
@@ -295,7 +295,7 @@ static int soc_tplg_widget_load(struct soc_tplg *tplg, static int soc_tplg_widget_ready(struct soc_tplg *tplg, struct snd_soc_dapm_widget *w, struct snd_soc_tplg_dapm_widget *tplg_w) {
if (tplg->comp && tplg->ops && tplg->ops->widget_ready)
if (tplg->ops && tplg->ops->widget_ready) return tplg->ops->widget_ready(tplg->comp, tplg->index, w, tplg_w);
@@ -307,7 +307,7 @@ static int soc_tplg_dai_load(struct soc_tplg *tplg, struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm, struct snd_soc_dai *dai) {
if (tplg->comp && tplg->ops && tplg->ops->dai_load)
if (tplg->ops && tplg->ops->dai_load) return tplg->ops->dai_load(tplg->comp, tplg->index,
dai_drv, pcm, dai);
@@ -318,7 +318,7 @@ static int soc_tplg_dai_load(struct soc_tplg *tplg, static int soc_tplg_dai_link_load(struct soc_tplg *tplg, struct snd_soc_dai_link *link, struct snd_soc_tplg_link_config *cfg) {
if (tplg->comp && tplg->ops && tplg->ops->link_load)
if (tplg->ops && tplg->ops->link_load) return tplg->ops->link_load(tplg->comp, tplg->index, link,
cfg);
return 0;
@@ -327,7 +327,7 @@ static int soc_tplg_dai_link_load(struct soc_tplg *tplg, /* tell the component driver that all firmware has been loaded in this request */ static void soc_tplg_complete(struct soc_tplg *tplg) {
if (tplg->comp && tplg->ops && tplg->ops->complete)
if (tplg->ops && tplg->ops->complete) tplg->ops->complete(tplg->comp);
}
@@ -684,7 +684,7 @@ EXPORT_SYMBOL_GPL(snd_soc_tplg_widget_bind_event); static int soc_tplg_init_kcontrol(struct soc_tplg *tplg, struct snd_kcontrol_new *k, struct snd_soc_tplg_ctl_hdr *hdr) {
if (tplg->comp && tplg->ops && tplg->ops->control_load)
if (tplg->ops && tplg->ops->control_load) return tplg->ops->control_load(tplg->comp, tplg->index, k, hdr);
@@ -1174,7 +1174,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, static int soc_tplg_add_route(struct soc_tplg *tplg, struct snd_soc_dapm_route *route) {
if (tplg->comp && tplg->ops && tplg->ops->dapm_route_load)
if (tplg->ops && tplg->ops->dapm_route_load) return tplg->ops->dapm_route_load(tplg->comp, tplg->index, route);
@@ -2564,7 +2564,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, }
/* pass control to component driver for optional further init */
if (tplg->comp && tplg->ops && tplg->ops->manifest)
if (tplg->ops && tplg->ops->manifest) ret = tplg->ops->manifest(tplg->comp, tplg->index,
_manifest);
if (!abi_match) /* free the duplicated one */
@@ -2736,6 +2736,10 @@ int snd_soc_tplg_component_load(struct snd_soc_component *comp, struct soc_tplg tplg; int ret;
/* component needs to exist to keep and reference data while
parsing */
if (!comp)
return -EINVAL;
Amadeusz,
Thanks for this change. I agree that the checks for tplg->comp in the above functions should be removed but is the check here really necessary at all? snd_soc_tplg_component_load() is typically called when a component is probed. Can it ever be null at all if it is getting probed?
Thanks, Ranjani
On 3/12/2020 2:57 PM, Sridharan, Ranjani wrote:
On Thu, Mar 12, 2020 at 3:14 AM Amadeusz Sławiński < amadeuszx.slawinski@linux.intel.com> wrote:
Function soc_tplg_dbytes_create(), calls soc_tplg_init_kcontrol() to perform additional driver specific initialization. While soc_tplg_init_kcontrol() ensures that component is valid before invoking ops->control_load, there is no such check at the end of soc_tplg_dbytes_create() where list_add() is used.
Also in quite a few places, there is reference of tplg->comp->dapm or tplg->comp->card, without any checks for tplg->comp.
In consequence of the above this may lead to referencing NULL pointer.
This allows for removal of now unnecessary checks.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
(...)
@@ -2736,6 +2736,10 @@ int snd_soc_tplg_component_load(struct snd_soc_component *comp, struct soc_tplg tplg; int ret;
/* component needs to exist to keep and reference data while
parsing */
if (!comp)
return -EINVAL;
Amadeusz,
Thanks for this change. I agree that the checks for tplg->comp in the above functions should be removed but is the check here really necessary at all? snd_soc_tplg_component_load() is typically called when a component is probed. Can it ever be null at all if it is getting probed?
Thanks, Ranjani
Well it can happen if you pass it wrong pointer for some reason (don't ask :P), I think it's better to have check than none at all. If you pass it NULL pointer to component it can parse part of a file and then suddenly give you NULL pointer dereference in some seemingly "random" function. I would say it's easier for programmer to understand what happens and use such functionality if it performs such check upfront.
Amadeusz
On Thu, Mar 12, 2020 at 7:11 AM Amadeusz Sławiński < amadeuszx.slawinski@linux.intel.com> wrote:
On 3/12/2020 2:57 PM, Sridharan, Ranjani wrote:
On Thu, Mar 12, 2020 at 3:14 AM Amadeusz Sławiński < amadeuszx.slawinski@linux.intel.com> wrote:
Function soc_tplg_dbytes_create(), calls soc_tplg_init_kcontrol() to perform additional driver specific initialization. While soc_tplg_init_kcontrol() ensures that component is valid before invoking ops->control_load, there is no such check at the end of soc_tplg_dbytes_create() where list_add() is used.
Also in quite a few places, there is reference of tplg->comp->dapm or tplg->comp->card, without any checks for tplg->comp.
In consequence of the above this may lead to referencing NULL pointer.
This allows for removal of now unnecessary checks.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
(...)
@@ -2736,6 +2736,10 @@ int snd_soc_tplg_component_load(struct snd_soc_component *comp, struct soc_tplg tplg; int ret;
/* component needs to exist to keep and reference data while
parsing */
if (!comp)
return -EINVAL;
Amadeusz,
Thanks for this change. I agree that the checks for tplg->comp in the
above
functions should be removed but is the check here really necessary at
all?
snd_soc_tplg_component_load() is typically called when a component is probed. Can it ever be null at all if it is getting probed?
Thanks, Ranjani
Well it can happen if you pass it wrong pointer for some reason (don't ask :P), I think it's better to have check than none at all. If you pass it NULL pointer to component it can parse part of a file and then suddenly give you NULL pointer dereference in some seemingly "random" function. I would say it's easier for programmer to understand what happens and use such functionality if it performs such check upfront.
Yes, I suppose it is not a bad idea to have the check for robustness. So,
Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
The patch
ASoC: topology: Perform component check upfront
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 c42464a4e67383d8daeeaa668ff875e399a38105 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Amadeusz=20S=C5=82awi=C5=84ski?= amadeuszx.slawinski@linux.intel.com Date: Thu, 12 Mar 2020 08:22:39 -0400 Subject: [PATCH] ASoC: topology: Perform component check upfront MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit
Function soc_tplg_dbytes_create(), calls soc_tplg_init_kcontrol() to perform additional driver specific initialization. While soc_tplg_init_kcontrol() ensures that component is valid before invoking ops->control_load, there is no such check at the end of soc_tplg_dbytes_create() where list_add() is used.
Also in quite a few places, there is reference of tplg->comp->dapm or tplg->comp->card, without any checks for tplg->comp.
In consequence of the above this may lead to referencing NULL pointer.
This allows for removal of now unnecessary checks.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com Link: https://lore.kernel.org/r/20200312122239.14489-1-amadeuszx.slawinski@linux.i... Signed-off-by: Mark Brown broonie@kernel.org --- sound/soc/soc-topology.c | 22 +++++++++++++--------- 1 file changed, 13 insertions(+), 9 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 22c38df40d5a..c98766957c10 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -251,7 +251,7 @@ static int soc_tplg_vendor_load_(struct soc_tplg *tplg, { int ret = 0;
- if (tplg->comp && tplg->ops && tplg->ops->vendor_load) + if (tplg->ops && tplg->ops->vendor_load) ret = tplg->ops->vendor_load(tplg->comp, tplg->index, hdr); else { dev_err(tplg->dev, "ASoC: no vendor load callback for ID %d\n", @@ -283,7 +283,7 @@ static int soc_tplg_vendor_load(struct soc_tplg *tplg, static int soc_tplg_widget_load(struct soc_tplg *tplg, struct snd_soc_dapm_widget *w, struct snd_soc_tplg_dapm_widget *tplg_w) { - if (tplg->comp && tplg->ops && tplg->ops->widget_load) + if (tplg->ops && tplg->ops->widget_load) return tplg->ops->widget_load(tplg->comp, tplg->index, w, tplg_w);
@@ -295,7 +295,7 @@ static int soc_tplg_widget_load(struct soc_tplg *tplg, static int soc_tplg_widget_ready(struct soc_tplg *tplg, struct snd_soc_dapm_widget *w, struct snd_soc_tplg_dapm_widget *tplg_w) { - if (tplg->comp && tplg->ops && tplg->ops->widget_ready) + if (tplg->ops && tplg->ops->widget_ready) return tplg->ops->widget_ready(tplg->comp, tplg->index, w, tplg_w);
@@ -307,7 +307,7 @@ static int soc_tplg_dai_load(struct soc_tplg *tplg, struct snd_soc_dai_driver *dai_drv, struct snd_soc_tplg_pcm *pcm, struct snd_soc_dai *dai) { - if (tplg->comp && tplg->ops && tplg->ops->dai_load) + if (tplg->ops && tplg->ops->dai_load) return tplg->ops->dai_load(tplg->comp, tplg->index, dai_drv, pcm, dai);
@@ -318,7 +318,7 @@ static int soc_tplg_dai_load(struct soc_tplg *tplg, static int soc_tplg_dai_link_load(struct soc_tplg *tplg, struct snd_soc_dai_link *link, struct snd_soc_tplg_link_config *cfg) { - if (tplg->comp && tplg->ops && tplg->ops->link_load) + if (tplg->ops && tplg->ops->link_load) return tplg->ops->link_load(tplg->comp, tplg->index, link, cfg);
return 0; @@ -327,7 +327,7 @@ static int soc_tplg_dai_link_load(struct soc_tplg *tplg, /* tell the component driver that all firmware has been loaded in this request */ static void soc_tplg_complete(struct soc_tplg *tplg) { - if (tplg->comp && tplg->ops && tplg->ops->complete) + if (tplg->ops && tplg->ops->complete) tplg->ops->complete(tplg->comp); }
@@ -684,7 +684,7 @@ EXPORT_SYMBOL_GPL(snd_soc_tplg_widget_bind_event); static int soc_tplg_init_kcontrol(struct soc_tplg *tplg, struct snd_kcontrol_new *k, struct snd_soc_tplg_ctl_hdr *hdr) { - if (tplg->comp && tplg->ops && tplg->ops->control_load) + if (tplg->ops && tplg->ops->control_load) return tplg->ops->control_load(tplg->comp, tplg->index, k, hdr);
@@ -1174,7 +1174,7 @@ static int soc_tplg_kcontrol_elems_load(struct soc_tplg *tplg, static int soc_tplg_add_route(struct soc_tplg *tplg, struct snd_soc_dapm_route *route) { - if (tplg->comp && tplg->ops && tplg->ops->dapm_route_load) + if (tplg->ops && tplg->ops->dapm_route_load) return tplg->ops->dapm_route_load(tplg->comp, tplg->index, route);
@@ -2563,7 +2563,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, }
/* pass control to component driver for optional further init */ - if (tplg->comp && tplg->ops && tplg->ops->manifest) + if (tplg->ops && tplg->ops->manifest) ret = tplg->ops->manifest(tplg->comp, tplg->index, _manifest);
if (!abi_match) /* free the duplicated one */ @@ -2735,6 +2735,10 @@ int snd_soc_tplg_component_load(struct snd_soc_component *comp, struct soc_tplg tplg; int ret;
+ /* component needs to exist to keep and reference data while parsing */ + if (!comp) + return -EINVAL; + /* setup parsing context */ memset(&tplg, 0, sizeof(tplg)); tplg.fw = fw;
participants (3)
-
Amadeusz Sławiński
-
Mark Brown
-
Sridharan, Ranjani