[bug report] ASoC: topology: fix endianness issues

[bug report] ASoC: topology: fix endianness issues

[Dan Carpenter]

[ 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.

   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

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

[Pierre-Louis Bossart]

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
> 

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

[Amadeusz Sławiński]

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.


>>
>>     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
>>

Re: [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

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

[Dan Carpenter]

Ok...  Heh.  This is why I ask about this code instead of assuming that
ancient code with no known bug reports is broken.  Anyway, I'm
definitely not going to be the one to patch this because I am
intimidated now.  :P

regards,
dan carpenter