[PATCH] soc: meson: fix a missing check on list iterator
The bug is here: *dai_name = dai->driver->name;
For for_each_component_dais(), just like list_for_each_entry, the list iterator 'runtime' will point to a bogus position containing HEAD if the list is empty or no element is found. This case must be checked before any use of the iterator, otherwise it will lead to a invalid memory access.
To fix the bug, just move the assignment into loop and return 0 when element is found, otherwise return -EINVAL;
Cc: stable@vger.kernel.org Fixes: 6ae9ca9ce986b ("ASoC: meson: aiu: add i2s and spdif support") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- sound/soc/meson/aiu.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c index d299a70db7e5..b52915c6f53b 100644 --- a/sound/soc/meson/aiu.c +++ b/sound/soc/meson/aiu.c @@ -61,14 +61,14 @@ int aiu_of_xlate_dai_name(struct snd_soc_component *component, return -EINVAL;
for_each_component_dais(component, dai) { - if (id == 0) - break; + if (id == 0) { + *dai_name = dai->driver->name; + return 0; + } id--; }
- *dai_name = dai->driver->name; - - return 0; + return -EINVAL; }
static int aiu_cpu_of_xlate_dai_name(struct snd_soc_component *component,
On Sun 27 Mar 2022 at 16:18, Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
The bug is here: *dai_name = dai->driver->name;
For for_each_component_dais(), just like list_for_each_entry, the list iterator 'runtime' will point to a bogus position containing HEAD if the list is empty or no element is found. This case must be checked before any use of the iterator, otherwise it will lead to a invalid memory access.
To fix the bug, just move the assignment into loop and return 0 when element is found, otherwise return -EINVAL;
Except we already checked that the id is valid and know an element will be be found once we enter the loop. No bug here and this patch does not seem necessary to me.
Cc: stable@vger.kernel.org Fixes: 6ae9ca9ce986b ("ASoC: meson: aiu: add i2s and spdif support") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com
sound/soc/meson/aiu.c | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)
diff --git a/sound/soc/meson/aiu.c b/sound/soc/meson/aiu.c index d299a70db7e5..b52915c6f53b 100644 --- a/sound/soc/meson/aiu.c +++ b/sound/soc/meson/aiu.c @@ -61,14 +61,14 @@ int aiu_of_xlate_dai_name(struct snd_soc_component *component, return -EINVAL;
for_each_component_dais(component, dai) {
if (id == 0)break;
if (id == 0) {*dai_name = dai->driver->name;return 0;}
- *dai_name = dai->driver->name;
- return 0;
- return -EINVAL;
}
static int aiu_cpu_of_xlate_dai_name(struct snd_soc_component *component,
On Sun, 27 Mar 2022 13:03:14 +0200, Jerome Brunet jbrunet@baylibre.com wrote:
On Sun 27 Mar 2022 at 16:18, Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
The bug is here: *dai_name = dai->driver->name;
For for_each_component_dais(), just like list_for_each_entry, the list iterator 'runtime' will point to a bogus position containing HEAD if the list is empty or no element is found. This case must be checked before any use of the iterator, otherwise it will lead to a invalid memory access.
To fix the bug, just move the assignment into loop and return 0 when element is found, otherwise return -EINVAL;
Except we already checked that the id is valid and know an element will be be found once we enter the loop. No bug here and this patch does not seem necessary to me.
Yea, you should be right, it is not a bug here. id already be checked before enter the loop:
if (id < 0 || id >= component->num_dai) return -EINVAL;
but if component->num_dai is not correct due to miscaculation or others reason and the door is reopened, this patch can avoid a invalid memory access. Anyway, it is a good choice to use the list iterator only inside the loop, as linus suggested[1]. and we are on the way to change all these use-after-iter cases.
[1]https://lore.kernel.org/lkml/20220217184829.1991035-1-jakobkoschel@gmail.com...
-- Xiaomeng Tong
On Sun 27 Mar 2022 at 21:08, Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
On Sun, 27 Mar 2022 13:03:14 +0200, Jerome Brunet jbrunet@baylibre.com wrote:
On Sun 27 Mar 2022 at 16:18, Xiaomeng Tong xiam0nd.tong@gmail.com wrote:
The bug is here: *dai_name = dai->driver->name;
For for_each_component_dais(), just like list_for_each_entry, the list iterator 'runtime' will point to a bogus position containing HEAD if the list is empty or no element is found. This case must be checked before any use of the iterator, otherwise it will lead to a invalid memory access.
To fix the bug, just move the assignment into loop and return 0 when element is found, otherwise return -EINVAL;
Except we already checked that the id is valid and know an element will be be found once we enter the loop. No bug here and this patch does not seem necessary to me.
Yea, you should be right, it is not a bug here. id already be checked before enter the loop:
if (id < 0 || id >= component->num_dai) return -EINVAL;
but if component->num_dai is not correct due to miscaculation or others reason and the door is reopened, this patch can avoid a invalid memory access.
This is a speculation which just does not hold ATM. What this patch does is adding dead code cause the last "return -EINVAL;" will never be reached.
This no fix nor improvement.
Anyway, it is a good choice to use the list iterator only inside the loop, as linus suggested[1]. and we are on the way to change all these use-after-iter cases.
[1]https://lore.kernel.org/lkml/20220217184829.1991035-1-jakobkoschel@gmail.com...
You can make improvements as long as the code is kept clean an maintainable. Dead code is not OK.
participants (2)
-
Jerome Brunet -
Xiaomeng Tong