| Message ID | 7eca678fa7faa9e160b998192e87220de81315c8.1726847965.git.hns@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: topology: fix stack corruption by code unification for creating standalone and widget controls | expand |
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.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.
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.17347774552974803458.b4-ty@kernel.org/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
On Mon, Sep 23, 2024 at 10:07:48AM +0200, H. Nikolaus Schaller wrote: > 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". Practically speaking the kernel C code assumes that uintptr_t is the same size as unsigned long and there's a whole bunch of porting would be needed if you want to change that. unintptr_t is a relatively new thing compared to the C that the kernel originally targeted.
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index af5d42b57be7e..3d82570293b29 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -889,7 +889,7 @@ static int soc_tplg_dbytes_create(struct soc_tplg *tplg, size_t size) return ret; /* register dynamic object */ - sbe = (struct soc_bytes_ext *)&kc.private_value; + sbe = (struct soc_bytes_ext *)kc.private_value; INIT_LIST_HEAD(&sbe->dobj.list); sbe->dobj.type = SND_SOC_DOBJ_BYTES; @@ -923,7 +923,7 @@ static int soc_tplg_dmixer_create(struct soc_tplg *tplg, size_t size) return ret; /* register dynamic object */ - sm = (struct soc_mixer_control *)&kc.private_value; + sm = (struct soc_mixer_control *)kc.private_value; INIT_LIST_HEAD(&sm->dobj.list); sm->dobj.type = SND_SOC_DOBJ_MIXER;
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> --- 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. 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. [1] According to my research this definition was done before Linux-2.6.12-rc2 sound/soc/soc-topology.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)