Re: [PATCH] ASoC: topology: fix stack corruption by code unification for creating standalone and widget controls

["H. Nikolaus Schaller" <hns@xxxxxxxxxxxxx>]

Hi,

> Am 23.09.2024 um 09:39 schrieb Amadeusz Sławiński <amadeuszx.slawinski@xxxxxxxxxxxxxxx>:
> 
> On 9/20/2024 5:59 PM, H. Nikolaus Schaller wrote:
>> After finding kernel crashes on omap4/5 aess on PandaES and OMAP5UEVM like
>> [   46.367041] Unable to handle kernel execution of memory at virtual address f164d95c when execute
>> a bisect did initially hint at commit 8f2942b9198c9 ("ASoC: topology: Unify
>> code for creating standalone and widget enum control")
>> Deeper analysis shows a bug in two of the three "ASoC: topology: Unify code"
>> patches. There, a variable is initialized to point into the struct snd_kcontrol_new
>> on stack instead of the newly devm_kzalloc'ed memory.
>> Hence, initialization through struct soc_mixer_control or struct soc_bytes_ext
>> members overwrites stack instead of the intended target address, leading to
>> the observed kernel crashes.
>> Fixes: 8f2942b9198c ("ASoC: topology: Unify code for creating standalone and widget enum control")
>> Fixes: 4654ca7cc8d6 ("ASoC: topology: Unify code for creating standalone and widget mixer control").
>> Fixes: 0867278200f7 ("ASoC: topology: Unify code for creating standalone and widget bytes control").
>> Tested-by: risca@xxxxxxxxxxxxxx
>> Signed-off-by: H. Nikolaus Schaller <hns@xxxxxxxxxxxxx>
>> ---
> 
> There was an earlier patch for same issue which got merged:
> https://lore.kernel.org/linux-sound/172675955522.1842725.17347774552974803458.b4-ty@xxxxxxxxxx/T/#m527c2b9bee99d40d7cd5224cb1d8046dd0528097

Ah, I see. They were just some days faster than me :) Great if it will be fixed asap.

> 
>> Notes:
>>     There seems to be another weakness of all three patches. The initialization
>>     of the kc.private_value is now done after calling soc_tplg_control_load()
>>     which may pass the incompletely initialized control down to some control_load()
>>     operation (if tplg->ops are defined).
>>          Since this feature is not used by the omap4/5 aess subsystem drivers it is
>>     not taken care of by this fix.
>>     
> 
> dobj is internal management detail of topology handling, I'm not convinced end users of topology API should touch it. I would say that I would even be worried that someone touches that, as they may corrupt list etc.

Well, someone may want to inspect values or use the list to see if it is empty.

> 
>>     Another general observation with this code (not related to these patches here)
>>     is that it does not appear to be 64 bit address safe since private_value of
>>     struct snd_kcontrol_new is 'unsigned long' [1] but used to store a pointer.
>>          This is fine on omap4/5 since they are 32 bit machines with 32 bit address
>>     range. A problem would be a machine with 32 bit unsigned long and >32 bit
>>     addresses.
> 
> As far as I know that's exactly the reason why it is unsigned long in kernel, you can store a pointer in it.

You can't store a 64 bit pointer in an 32 bit unsigned int without getting into trouble
on conversion back :) That is what uintptr_t is intended for, but it is also just an
unsigned long in linux/types.h. AFAIK it is defined by C99 as "unsigned integer type
capable of holding a pointer".

BR and thanks,
Nikolaus