* [PATCH] soundwire: fix error handling
@ 2020-08-29 15:35 trix
2020-08-31 17:47 ` Nick Desaulniers
2020-09-01 11:02 ` Vinod Koul
0 siblings, 2 replies; 9+ messages in thread
From: trix @ 2020-08-29 15:35 UTC (permalink / raw)
To: vkoul, yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
natechancellor, ndesaulniers, shreyas.nc
Cc: alsa-devel, linux-kernel, Tom Rix
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:
--
2.18.1
^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-29 15:35 [PATCH] soundwire: fix error handling trix
@ 2020-08-31 17:47 ` Nick Desaulniers
2020-08-31 17:48 ` Nick Desaulniers
2020-08-31 22:45 ` Pierre-Louis Bossart
2020-09-01 11:02 ` Vinod Koul
1 sibling, 2 replies; 9+ messages in thread
From: Nick Desaulniers @ 2020-08-31 17:47 UTC (permalink / raw)
To: trix
Cc: Vinod Koul, yung-chuan.liao, Pierre-Louis Bossart, Sanyog Kale,
Nathan Chancellor, shreyas.nc, alsa-devel, LKML
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
>
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-31 17:47 ` Nick Desaulniers
@ 2020-08-31 17:48 ` Nick Desaulniers
2020-08-31 18:20 ` Tom Rix
2020-08-31 22:45 ` Pierre-Louis Bossart
1 sibling, 1 reply; 9+ messages in thread
From: Nick Desaulniers @ 2020-08-31 17:48 UTC (permalink / raw)
To: trix
Cc: Vinod Koul, yung-chuan.liao, Pierre-Louis Bossart, Sanyog Kale,
Nathan Chancellor, shreyas.nc, alsa-devel, LKML,
clang-built-linux
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.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-31 17:48 ` Nick Desaulniers
@ 2020-08-31 18:20 ` Tom Rix
2020-08-31 18:39 ` Nick Desaulniers
0 siblings, 1 reply; 9+ messages in thread
From: Tom Rix @ 2020-08-31 18:20 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Vinod Koul, yung-chuan.liao, Pierre-Louis Bossart, Sanyog Kale,
Nathan Chancellor, shreyas.nc, alsa-devel, LKML,
clang-built-linux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-31 18:20 ` Tom Rix
@ 2020-08-31 18:39 ` Nick Desaulniers
2020-08-31 18:57 ` Tom Rix
0 siblings, 1 reply; 9+ messages in thread
From: Nick Desaulniers @ 2020-08-31 18:39 UTC (permalink / raw)
To: Tom Rix
Cc: Vinod Koul, yung-chuan.liao, Pierre-Louis Bossart, Sanyog Kale,
Nathan Chancellor, shreyas.nc, alsa-devel, LKML,
clang-built-linux
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.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-31 18:39 ` Nick Desaulniers
@ 2020-08-31 18:57 ` Tom Rix
0 siblings, 0 replies; 9+ messages in thread
From: Tom Rix @ 2020-08-31 18:57 UTC (permalink / raw)
To: Nick Desaulniers
Cc: Vinod Koul, yung-chuan.liao, Pierre-Louis Bossart, Sanyog Kale,
Nathan Chancellor, shreyas.nc, alsa-devel, LKML,
clang-built-linux
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-31 17:47 ` Nick Desaulniers
2020-08-31 17:48 ` Nick Desaulniers
@ 2020-08-31 22:45 ` Pierre-Louis Bossart
1 sibling, 0 replies; 9+ messages in thread
From: Pierre-Louis Bossart @ 2020-08-31 22:45 UTC (permalink / raw)
To: Nick Desaulniers, trix
Cc: alsa-devel, LKML, Vinod Koul, shreyas.nc, Nathan Chancellor,
yung-chuan.liao, Sanyog Kale
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>
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-08-29 15:35 [PATCH] soundwire: fix error handling trix
2020-08-31 17:47 ` Nick Desaulniers
@ 2020-09-01 11:02 ` Vinod Koul
2020-09-01 17:41 ` Nick Desaulniers
1 sibling, 1 reply; 9+ messages in thread
From: Vinod Koul @ 2020-09-01 11:02 UTC (permalink / raw)
To: trix
Cc: yung-chuan.liao, pierre-louis.bossart, sanyog.r.kale,
natechancellor, ndesaulniers, shreyas.nc, alsa-devel,
linux-kernel
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
--
~Vinod
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] soundwire: fix error handling
2020-09-01 11:02 ` Vinod Koul
@ 2020-09-01 17:41 ` Nick Desaulniers
0 siblings, 0 replies; 9+ messages in thread
From: Nick Desaulniers @ 2020-09-01 17:41 UTC (permalink / raw)
To: Vinod Koul
Cc: Tom Rix, yung-chuan.liao, Pierre-Louis Bossart, Sanyog Kale,
Nathan Chancellor, shreyas.nc, alsa-devel, LKML
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.
--
Thanks,
~Nick Desaulniers
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2020-09-01 17:42 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-08-29 15:35 [PATCH] soundwire: fix error handling trix
2020-08-31 17:47 ` Nick Desaulniers
2020-08-31 17:48 ` Nick Desaulniers
2020-08-31 18:20 ` Tom Rix
2020-08-31 18:39 ` Nick Desaulniers
2020-08-31 18:57 ` Tom Rix
2020-08-31 22:45 ` Pierre-Louis Bossart
2020-09-01 11:02 ` Vinod Koul
2020-09-01 17:41 ` Nick Desaulniers
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).