Re: [bug report] ASoC: topology: fix endianness issues

[Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>]

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?

> 
>     893                  return -EINVAL;
>     894
>     895          se->dobj.control.dvalues = devm_kzalloc(tplg->dev, le32_to_cpu(ec->items) *
>     896                                             sizeof(u32),
>     897                                             GFP_KERNEL);
>     898          if (!se->dobj.control.dvalues)
>     899                  return -ENOMEM;
>     900
>     901          /* convert from little-endian */
>     902          for (i = 0; i < le32_to_cpu(ec->items); i++) {
>     903                  se->dobj.control.dvalues[i] = le32_to_cpu(ec->values[i]);
>     904          }
>     905
>     906          return 0;
>     907  }
> 
> regards,
> dan carpenter
>