ASoC: topology: Perform component check upfront

Commit Message

Amadeusz Sławiński March 12, 2020, 12:22 p.m. UTC

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 <[e-mail]>
---
 sound/soc/soc-topology.c | 22 +++++++++++++---------
 1 file changed, 13 insertions(+), 9 deletions(-)

Comments

Sridharan, Ranjani March 12, 2020, 1:57 p.m. UTC | #1

On Thu, Mar 12, 2020 at 3:14 AM Amadeusz Sławiński <
[e-mail]> 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 <[e-mail]>
> ---
>  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

Amadeusz Sławiński March 12, 2020, 2:11 p.m. UTC | #2

On 3/12/2020 2:57 PM, Sridharan, Ranjani wrote:
> On Thu, Mar 12, 2020 at 3:14 AM Amadeusz Sławiński <
> [e-mail]> 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 <[e-mail]>
>> ---

(...)

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

Sridharan, Ranjani March 12, 2020, 2:14 p.m. UTC | #3

On Thu, Mar 12, 2020 at 7:11 AM Amadeusz Sławiński <
[e-mail]> wrote:

>
>
> On 3/12/2020 2:57 PM, Sridharan, Ranjani wrote:
> > On Thu, Mar 12, 2020 at 3:14 AM Amadeusz Sławiński <
> > [e-mail]> 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 <[e-mail]>
> >> ---
>
> (...)
>
> >> @@ -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 <[e-mail]>

Patch

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;