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