[PATCH 1/2] ASoC: topology: Fix wrong size check
Dan reported that smatch reports wrong size check and after analysis it is confirmed that we are comparing wrong value: pointer size instead of array size. However the check itself is problematic as in UAPI header there are two fields:
struct snd_soc_tplg_enum_control { (...) char texts[SND_SOC_TPLG_NUM_TEXTS][SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; __le32 values[SND_SOC_TPLG_NUM_TEXTS * SNDRV_CTL_ELEM_ID_NAME_MAXLEN / 4];
the texts field is for names and the values one for values assigned to those named fields, after analysis it becomes clear that there is quite a lot overhead values than we may possibly name. So instead of changing check to ARRAY_SIZE(ec->values), as it was first suggested, use hardcoded value of SND_SOC_TPLG_NUM_TEXTS.
Link: https://lore.kernel.org/alsa-devel/X9B0eDcKy+9B6kZl@mwanda/ Reported-by: Dan Carpenter dan.carpenter@oracle.com Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-topology.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index eb2633dd6454..7fb3a87ab860 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -889,10 +889,16 @@ static int soc_tplg_denum_create_values(struct soc_tplg *tplg, struct soc_enum * { int i;
- if (le32_to_cpu(ec->items) > sizeof(*ec->values)) + /* + * Following "if" checks if we have at most SND_SOC_TPLG_NUM_TEXTS + * values instead of using ARRAY_SIZE(ec->values) due to the fact that + * it is oversized for its purpose. Additionally it is done so because + * it is defined in UAPI header where it can't be easily changed. + */ + if (le32_to_cpu(ec->items) > SND_SOC_TPLG_NUM_TEXTS) return -EINVAL;
- se->dobj.control.dvalues = devm_kzalloc(tplg->dev, le32_to_cpu(ec->items) * + se->dobj.control.dvalues = devm_kcalloc(tplg->dev, le32_to_cpu(ec->items), sizeof(u32), GFP_KERNEL); if (!se->dobj.control.dvalues)
When we parse "values" we perform check if there is correct number of them. However similar check is missing in case of "texts", add it.
Signed-off-by: Amadeusz Sławiński amadeuszx.slawinski@linux.intel.com --- sound/soc/soc-topology.c | 3 +++ 1 file changed, 3 insertions(+)
diff --git a/sound/soc/soc-topology.c b/sound/soc/soc-topology.c index 7fb3a87ab860..950c45008e24 100644 --- a/sound/soc/soc-topology.c +++ b/sound/soc/soc-topology.c @@ -856,6 +856,9 @@ static int soc_tplg_denum_create_texts(struct soc_tplg *tplg, struct soc_enum *s { int i, ret;
+ if (le32_to_cpu(ec->items) > ARRAY_SIZE(ec->texts)) + return -EINVAL; + se->dobj.control.dtexts = devm_kcalloc(tplg->dev, le32_to_cpu(ec->items), sizeof(char *), GFP_KERNEL); if (se->dobj.control.dtexts == NULL)
On Thu, 2020-12-10 at 10:25 -0500, Amadeusz Sławiński wrote:
When we parse "values" we perform check if there is correct number of them. However similar check is missing in case of "texts", add it.
Signed-off-by: Amadeusz Sławiński < amadeuszx.slawinski@linux.intel.com>
Both patches look good, Amadeusz!
Reviewed-by: Ranjani Sridharan ranjani.sridharan@linux.intel.com
On Thu, 10 Dec 2020 10:25:40 -0500, Amadeusz Sławiński wrote:
Dan reported that smatch reports wrong size check and after analysis it is confirmed that we are comparing wrong value: pointer size instead of array size. However the check itself is problematic as in UAPI header there are two fields:
struct snd_soc_tplg_enum_control { (...) char texts[SND_SOC_TPLG_NUM_TEXTS][SNDRV_CTL_ELEM_ID_NAME_MAXLEN]; __le32 values[SND_SOC_TPLG_NUM_TEXTS * SNDRV_CTL_ELEM_ID_NAME_MAXLEN / 4];
[...]
Applied to
https://git.kernel.org/pub/scm/linux/kernel/git/broonie/sound.git for-next
Thanks!
[1/2] ASoC: topology: Fix wrong size check commit: 631c78ed72bbf852cc09b24e4e4e412ed88770f2 [2/2] ASoC: topology: Add missing size check commit: f5824e5ce1cdba523a357a4d3ffbe0876a27330f
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 (3)
-
Amadeusz Sławiński
-
Mark Brown
-
Ranjani Sridharan