[PATCH] ASoC: topology: Perform component check upfront

[Sridharan, Ranjani]

On Thu, Mar 12, 2020 at 7:11 AM Amadeusz Sławiński <
amadeuszx.slawinski at 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 at 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 at 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 at linux.intel.com>