Hi,
Am 23.09.2024 um 09:39 schrieb Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com:
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@dalakolonin.se Signed-off-by: H. Nikolaus Schaller hns@goldelico.com
There was an earlier patch for same issue which got merged: https://lore.kernel.org/linux-sound/172675955522.1842725.1734777455297480345...
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