On Wed, 2020-12-09 at 17:19 +0100, Amadeusz Sławiński wrote:
> On 12/9/2020 3:30 PM, Pierre-Louis Bossart wrote:
> > Thanks Dan for the bug report.
> >
> > On 12/9/20 12:53 AM, Dan Carpenter wrote:
> > > [ This bug is way older than your patch but you might know the
> > > answer.
> > > -dan ]
> > >
> > > Hello Pierre-Louis Bossart,
> > >
> > > The patch 5aebe7c7f9c2: "ASoC: topology: fix endianness issues"
> > > from
> > > Apr 4, 2019, leads to the following static checker warning:
> > >
> > > sound/soc/soc-topology.c:903 soc_tplg_denum_create_values()
> > > warn: potential pointer math issue ('se-
> > > >dobj.control.dvalues' is
> > > a 64 bit pointer)
> > >
> > > sound/soc/soc-topology.c
> > > 887 static int soc_tplg_denum_create_values(struct soc_tplg
> > > *tplg, struct soc_enum *se,
> > > 888 struct
> > > snd_soc_tplg_enum_control *ec)
> > > 889 {
> > > 890 int i;
> > > 891
> > > 892 if (le32_to_cpu(ec->items) > sizeof(*ec-
> > > >values))
> > >
> > > The warning is from this line where Smatch starts to think that
> > > "ec->items" is the number of bytes, but later in the function we
> > > treat
> > > it was the number of elements.
> > >
> > > I do think probably this should be if:
> > >
> > > if (le32_to_cpu(ec->items) > ARRAY_SIZE(ec->values))
> > > return -EINVAL;
> > >
> > > The ec->values[] is an array of u32:
> > >
> > > __le32 values[SND_SOC_TPLG_NUM_TEXTS *
> > > SNDRV_CTL_ELEM_ID_NAME_MAXLEN / 4];
> > >
> > > so this code will return -EINVAL for anything larger than 4.
> > > Changing
> > > it to ARRAY_SIZE() would raise the limit to 176.
> >
> > I agree with your analysis, even in the initial code the pattern
> > was
> >
> > if (ec->items > sizeof(*ec->values))
> >
> > and that's indeed a clear confusion between number of elements and
> > total
> > number of bytes.
> >
> > Ranjani and Amadeusz are more familiar than me with the topology
> > code,
> > let's see if they concur?
> >
>
> Yes, seems about right, we can also replace devm_kzalloc below with
> devm_kcalloc to make it even more clear that we are dealing with
> array.
>
>
> On related note looking at the UAPI header... I noticed:
>
> struct snd_soc_tplg_enum_control {
> (...)
> char
> texts[SND_SOC_TPLG_NUM_TEXTS][SNDRV_CTL_ELEM_ID_NAME_MAXLEN];
> __le32 values[SND_SOC_TPLG_NUM_TEXTS *
> SNDRV_CTL_ELEM_ID_NAME_MAXLEN / 4];
>
> which means that there is 16 names and then there is an array of
> values,
> which seems somehow oversized compared to names... not sure why we
> multiply it by maximum length of name and then divide...
>
> I guess we could change it to something like:
> __le32 values[SND_SOC_TPLG_NUM_TEXTS];
> __le32 reserved[160];
>
> I'm not sure if we want to change it, but I guess it is something to
> keep in the back of the head. Quite frankly looking at it bit more
> I'm
> starting to think that those values from topology are not used
> anywhere
> after being assigned in the function from which this investigation
> started, simple grep for "dvalues" reports nothing more than
> soc-topology.c file, but I might have missed something.
>
> Another thing which I started wondering about, why we even keep
> those
> values in "se->dobj.control", can't we just use "se" directly, as it
> seems to have required fields.
>
> I guess I will look at it again tomorrow when I have a bit more
> fresh
> head, but I guess for now ARRAY_SIZE and devm_kcalloc change seems
> good
> to me, as the rest will have less to do with "array" type and more
> with
> general logic of parsing things.
Would this be a simpler check than using ARRAY_SIZE?
if (le32_to_cpu(ec->items) > SND_SOC_TPLG_NUM_TEXTS )
return -EINVAL;
Thanks,Ranjani
