Re: [PATCH v2 09/11] ASoC: fsl_rpmsg: Use guard() for mutex & spin locks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] Non-synchronous timer cancellation (`timer_delete()`) and improper teardown order allow race conditions and use-after-free.
- [High] Unvalidated remote processor command index allows out-of-bounds array write.
- [High] Workqueue destroyed before ALSA component unregistration leads to Use-After-Free.
--

commit fb7a6469268cda085459a69bd6a9d37338ef0a42
Author: bui duc phuc <phucduc.bui@gmail.com>

ASoC: fsl_rpmsg: Use guard() for mutex & spin locks
    
Clean up the code using guard() for mutex & spin locks.
Merely code refactoring, and no behavior change.

> diff --git a/sound/soc/fsl/imx-pcm-rpmsg.c b/sound/soc/fsl/imx-pcm-rpmsg.c
> index 031e5272215d4..7210393dfa5d9 100644
> --- a/sound/soc/fsl/imx-pcm-rpmsg.c
> +++ b/sound/soc/fsl/imx-pcm-rpmsg.c

[ ... ]

> @@ -71,7 +67,6 @@ static int imx_rpmsg_pcm_send_message(struct rpmsg_msg *msg,
>  	if (!ret) {
>  		dev_err(&rpdev->dev, "rpmsg_send cmd %d timeout!\n",
>  			msg->s_msg.header.cmd);
> -		mutex_unlock(&info->msg_lock);
>  		return -ETIMEDOUT;
>  	}

[Severity: High]
This is a pre-existing issue, but can this out-of-bounds write occur here?

Looking at the code just below this in imx_rpmsg_pcm_send_message():

	memcpy(&msg->r_msg, &info->r_msg, sizeof(struct rpmsg_r_msg));
	memcpy(&info->msg[msg->r_msg.header.cmd].r_msg,
	       &msg->r_msg, sizeof(struct rpmsg_r_msg));

The msg->r_msg.header.cmd field comes from the remote processor and is used
directly as an array index for info->msg. Since cmd is an 8-bit unsigned
integer, and the array bounds are likely smaller, a compromised remote
processor could send a command ID up to 255.

Does this allow an out-of-bounds write that corrupts adjacent heap
structures like the workqueue pointer?

[ ... ]

> @@ -681,10 +672,10 @@ static void imx_rpmsg_pcm_work(struct work_struct *work)
>  		info->send_message(&work_of_rpmsg->msg, info);
>  
>  	/* update read index */
> -	spin_lock_irqsave(&info->wq_lock, flags);
> -	info->work_read_index++;
> -	info->work_read_index %= WORK_MAX_NUM;
> -	spin_unlock_irqrestore(&info->wq_lock, flags);
> +	scoped_guard(spinlock_irqsave, &info->wq_lock) {
> +		info->work_read_index++;
> +		info->work_read_index %= WORK_MAX_NUM;
> +	}
>  }

[Severity: High]
This is a pre-existing issue, but does using timer_delete() instead of
timer_delete_sync() or timer_shutdown_sync() allow the timer callback
to execute after teardown?

In imx_rpmsg_pcm_close() and imx_rpmsg_terminate_all():

	timer_delete(&info->stream_timer[substream->stream].timer);

This non-synchronous deletion means the timer callback might continue
executing concurrently. In imx_rpmsg_pcm_close(), timer_delete() is called
after flush_workqueue(), so a late timer callback could queue new work
after the stream is supposedly closed.

Additionally, imx_rpmsg_pcm_remove() does not appear to stop timers
before devres frees the info structure, potentially leading to a
use-after-free if a timer is still pending.

[Severity: High]
This isn't a bug introduced by this patch, but does the current cleanup
order in imx_rpmsg_pcm_remove() cause a use-after-free?

The driver registers the component using devm_snd_soc_register_component()
in imx_rpmsg_pcm_probe(), which relies on devres for cleanup. However, the
workqueue is explicitly destroyed in imx_rpmsg_pcm_remove():

static void imx_rpmsg_pcm_remove(struct platform_device *pdev)
{
	...
	if (info->rpmsg_wq)
		destroy_workqueue(info->rpmsg_wq);
}

Since devres runs after the remove callback returns, the component
remains active while the workqueue is already destroyed. If a stream
is closed during this window, imx_rpmsg_pcm_close() will call
flush_workqueue(info->rpmsg_wq) on the destroyed workqueue.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615093824.115751-1-phucduc.bui@gmail.com?part=9