[PATCH] soundwire: fix error handling
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
stream.c:844:9: warning: Use of memory after it is freed kfree(bus->defer_msg.msg->buf); ^~~~~~~~~~~~~~~~~~~~~~~
This happens in an error handler cleaning up memory allocated for elements in a list.
list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus;
kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg); }
And is triggered when the call to sdw_bank_switch() fails. There are a two problems.
First, when sdw_bank_switch() fails, though it frees memory it does not clear bus's reference 'defer_msg.msg' to that memory.
The second problem is the freeing msg->buf. In some cases msg will be NULL so this will dereference a null pointer. Need to check before freeing.
Fixes: 99b8a5d608a6 ("soundwire: Add bank switch routine") Signed-off-by: Tom Rix trix@redhat.com --- drivers/soundwire/stream.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37290a799023..6e36deb505b1 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -717,6 +717,7 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) kfree(wbuf); error_1: kfree(wr_msg); + bus->defer_msg.msg = NULL; return ret; }
@@ -840,9 +841,10 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) error: list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus; - - kfree(bus->defer_msg.msg->buf); - kfree(bus->defer_msg.msg); + if (bus->defer_msg.msg) { + kfree(bus->defer_msg.msg->buf); + kfree(bus->defer_msg.msg); + } }
msg_unlock:
On Sat, Aug 29, 2020 at 8:35 AM trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
stream.c:844:9: warning: Use of memory after it is freed kfree(bus->defer_msg.msg->buf); ^~~~~~~~~~~~~~~~~~~~~~~
This happens in an error handler cleaning up memory allocated for elements in a list.
list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus; kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg); }And is triggered when the call to sdw_bank_switch() fails. There are a two problems.
First, when sdw_bank_switch() fails, though it frees memory it does not clear bus's reference 'defer_msg.msg' to that memory.
The second problem is the freeing msg->buf. In some cases msg will be NULL so this will dereference a null pointer. Need to check before freeing.
Fixes: 99b8a5d608a6 ("soundwire: Add bank switch routine") Signed-off-by: Tom Rix trix@redhat.com
drivers/soundwire/stream.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37290a799023..6e36deb505b1 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -717,6 +717,7 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) kfree(wbuf); error_1: kfree(wr_msg);
bus->defer_msg.msg = NULL;
This fix looks correct to me because L668 sets `bus->defer_msg.msg = wr_msg;`, but on error L719 frees `wr_msg`, so now `bus->defer_msg.msg` is a dangling pointer.
return ret;}
@@ -840,9 +841,10 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) error: list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus;
kfree(bus->defer_msg.msg->buf);kfree(bus->defer_msg.msg);
if (bus->defer_msg.msg) {kfree(bus->defer_msg.msg->buf);kfree(bus->defer_msg.msg);}
I'd prefer a conditional check for each, but sdw_ml_sync_bank_switch() has this same pattern, so it looks like the lifetime of these two match.
Reviewed-by: Nick Desaulniers ndesaulniers@google.com
Thanks for the patch!
}msg_unlock:
2.18.1
On Mon, Aug 31, 2020 at 10:47 AM Nick Desaulniers ndesaulniers@google.com wrote:
On Sat, Aug 29, 2020 at 8:35 AM trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
Also, Tom, please use ./scripts/get_maintainer.pl on your patches to CC our mailing list; clang-built-linux@googlegroups.com.
On 8/31/20 10:48 AM, Nick Desaulniers wrote:
On Mon, Aug 31, 2020 at 10:47 AM Nick Desaulniers ndesaulniers@google.com wrote:
On Sat, Aug 29, 2020 at 8:35 AM trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
Also, Tom, please use ./scripts/get_maintainer.pl on your patches to CC our mailing list; clang-built-linux@googlegroups.com.
gcc is still doing the building, so it this appropriate ?
Asking because i have been sed-ing clang-build-linux out.
Tom
On Mon, Aug 31, 2020 at 11:20 AM Tom Rix trix@redhat.com wrote:
On 8/31/20 10:48 AM, Nick Desaulniers wrote:
On Mon, Aug 31, 2020 at 10:47 AM Nick Desaulniers ndesaulniers@google.com wrote:
On Sat, Aug 29, 2020 at 8:35 AM trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
Also, Tom, please use ./scripts/get_maintainer.pl on your patches to CC our mailing list; clang-built-linux@googlegroups.com.
gcc is still doing the building, so it this appropriate ?
Asking because i have been sed-ing clang-build-linux out.
ah, right, you can use `--use-cc=clang` for analyses of builds with clang. It doesn't hurt to include our mailing list, since we'd like to know if issues get reported with the analyzer itself.
On 8/31/20 11:39 AM, Nick Desaulniers wrote:
On Mon, Aug 31, 2020 at 11:20 AM Tom Rix trix@redhat.com wrote:
On 8/31/20 10:48 AM, Nick Desaulniers wrote:
On Mon, Aug 31, 2020 at 10:47 AM Nick Desaulniers ndesaulniers@google.com wrote:
On Sat, Aug 29, 2020 at 8:35 AM trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
Also, Tom, please use ./scripts/get_maintainer.pl on your patches to CC our mailing list; clang-built-linux@googlegroups.com.
gcc is still doing the building, so it this appropriate ?
Asking because i have been sed-ing clang-build-linux out.
ah, right, you can use `--use-cc=clang` for analyses of builds with clang. It doesn't hurt to include our mailing list, since we'd like to know if issues get reported with the analyzer itself.
Ok, i'll include it.
The only real issue so far has been https://reviews.llvm.org/D83984
which fixes reporting on a couple of files.
Tom
On 8/31/20 12:47 PM, Nick Desaulniers wrote:
On Sat, Aug 29, 2020 at 8:35 AM trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
stream.c:844:9: warning: Use of memory after it is freed kfree(bus->defer_msg.msg->buf); ^~~~~~~~~~~~~~~~~~~~~~~
This happens in an error handler cleaning up memory allocated for elements in a list.
list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus; kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg); }And is triggered when the call to sdw_bank_switch() fails. There are a two problems.
First, when sdw_bank_switch() fails, though it frees memory it does not clear bus's reference 'defer_msg.msg' to that memory.
The second problem is the freeing msg->buf. In some cases msg will be NULL so this will dereference a null pointer. Need to check before freeing.
Fixes: 99b8a5d608a6 ("soundwire: Add bank switch routine") Signed-off-by: Tom Rix trix@redhat.com
drivers/soundwire/stream.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37290a799023..6e36deb505b1 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -717,6 +717,7 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) kfree(wbuf); error_1: kfree(wr_msg);
bus->defer_msg.msg = NULL;This fix looks correct to me because L668 sets `bus->defer_msg.msg = wr_msg;`, but on error L719 frees `wr_msg`, so now `bus->defer_msg.msg` is a dangling pointer.
return ret;}
@@ -840,9 +841,10 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) error: list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus;
kfree(bus->defer_msg.msg->buf);kfree(bus->defer_msg.msg);
if (bus->defer_msg.msg) {kfree(bus->defer_msg.msg->buf);kfree(bus->defer_msg.msg);}I'd prefer a conditional check for each, but sdw_ml_sync_bank_switch() has this same pattern, so it looks like the lifetime of these two match.
Reviewed-by: Nick Desaulniers ndesaulniers@google.com
Also looks good to me.
Reviewed-by: Pierre-Louis Bossart pierre-louis.bossart@linux.intel.com
Hello Tom,
On 29-08-20, 08:35, trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
stream.c:844:9: warning: Use of memory after it is freed kfree(bus->defer_msg.msg->buf); ^~~~~~~~~~~~~~~~~~~~~~~
This happens in an error handler cleaning up memory allocated for elements in a list.
list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus;
kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg);}
And is triggered when the call to sdw_bank_switch() fails. There are a two problems.
First, when sdw_bank_switch() fails, though it frees memory it does not clear bus's reference 'defer_msg.msg' to that memory.
The second problem is the freeing msg->buf. In some cases msg will be NULL so this will dereference a null pointer. Need to check before freeing.
The change looks good to me, but the title of patch should be revised.
The patch subject should describe the patch, in this case is setting pointer to null on cleanup, so an appropriate subject may be" "[PATCH]: soundwire: set defer_msg to null".
Please revise subject line and update including the ack/reviews received
Thanks
Fixes: 99b8a5d608a6 ("soundwire: Add bank switch routine") Signed-off-by: Tom Rix trix@redhat.com
drivers/soundwire/stream.c | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c index 37290a799023..6e36deb505b1 100644 --- a/drivers/soundwire/stream.c +++ b/drivers/soundwire/stream.c @@ -717,6 +717,7 @@ static int sdw_bank_switch(struct sdw_bus *bus, int m_rt_count) kfree(wbuf); error_1: kfree(wr_msg);
- bus->defer_msg.msg = NULL; return ret;
}
@@ -840,9 +841,10 @@ static int do_bank_switch(struct sdw_stream_runtime *stream) error: list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus;
kfree(bus->defer_msg.msg->buf);kfree(bus->defer_msg.msg);
if (bus->defer_msg.msg) {kfree(bus->defer_msg.msg->buf);kfree(bus->defer_msg.msg);}msg_unlock:
2.18.1
On Tue, Sep 1, 2020 at 4:02 AM Vinod Koul vkoul@kernel.org wrote:
Hello Tom,
On 29-08-20, 08:35, trix@redhat.com wrote:
From: Tom Rix trix@redhat.com
clang static analysis flags this problem
stream.c:844:9: warning: Use of memory after it is freed kfree(bus->defer_msg.msg->buf); ^~~~~~~~~~~~~~~~~~~~~~~
This happens in an error handler cleaning up memory allocated for elements in a list.
list_for_each_entry(m_rt, &stream->master_list, stream_node) { bus = m_rt->bus; kfree(bus->defer_msg.msg->buf); kfree(bus->defer_msg.msg); }And is triggered when the call to sdw_bank_switch() fails. There are a two problems.
First, when sdw_bank_switch() fails, though it frees memory it does not clear bus's reference 'defer_msg.msg' to that memory.
The second problem is the freeing msg->buf. In some cases msg will be NULL so this will dereference a null pointer. Need to check before freeing.
The change looks good to me, but the title of patch should be revised.
The patch subject should describe the patch, in this case is setting pointer to null on cleanup, so an appropriate subject may be" "[PATCH]: soundwire: set defer_msg to null".
Please revise subject line and update including the ack/reviews received
That's fair, I think soundwire: fix double free of dangling pointer would be most precise.
participants (5)
-
Nick Desaulniers -
Pierre-Louis Bossart -
Tom Rix -
trix＠redhat.com
-
Vinod Koul