[PATCH] soc: soc-dapm: fix two incorrect uses of list iterator
These two bug are here: list_for_each_entry_safe_continue(w, n, list, power_list); list_for_each_entry_safe_continue(w, n, list, power_list);
After the list_for_each_entry_safe_continue() exits, the list iterator will always be a bogus pointer which point to an invalid struct objdect containing HEAD member. The funciton poniter 'w->event' will be a invalid value which can lead to a control-flow hijack if the 'w' can be controlled.
The original intention was to break the outer list_for_each_entry_safe() loop if w->event is NULL, but forgot to *break* switch statement first. So just add a break to fix the bug.
Cc: stable@vger.kernel.org Fixes: 163cac061c973 ("ASoC: Factor out DAPM sequence execution") Signed-off-by: Xiaomeng Tong xiam0nd.tong@gmail.com --- sound/soc/soc-dapm.c | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/sound/soc/soc-dapm.c b/sound/soc/soc-dapm.c index b06c5682445c..2a5a64d21856 100644 --- a/sound/soc/soc-dapm.c +++ b/sound/soc/soc-dapm.c @@ -1686,9 +1686,11 @@ static void dapm_seq_run(struct snd_soc_card *card,
switch (w->id) { case snd_soc_dapm_pre: - if (!w->event) + if (!w->event) { list_for_each_entry_safe_continue(w, n, list, power_list); + break; + }
if (event == SND_SOC_DAPM_STREAM_START) ret = w->event(w, @@ -1699,9 +1701,11 @@ static void dapm_seq_run(struct snd_soc_card *card, break;
case snd_soc_dapm_post: - if (!w->event) + if (!w->event) { list_for_each_entry_safe_continue(w, n, list, power_list); + break; + }
if (event == SND_SOC_DAPM_STREAM_START) ret = w->event(w,
On Sun, Mar 27, 2022 at 04:21:38PM +0800, Xiaomeng Tong wrote:
case snd_soc_dapm_pre:
if (!w->event)
if (!w->event) { list_for_each_entry_safe_continue(w, n, list, power_list);break;}
This doesn't make much sense. The intent here seems to clearly be to continue; the loop but this doesn't do that - instead it appears that continue doesn't actually do the equivalent of a continue but rather skips over an entry. This should instead be replaced with a plain continue statement.
THe naming of _continue() needs fixing I think - it's just asking to be a bug. Fortunately there's very few users.
On Mon, 28 Mar 2022 17:31:51 +0100, Mark Brown wrote:
On Sun, Mar 27, 2022 at 04:21:38PM +0800, Xiaomeng Tong wrote:
case snd_soc_dapm_pre:
if (!w->event)
if (!w->event) { list_for_each_entry_safe_continue(w, n, list, power_list);break;}This doesn't make much sense. The intent here seems to clearly be to continue; the loop but this doesn't do that - instead it appears that continue doesn't actually do the equivalent of a continue but rather skips over an entry. This should instead be replaced with a plain continue statement.
Yes, you are right. Sorry for a slip of the pen in commit message: should be "to *continue* the outer list_for_each_entry_safe() loop" not "to break ...".
I have resend a PATCH v2 for the fix as you suggested, and cc you. Thank you.
THe naming of _continue() needs fixing I think - it's just asking to be a bug. Fortunately there's very few users.
-- Xiaomeng Tong
participants (2)
-
Mark Brown -
Xiaomeng Tong