[PATCH 0/6] ASoC: Sanity checks and soc-topology updates
Couple of soc-topology related changes and a use-after-free fix. Said fix and two sanity checks for soc-topology lead the way. While the use-after-free is quite obvious, the sanity checks are here to cover for cases where user malformed the topology file -or- access to filesystem somehow got interrupted during copy operation. We shouldn't be reading outside the file boundary.
Afterward a change to soc_tplg_add_kcontrol(): device being passed to soc_tplg_add_dcontrol() from comp->dev to tplg->dev which corrects dev_xxx() invoked later on. Also, device used for topology memory allocations from component->dev to component->card->dev so memory gets freed each time card device (usually platform device) is removed rather than the component device what may happen less frequently.
Dummy component gets smarter and no longer overrides hw_params if there are other components accociated with related struct snd_soc_pcm_runtime instance.
Amadeusz Sławiński (5): ASoC: core: Remove invalid snd_soc_component_set_jack call ASoC: topology: Check for dapm widget completeness ASoC: topology: Use correct device for prints ASoC: topology: Change topology device to card device ASoC: Stop dummy from overriding hwparams
Cezary Rojewski (1): ASoC: topology: Add header payload_size verification
sound/soc/soc-core.c | 3 --- sound/soc/soc-topology.c | 34 ++++++++++++++++++++++++++++++---- sound/soc/soc-utils.c | 13 +++++++++++++ 3 files changed, 43 insertions(+), 7 deletions(-)
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
If snd_soc_component_set_jack() is called after snd_soc_component_remove() it may operate on memory which is freed in ->remove handler.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/soc-core.c | 3 --- 1 file changed, 3 deletions(-)
diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 121a12244f08..7eaa8fc81d31 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -1363,9 +1363,6 @@ static void soc_remove_component(struct snd_soc_component *component, if (probed) snd_soc_component_remove(component);
- /* For framework level robustness */ - snd_soc_component_set_jack(component, NULL, NULL); - list_del_init(&component->card_list); snd_soc_dapm_free(snd_soc_component_get_dapm(component)); soc_cleanup_component_debugfs(component);
Add sanity check to make sure the data is read within file boundary. Helps in situations where file is only partially copied or malformed.
Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/soc-topology.c | 9 +++++++++ 1 file changed, 9 insertions(+)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 88f849b119da..2484de8fd79a 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2438,6 +2438,7 @@ static int soc_tplg_manifest_load(struct soc_tplg *tplg, _manifest = manifest; } else { abi_match = false; + ret = manifest_new_ver(tplg, manifest, &_manifest); if (ret < 0) return ret; @@ -2468,6 +2469,14 @@ static int soc_valid_header(struct soc_tplg *tplg, return -EINVAL; }
+ if (soc_tplg_get_hdr_offset(tplg) + hdr->payload_size >= tplg->fw->size) { + dev_err(tplg->dev, + "ASoC: invalid header of type %d at offset %ld payload_size %d\n", + le32_to_cpu(hdr->type), soc_tplg_get_hdr_offset(tplg), + hdr->payload_size); + return -EINVAL; + } + /* big endian firmware objects not supported atm */ if (le32_to_cpu(hdr->magic) == SOC_TPLG_MAGIC_BIG_ENDIAN) { dev_err(tplg->dev,
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
Add sanity checks to make sure the data is read within file boundary. Helps in situations where file is only partially copied or malformed.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/soc-topology.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 2484de8fd79a..5b73e1455c23 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -1591,11 +1591,28 @@ static int soc_tplg_dapm_widget_elems_load(struct soc_tplg *tplg, struct snd_soc_tplg_dapm_widget *widget = (struct snd_soc_tplg_dapm_widget *) tplg->pos; int ret;
+ /* + * check if widget itself fits within topology file + * use sizeof instead of widget->size, as we can't be sure + * it is set properly yet (file may end before it is present) + */ + if (soc_tplg_get_offset(tplg) + sizeof(*widget) >= tplg->fw->size) { + dev_err(tplg->dev, "ASoC: invalid widget data size\n"); + return -EINVAL; + } + + /* check if widget has proper size */ if (le32_to_cpu(widget->size) != sizeof(*widget)) { dev_err(tplg->dev, "ASoC: invalid widget size\n"); return -EINVAL; }
+ /* check if widget private data fits within topology file */ + if (soc_tplg_get_offset(tplg) + le32_to_cpu(widget->priv.size) >= tplg->fw->size) { + dev_err(tplg->dev, "ASoC: invalid widget private data size\n"); + return -EINVAL; + } + ret = soc_tplg_dapm_widget_create(tplg, widget); if (ret < 0) { dev_err(tplg->dev, "ASoC: failed to load widget %s\n",
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
soc_tplg_add_dcontrol() passes device as argument which is later used to print messages. Align it with all other prints in file to use tplg->dev.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/soc-topology.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 5b73e1455c23..712b706af677 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -351,7 +351,7 @@ static int soc_tplg_add_kcontrol(struct soc_tplg *tplg, struct snd_soc_component *comp = tplg->comp;
return soc_tplg_add_dcontrol(comp->card->snd_card, - comp->dev, k, comp->name_prefix, comp, kcontrol); + tplg->dev, k, comp->name_prefix, comp, kcontrol); }
/* remove a mixer kcontrol */
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
Topology needs device for prints and resource allocation. So far, component->dev is used. However, this may lead to high memory use in model where card is an independent driver which can be reloaded and topology is loaded from component's probe() method. Every time machine driver is reloaded topology is being loaded anew, each time allocating new memory. Said memory will only be freed when component itself is being freed.
Address the problem by tying topology to component->card->dev instead, so memory occupied by the topology is freed whenever related machine device gets removed.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/soc-topology.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 712b706af677..53b8f88998e7 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -2668,17 +2668,17 @@ int snd_soc_tplg_component_load(struct snd_soc_component *comp, /* * check if we have sane parameters: * comp - needs to exist to keep and reference data while parsing - * comp->dev - used for resource management and prints * comp->card - used for setting card related parameters + * comp->card->dev - used for resource management and prints * fw - we need it, as it is the very thing we parse */ - if (!comp || !comp->dev || !comp->card || !fw) + if (!comp || !comp->card || !comp->card->dev || !fw) return -EINVAL;
/* setup parsing context */ memset(&tplg, 0, sizeof(tplg)); tplg.fw = fw; - tplg.dev = comp->dev; + tplg.dev = comp->card->dev; tplg.comp = comp; if (ops) { tplg.ops = ops;
From: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com
In case that there are other components assigned to runtime device, depending on order dummy component can override their params with its own, which shouldn't happen. Check if there are any other components assigned to rtd and if so, skip setting hwparams.
Occurs when using topology where 'snd-soc-dummy' gets assigned by default as codec and platform component.
Alternative approach would be to copy whole dummy handling and rename it to "snd-soc-null" or something similar. And remove hwparams assignment to make it really do nothing.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com Signed-off-by: Cezary Rojewski cezary.rojewski@intel.com --- sound/soc/soc-utils.c | 13 +++++++++++++ 1 file changed, 13 insertions(+)
diff --git a/sound/soc/soc-utils.c b/sound/soc/soc-utils.c index 299b5d6ebfd1..a4efe7e52a8b 100644 --- a/sound/soc/soc-utils.c +++ b/sound/soc/soc-utils.c @@ -63,10 +63,23 @@ static const struct snd_pcm_hardware dummy_dma_hardware = { .periods_max = 128, };
+ +static const struct snd_soc_component_driver dummy_platform; + static int dummy_dma_open(struct snd_soc_component *component, struct snd_pcm_substream *substream) { struct snd_soc_pcm_runtime *rtd = asoc_substream_to_rtd(substream); + int i; + + /* + * If there are other components associated with rtd, we shouldn't + * override their hwparams + */ + for_each_rtd_components(rtd, i, component) { + if (component->driver == &dummy_platform) + return 0; + }
/* BE's dont need dummy params */ if (!rtd->dai_link->no_pcm)
On Fri, 15 Oct 2021 18:12:51 +0200, Cezary Rojewski wrote:
Couple of soc-topology related changes and a use-after-free fix. Said fix and two sanity checks for soc-topology lead the way. While the use-after-free is quite obvious, the sanity checks are here to cover for cases where user malformed the topology file -or- access to filesystem somehow got interrupted during copy operation. We shouldn't be reading outside the file boundary.
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/6] ASoC: core: Remove invalid snd_soc_component_set_jack call commit: 7db53c21b1c3c25676d1125049bc92c756421cd6 [2/6] ASoC: topology: Add header payload_size verification commit: 86e2d14b6d1a68941b6c0ef39502ec1433b680cb [3/6] ASoC: topology: Check for dapm widget completeness commit: 2e288333e9e0a14f9895321a848992b25a0e5563 [4/6] ASoC: topology: Use correct device for prints commit: 2a710bb35a5abfbca48ed7c229205ace472692d3 [5/6] ASoC: topology: Change topology device to card device commit: f714fbc1e89a3533b2578e0c90ce4f5c05a57f96 [6/6] ASoC: Stop dummy from overriding hwparams commit: 6c504663ba2ee2abeaf5622e27082819326c1bd4
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
participants (2)
-
Cezary Rojewski -
Mark Brown