[bug report] ASoC: topology: fix endianness issues

[Ranjani Sridharan]

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