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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <3ceac85f-e419-44b1-b04b-1d1cf99a3e87@linux.intel.com>
Date: Mon, 23 Sep 2024 09:39:17 +0200
From: Amadeusz Sławiński
 <amadeuszx.slawinski@...ux.intel.com>
To: "H. Nikolaus Schaller" <hns@...delico.com>,
 Liam Girdwood <lgirdwood@...il.com>, Mark Brown <broonie@...nel.org>,
 Jaroslav Kysela <perex@...ex.cz>, Takashi Iwai <tiwai@...e.com>
Cc: linux-sound@...r.kernel.org, linux-kernel@...r.kernel.org,
 letux-kernel@...nphoenux.org, kernel@...a-handheld.com, risca@...akolonin.se
Subject: Re: [PATCH] ASoC: topology: fix stack corruption by code unification
 for creating standalone and widget controls

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@...akolonin.se
> Signed-off-by: H. Nikolaus Schaller <hns@...delico.com>
> ---
> 

There was an earlier patch for same issue which got merged:
https://lore.kernel.org/linux-sound/172675955522.1842725.17347774552974803458.b4-ty@kernel.org/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.