2024-02-11 15:11:32

by Daniil Dulov

Subject: [PATCH] soundwire: fix double free of pointer

If sdw_ml_sync_bank_switch() returns error not on the first iteration,
it leads to freeing prevously freed memory. So, set the pointer to NULL
after each successful bank switch.

Signed-off-by: Daniil Dulov <[email protected]>
---
drivers/soundwire/stream.c | 1 +
1 file changed, 1 insertion(+)

diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
index 304ff2ee7d75..d650e6f0f8e7 100644
--- a/drivers/soundwire/stream.c
+++ b/drivers/soundwire/stream.c
@@ -833,6 +833,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
"multi link bank switch failed: %d\n", ret);
goto error;
}
+ bus->defer_msg.msg = NULL;

if (multi_link)
mutex_unlock(&bus->msg_lock);
--
2.25.1

2024-02-12 18:30:36

by Pierre-Louis Bossart

Subject: Re: [PATCH] soundwire: fix double free of pointer

On 2/11/24 09:09, Daniil Dulov wrote:
> If sdw_ml_sync_bank_switch() returns error not on the first iteration,
> it leads to freeing prevously freed memory. So, set the pointer to NULL
> after each successful bank switch.
>
> Signed-off-by: Daniil Dulov <[email protected]>
> ---
> drivers/soundwire/stream.c | 1 +
> 1 file changed, 1 insertion(+)
>
> diff --git a/drivers/soundwire/stream.c b/drivers/soundwire/stream.c
> index 304ff2ee7d75..d650e6f0f8e7 100644
> --- a/drivers/soundwire/stream.c
> +++ b/drivers/soundwire/stream.c
> @@ -833,6 +833,7 @@ static int do_bank_switch(struct sdw_stream_runtime *stream)
> "multi link bank switch failed: %d\n", ret);
> goto error;
> }
> + bus->defer_msg.msg = NULL;
>
> if (multi_link)
> mutex_unlock(&bus->msg_lock);

Not following what the issue is...

On success, sdw_ml_sync_bank_switch() frees the buffers with

if (bus->defer_msg.msg) {
kfree(bus->defer_msg.msg->buf);
kfree(bus->defer_msg.msg);
bus->defer_msg.msg = NULL;
}

So if there is an issue on the second iteration, then the loop will
detect already freed memory in the previous iteration and skip it:

/* Check if bank switch was successful */
ret = sdw_ml_sync_bank_switch(bus);
if (ret < 0) {
dev_err(bus->dev,
"multi link bank switch failed: %d\n", ret);
goto error;
}

error:
list_for_each_entry(m_rt, &stream->master_list, stream_node) {
bus = m_rt->bus;
if (bus->defer_msg.msg) { <<<< TEST FOR FREED MEMORY
kfree(bus->defer_msg.msg->buf);
kfree(bus->defer_msg.msg);
bus->defer_msg.msg = NULL;
}
}

It could very well be that I need more coffee on this post-SuperBowl
Monday morning, but I just don't see the problem.