KASAN use-after-free in snd_soc_unregister_dai() on module unload #2186

plbossart · 2020-06-10T16:25:03Z

Tested with following branch (topic/sof-dev + traces and comments)

https://github.com/plbossart/sound/tree/kasan/hdac_component_use_after_free

sof-dev-defconfig.txt

dmesg-kasan.txt

standard test: boot first with snd-sof-pci blacklisted, then modprobe snd-sof-pci and rmmod snd-sof-pci.

It seems to me that we have the same DAIs declared in multiple places for HDaudio support.

@kv2019i can you reproduce on a plain vanilla HDaudio platform?

plbossart · 2020-06-10T20:57:32Z

I get the same error on a completely different device (Broadwell Chromebook) so this looks like a core issue with our memory management and devm_ handling during module unload

dmesg-kasan-samus.txt

@ranj063 does this ring a bell?

plbossart · 2020-06-10T21:05:14Z

@ranj063 the constant on the two platforms is that the use-after-free happens after dealing with the platform-registered DAIs and before the topology-added ones. So I suspect that removing the topology might do too many things and that the devm_ release deal with data already freed.

plbossart · 2020-06-10T22:10:12Z

This seems to be consistently happening during the list_del operation in snd_soc_unregister_dai.

dmesg-kasan-list-del.txt

plbossart · 2020-06-11T00:33:37Z

Update: the use-after-free happens when using list_del for the last platform-registered DAI, before taking care of the topology-registered DAIs.

[  177.173443] sof-audio-pci 0000:00:1f.3: ASoC: Unregistered DAI 'Alt Analog CPU DAI'
[  177.173449] sof-audio-pci 0000:00:1f.3: plb: snd_soc_unregister_dai before list_del
[  177.173453] ==================================================================
[  177.173486] BUG: KASAN: use-after-free in snd_soc_unregister_dai+0x7c/0xe1 [snd_soc_core]
[  177.173492] Write of size 8 at addr ffff8884409cd1a0 by task rmmod/1421

sinahuang · 2020-06-11T05:25:52Z

Error also can be seen on GLK Chrome with onboard codec DA7219 in I2S mode with kernel https://github.com/plbossart/sound/tree/kasan/hdac_component_use_after_free branch (with original issue kconfig)+master branch (commit:700ca2ae).

[ 1253.632723] sof-audio-pci 0000:00:0e.0: display power disable
[ 1253.639393] ==================================================================
[ 1253.639434] BUG: KASAN: use-after-free in snd_soc_unregister_dai+0x3a/0xb0 [snd_soc_core]
[ 1253.639441] Write of size 8 at addr ffff888154bc00a0 by task rmmod/1908

[ 1253.639453] CPU: 1 PID: 1908 Comm: rmmod Not tainted 5.7.0-rc7-default-validation-0611-pieere-0-c5e14df #rc7
[ 1253.639457] Hardware name: Google Bobba/Bobba, BIOS Google_Bobba.11825.0.2019_03_06_2015 03/06/2019

dmesg.log

bardliao · 2020-06-11T09:46:05Z

Looks like those dai created by soc_tplg_dai_create() -> snd_soc_register_dai() will be freed too early. I won't see the issue if I change devm_kzalloc to kzalloc and devm_kstrdup to kstrdup. I don't understand why only those dais created by topology will be freed too early, but not those dais allocated by the same device.

bardliao · 2020-06-11T12:30:50Z

I think maybe we shouldn't use devm_snd_soc_register_component() if we use topology. The reason is that release_nodes will free all devres in the device in reverse order. If we see devm_snd_soc_register_component(), we can see devres_add() is called after snd_soc_register_component(). That is no problem in normal case since we will unregister ASoC component first, then free other devm_ allocated resources. But, soc_tplg_dai_create()->snd_soc_register_dai() is called after snd_soc_register_component(), and we use devm_ functions to allocate dai and dai->name there. It means they will be freed before ASoC component is unregistered and that's the reason of use-after-free.

The registration of DAIs may be done at two distinct times, once during a component registration and later when loading a topology. Since devm_ managed resources are freed in the reverse order they were allocated, when a component starts unregistering DAIs by walking through the DAI list, the memory allocated for the topology-registered DAIs was freed already, which leads to 100% reproducible KASAN use-after-free reports. This patch suggests a new devm_ function to force the DAI list to be updated prior to freeing the memory chunks referenced by the list pointers. BugLink: thesofproject#2186 Suggested-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use devm_ to avoid use-after-free KASAN reports and simplify error handling. BugLink: thesofproject#2186 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

