On Thu, Jun 09, 2022 at 02:45:18PM -0700, Jared Kangas wrote:
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:)
Apparently Greg applied this to staging-next before we had a change to look at it. You should have received a notification from Greg when he did so.
https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/staging.git/commit/?h...
It seems unlikely that this would cause any issues in real life, but there's still a chance it will be picked up by the stable team despite the lack of a CC stable tag.
I've just sent a follow-up patch to replace the list macro.
Johan