| Message ID | 20220327082138.13696-1-xiam0nd.tong@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | soc: soc-dapm: fix two incorrect uses of list iterator | expand |
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
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,
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(-)