plbossart · 2020-06-11T20:05:21Z

@sinahuang can you try PR #2188 on GLK? Thanks!

sinahuang · 2020-06-12T03:41:20Z

Error not seen on GLK codec DA7219 in I2S mode with sof-dev+ PR #2188 and master FW(commit:a8ec8b) .

The registration of DAIs may be done at two distinct times, once during a component registration and later when loading a topology. Since devm_ managed resources are freed in the reverse order they were allocated, when a component starts unregistering DAIs by walking through the DAI list, the memory allocated for the topology-registered DAIs was freed already, which leads to 100% reproducible KASAN use-after-free reports. This patch suggests a new devm_ function to force the DAI list to be updated prior to freeing the memory chunks referenced by the list pointers. BugLink: #2186 Suggested-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

Use devm_ to avoid use-after-free KASAN reports and simplify error handling. BugLink: #2186 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>

The registration of DAIs may be done at two distinct times, once during a component registration and later when loading a topology. Since devm_ managed resources are freed in the reverse order they were allocated, when a component starts unregistering DAIs by walking through the DAI list, the memory allocated for the topology-registered DAIs was freed already, which leads to 100% reproducible KASAN use-after-free reports. This patch suggests a new devm_ function to force the DAI list to be updated prior to freeing the memory chunks referenced by the list pointers. BugLink: #2186 Suggested-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Use devm_ to avoid use-after-free KASAN reports and simplify error handling. BugLink: #2186 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

The registration of DAIs may be done at two distinct times, once during a component registration and later when loading a topology. Since devm_ managed resources are freed in the reverse order they were allocated, when a component starts unregistering DAIs by walking through the DAI list, the memory allocated for the topology-registered DAIs was freed already, which leads to 100% reproducible KASAN use-after-free reports. This patch suggests a new devm_ function to force the DAI list to be updated prior to freeing the memory chunks referenced by the list pointers. BugLink: thesofproject#2186 Suggested-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

Use devm_ to avoid use-after-free KASAN reports and simplify error handling. BugLink: thesofproject#2186 Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com>

The registration of DAIs may be done at two distinct times, once during a component registration and later when loading a topology. Since devm_ managed resources are freed in the reverse order they were allocated, when a component starts unregistering DAIs by walking through the DAI list, the memory allocated for the topology-registered DAIs was freed already, which leads to 100% reproducible KASAN use-after-free reports. This patch suggests a new devm_ function to force the DAI list to be updated prior to freeing the memory chunks referenced by the list pointers. Suggested-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> BugLink: thesofproject#2186 Link: https://lore.kernel.org/r/20200612205938.26415-2-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>

Use devm_ to avoid use-after-free KASAN reports and simplify error handling. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> BugLink: thesofproject#2186 Link: https://lore.kernel.org/r/20200612205938.26415-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>

The registration of DAIs may be done at two distinct times, once during a component registration and later when loading a topology. Since devm_ managed resources are freed in the reverse order they were allocated, when a component starts unregistering DAIs by walking through the DAI list, the memory allocated for the topology-registered DAIs was freed already, which leads to 100% reproducible KASAN use-after-free reports. This patch suggests a new devm_ function to force the DAI list to be updated prior to freeing the memory chunks referenced by the list pointers. Suggested-by: Bard Liao <yung-chuan.liao@linux.intel.com> Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> BugLink: thesofproject/linux#2186 Link: https://lore.kernel.org/r/20200612205938.26415-2-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>

Use devm_ to avoid use-after-free KASAN reports and simplify error handling. Signed-off-by: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com> Reviewed-by: Ranjani Sridharan <ranjani.sridharan@linux.intel.com> Reviewed-by: Bard Liao <yung-chuan.liao@linux.intel.com> Reviewed-by: Kai Vehmanen <kai.vehmanen@linux.intel.com> BugLink: thesofproject/linux#2186 Link: https://lore.kernel.org/r/20200612205938.26415-3-pierre-louis.bossart@linux.intel.com Signed-off-by: Mark Brown <broonie@kernel.org>