linux-kernel - Re: [PATCH] soundwire: fix double free of pointer

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <83c8bfa3-4377-4198-b48e-351f9a9f63ed@linux.intel.com>
Date: Mon, 12 Feb 2024 08:49:11 -0600
From: Pierre-Louis Bossart <pierre-louis.bossart@...ux.intel.com>
To: Daniil Dulov <d.dulov@...ddin.ru>, Vinod Koul <vkoul@...nel.org>
Cc: Bard Liao <yung-chuan.liao@...ux.intel.com>,
 Sanyog Kale <sanyog.r.kale@...el.com>, alsa-devel@...a-project.org,
 linux-kernel@...r.kernel.org, lvc-project@...uxtesting.org
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 <d.dulov@...ddin.ru>
> ---
>  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.