[PATCH] staging: greybus: audio: fix loop cursor use after iteration

[PATCH] staging: greybus: audio: fix loop cursor use after iteration

[Jared Kangas]

gbaudio_dapm_free_controls() iterates over widgets using
list_for_each_entry_safe(), 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.

This was detected with the help of Coccinelle.

Signed-off-by: Jared Kangas <kangas.jd@gmail.com>
---
 drivers/staging/greybus/audio_helper.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
index 843760675876..07461a5d97c7 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, *next_w, *tmp_w;
 #ifdef CONFIG_DEBUG_FS
 	struct dentry *parent = dapm->debugfs_dapm;
 	struct dentry *debugfs_w = NULL;
@@ -124,13 +124,14 @@ 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,
+		w = NULL;
+		list_for_each_entry_safe(tmp_w, next_w, &dapm->card->widgets,
 					 list) {
-			if (w->dapm != dapm)
-				continue;
-			if (!strcmp(w->name, widget->name))
+			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

Re: [PATCH] staging: greybus: audio: fix loop cursor use after iteration

[Dan Carpenter]

On Sun, Jun 05, 2022 at 04:18:06PM -0700, Jared Kangas wrote:
> gbaudio_dapm_free_controls() iterates over widgets using
> list_for_each_entry_safe(), 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.
> 
> This was detected with the help of Coccinelle.
> 

This needs a Fixes tag.

Fixes: 510e340efe0c ("staging: greybus: audio: Add helper APIs for dynamic audio modules")

Otherwise, looks good!

Reviewed-by: Dan Carpenter <dan.carpenter@oracle.com>

regards,
dan carpenter

Re: [PATCH] staging: greybus: audio: fix loop cursor use after iteration

[Jared Kangas]

Thanks for catching that! Is there anything I need to do to add the
'Fixes:' tag to the patch? From my understanding, adding tags is done by
a maintainer later on, but I'm new to the patch submission process so I
want to make sure I'm not missing anything.

Jared

Re: [PATCH] staging: greybus: audio: fix loop cursor use after iteration

[Dan Carpenter]

On Tue, Jun 07, 2022 at 09:40:50AM -0700, Jared Kangas wrote:
> Thanks for catching that! Is there anything I need to do to add the
> 'Fixes:' tag to the patch? From my understanding, adding tags is done by
> a maintainer later on, but I'm new to the patch submission process so I
> want to make sure I'm not missing anything.

No, it's up to the person sending the patch to add the fixes tag.

regards,
dan carpenter

Re: [PATCH] staging: greybus: audio: fix loop cursor use after iteration

[Alex Elder]

On 6/7/22 1:39 PM, Dan Carpenter wrote:
> On Tue, Jun 07, 2022 at 09:40:50AM -0700, Jared Kangas wrote:
>> Thanks for catching that! Is there anything I need to do to add the
>> 'Fixes:' tag to the patch? From my understanding, adding tags is done by
>> a maintainer later on, but I'm new to the patch submission process so I
>> want to make sure I'm not missing anything.
> 
> No, it's up to the person sending the patch to add the fixes tag.

And FYI, "git blame" can be your friend here.

     git blame drivers/staging/greybus/audio_helper.c

It looks like commit 510e340efe0cbd is a possible
candidate, but I'll leave it up to you to determine
that.

					-Alex

> regards,
> dan carpenter
> 

Re: [PATCH] staging: greybus: audio: fix loop cursor use after iteration

[Johan Hovold]

On Sun, Jun 05, 2022 at 04:18:06PM -0700, Jared Kangas wrote:
> gbaudio_dapm_free_controls() iterates over widgets using
> list_for_each_entry_safe(), 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.
> 
> This was detected with the help of Coccinelle.

Please add the missing Fixes tag and a CC stable tag here as Dan
mentioned.

> Signed-off-by: Jared Kangas <kangas.jd@gmail.com>
> ---
>  drivers/staging/greybus/audio_helper.c | 13 +++++++------
>  1 file changed, 7 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/staging/greybus/audio_helper.c b/drivers/staging/greybus/audio_helper.c
> index 843760675876..07461a5d97c7 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, *next_w, *tmp_w;
>  #ifdef CONFIG_DEBUG_FS
>  	struct dentry *parent = dapm->debugfs_dapm;
>  	struct dentry *debugfs_w = NULL;
> @@ -124,13 +124,14 @@ 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,
> +		w = NULL;
> +		list_for_each_entry_safe(tmp_w, next_w, &dapm->card->widgets,

This should be list_for_each_entry() as w is not unlinked in this loop.

>  					 list) {
> -			if (w->dapm != dapm)
> -				continue;
> -			if (!strcmp(w->name, widget->name))
> +			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",

Looks good otherwise:

Reviewed-by: Johan Hovold <johan@kernel.org>