Received-SPF: pass (google.com: domain of linux-kernel+bounces-61846-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Message-ID: <83c8bfa3-4377-4198-b48e-351f9a9f63ed@linux.intel.com>
Date: Mon, 12 Feb 2024 08:49:11 -0600
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] soundwire: fix double free of pointer
Content-Language: en-US
To: Daniil Dulov <d.dulov@aladdin.ru>, Vinod Koul <vkoul@kernel.org>
Cc: Bard Liao <yung-chuan.liao@linux.intel.com>,
 Sanyog Kale <sanyog.r.kale@intel.com>, alsa-devel@alsa-project.org,
 linux-kernel@vger.kernel.org, lvc-project@linuxtesting.org
References: <20240211150937.4058-1-d.dulov@aladdin.ru>
From: Pierre-Louis Bossart <pierre-louis.bossart@linux.intel.com>
In-Reply-To: <20240211150937.4058-1-d.dulov@aladdin.ru>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit


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@aladdin.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.