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

[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

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

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