[PATCH 0/2] ASoC: topology: fix use-after-free when removing components

[PATCH 0/2] ASoC: topology: fix use-after-free when removing components

[Pierre-Louis Bossart]

This patchset fixes a memory allocation issue and removes a 100%
reproducible use-after-free report thrown by KASAN in automated module
removal tests across multiple platforms.

All the credit goes to Bard Liao for root-causing the issue. DAIs may
be registered at the same time as a component, or when the topology is
loaded. This two-step registration causes the memory for
topology-based DAIs to allocated last, and conversely to be released
first by devres, before the component is released and the DAIs removed
from the component DAI list with snd_soc_unregister_dais().

When we remove a component, by the time we walk through its dai list
to unregister all dais, the dais allocated by the topology have been
freed already by devres and the list is corrupted with pointers that
are no longer valid.

The suggestion is to add an explicit devm_ based registration for
topology-based dais, so that each dai is cleanly removed from the
component dai list in the release operation before devres releases the
allocated memory.

Pierre-Louis Bossart (2):
  ASoC: soc-devres: add devm_snd_soc_register_dai()
  ASoC: soc-topology: use devm_snd_soc_register_dai()

 include/sound/soc.h      |  4 ++++
 sound/soc/soc-devres.c   | 37 +++++++++++++++++++++++++++++++++++++
 sound/soc/soc-topology.c |  3 +--
 3 files changed, 42 insertions(+), 2 deletions(-)

-- 
2.20.1

[PATCH 1/2] ASoC: soc-devres: add devm_snd_soc_register_dai()

[Pierre-Louis Bossart]

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: https://github.com/thesofproject/linux/issues/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>
---
 include/sound/soc.h    |  4 ++++
 sound/soc/soc-devres.c | 37 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 828baea66b76..2756f9bcac3e 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -1363,6 +1363,10 @@ void snd_soc_remove_pcm_runtime(struct snd_soc_card *card,
 struct snd_soc_dai *snd_soc_register_dai(struct snd_soc_component *component,
 					 struct snd_soc_dai_driver *dai_drv,
 					 bool legacy_dai_naming);
+struct snd_soc_dai *devm_snd_soc_register_dai(struct device *dev,
+					      struct snd_soc_component *component,
+					      struct snd_soc_dai_driver *dai_drv,
+					      bool legacy_dai_naming);
 void snd_soc_unregister_dai(struct snd_soc_dai *dai);
 
 struct snd_soc_dai *snd_soc_find_dai(
diff --git a/sound/soc/soc-devres.c b/sound/soc/soc-devres.c
index a9ea172a66a7..11e5d7962370 100644
--- a/sound/soc/soc-devres.c
+++ b/sound/soc/soc-devres.c
@@ -9,6 +9,43 @@
 #include <sound/soc.h>
 #include <sound/dmaengine_pcm.h>
 
+static void devm_dai_release(struct device *dev, void *res)
+{
+	snd_soc_unregister_dai(*(struct snd_soc_dai **)res);
+}
+
+/**
+ * devm_snd_soc_register_dai - resource-managed dai registration
+ * @dev: Device used to manage component
+ * @component: The component the DAIs are registered for
+ * @dai_drv: DAI driver to use for the DAI
+ * @legacy_dai_naming: if %true, use legacy single-name format;
+ *	if %false, use multiple-name format;
+ */
+struct snd_soc_dai *devm_snd_soc_register_dai(struct device *dev,
+					      struct snd_soc_component *component,
+					      struct snd_soc_dai_driver *dai_drv,
+					      bool legacy_dai_naming)
+{
+	struct snd_soc_dai **ptr;
+	struct snd_soc_dai *dai;
+
+	ptr = devres_alloc(devm_dai_release, sizeof(*ptr), GFP_KERNEL);
+	if (!ptr)
+		return NULL;
+
+	dai = snd_soc_register_dai(component, dai_drv, legacy_dai_naming);
+	if (dai) {
+		*ptr = dai;
+		devres_add(dev, ptr);
+	} else {
+		devres_free(ptr);
+	}
+
+	return dai;
+}
+EXPORT_SYMBOL_GPL(devm_snd_soc_register_dai);
+
 static void devm_component_release(struct device *dev, void *res)
 {
 	snd_soc_unregister_component(*(struct device **)res);
-- 
2.20.1

[PATCH 2/2] ASoC: soc-topology: use devm_snd_soc_register_dai()

[Pierre-Louis Bossart]

Use devm_ to avoid use-after-free KASAN reports and simplify error
handling.

BugLink: https://github.com/thesofproject/linux/issues/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>
---
 sound/soc/soc-topology.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c
index 9e89633676b7..43e5745b06aa 100644
--- a/sound/soc/soc-topology.c
+++ b/sound/soc/soc-topology.c
@@ -1851,7 +1851,7 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg,
 	list_add(&dai_drv->dobj.list, &tplg->comp->dobj_list);
 
 	/* register the DAI to the component */
-	dai = snd_soc_register_dai(tplg->comp, dai_drv, false);
+	dai = devm_snd_soc_register_dai(tplg->comp->dev, tplg->comp, dai_drv, false);
 	if (!dai)
 		return -ENOMEM;
 
@@ -1859,7 +1859,6 @@ static int soc_tplg_dai_create(struct soc_tplg *tplg,
 	ret = snd_soc_dapm_new_dai_widgets(dapm, dai);
 	if (ret != 0) {
 		dev_err(dai->dev, "Failed to create DAI widgets %d\n", ret);
-		snd_soc_unregister_dai(dai);
 		return ret;
 	}
 
-- 
2.20.1

Re: [PATCH 0/2] ASoC: topology: fix use-after-free when removing components

[Mark Brown]

On Fri, 12 Jun 2020 15:59:36 -0500, Pierre-Louis Bossart wrote:
> This patchset fixes a memory allocation issue and removes a 100%
> reproducible use-after-free report thrown by KASAN in automated module
> removal tests across multiple platforms.
> 
> All the credit goes to Bard Liao for root-causing the issue. DAIs may
> be registered at the same time as a component, or when the topology is
> loaded. This two-step registration causes the memory for
> topology-based DAIs to allocated last, and conversely to be released
> first by devres, before the component is released and the DAIs removed
> from the component DAI list with snd_soc_unregister_dais().
> 
> [...]

Applied to

   https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next

Thanks!

[1/2] ASoC: soc-devres: add devm_snd_soc_register_dai()
      commit: 0fae253af563cf5d1f5dc651d520c3eafd74f183
[2/2] ASoC: soc-topology: use devm_snd_soc_register_dai()
      commit: 6ae4902f2f3400503f9b78e87e8371e4ffde1e0c

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark