Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Jared Kangas <kangas.jd@gmail.com>
To:     vaibhav.sr@gmail.com
Cc:     elder@kernel.org, gregkh@linuxfoundation.org,
        greybus-dev@lists.linaro.org, johan@kernel.org,
        linux-kernel@vger.kernel.org, stable@vger.kernel.org,
        linux-staging@lists.linux.dev, mgreer@animalcreek.com,
        kangas.jd@gmail.com, Dan Carpenter <dan.carpenter@oracle.com>
Subject: [PATCH v2] staging: greybus: audio: fix loop cursor use after iteration
Date:   Thu,  9 Jun 2022 14:45:18 -0700
Message-Id: <20220609214517.85661-1-kangas.jd@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

gbaudio_dapm_free_controls() iterates over widgets using the
list_for_each_entry*() family of macros from <linux/list.h>, which
leaves the loop cursor pointing to a meaningless structure if it
completes a traversal of the list. The cursor was set to NULL at the end
of the loop body, but would be overwritten by the final loop cursor
update.

Because of this behavior, the widget could be non-null after the loop
even if the widget wasn't found, and the cleanup logic would treat the
pointer as a valid widget to free.

To fix this, introduce a temporary variable to act as the loop cursor
and copy it to a variable that can be accessed after the loop finishes.
Due to not removing any list elements, use list_for_each_entry() instead
of list_for_each_entry_safe() in the revised loop.

This was detected with the help of Coccinelle.

Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")
Cc: stable@vger.kernel.org
Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>
Reviewed-by: Johan Hovold <johan@kernel.org>
Signed-off-by: Jared Kangas <kangas.jd@gmail.com>
---

Changes since v1:
 * Removed safe list iteration as suggested by Johan Hovold <johan@kernel.org>
 * Updated patch changelog to explain the list iteration change
 * Added tags to changelog based on feedback (Cc:, Fixes:, Reviewed-by:)

 drivers/staging/greybus/audio_helper.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
index 843760675876..05e91e6bc2a0 100644
--- a/drivers/staging/greybus/audio_helper.c
+++ b/drivers/staging/greybus/audio_helper.c
@@ -115,7 +115,7 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
 			       int num)
 {
 	int i;
-	struct snd_soc_dapm_widget *w, *next_w;
+	struct snd_soc_dapm_widget *w, *tmp_w;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *parent = dapm->debugfs_dapm;
 	struct dentry *debugfs_w = NULL;
@@ -124,13 +124,13 @@ int gbaudio_dapm_free_controls(struct snd_soc_dapm_context *dapm,
 	mutex_lock(&dapm->card->dapm_mutex);
 	for (i = 0; i < num; i++) {
 		/* below logic can be optimized to identify widget pointer */
-		list_for_each_entry_safe(w, next_w, &dapm->card->widgets,
-					 list) {
-			if (w->dapm != dapm)
-				continue;
-			if (!strcmp(w->name, widget->name))
+		w = NULL;
+		list_for_each_entry(tmp_w, &dapm->card->widgets, list) {
+			if (tmp_w->dapm == dapm &&
+			    !strcmp(tmp_w->name, widget->name)) {
+				w = tmp_w;
 				break;
-			w = NULL;
+			}
 		}
 		if (!w) {
 			dev_err(dapm->dev, "%s: widget not found\n",
-- 
2.34.3