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
