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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Non-synchronous timer cancellation in teardown paths (`imx_rpmsg_pcm_close` and `imx_rpmsg_terminate_all`) can lead to use-after-free, queueing work on a destroyed workqueue, and sending messages after stream closure.
--

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

ASoC: fsl_rpmsg: Use guard() for mutex & spin locks

This commit cleans up the code by using guard() for mutex and spin locks.
This is intended to be a refactoring with 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
> @@ -39,10 +39,9 @@ static int imx_rpmsg_pcm_send_message(struct rpmsg_msg *msg,
>  	struct rpmsg_device *rpdev = info->rpdev;
>  	int ret = 0;
>  
> -	mutex_lock(&info->msg_lock);
> +	guard(mutex)(&info->msg_lock);
>  	if (!rpdev) {
>  		dev_err(info->dev, "rpmsg channel not ready\n");
> -		mutex_unlock(&info->msg_lock);
>  		return -EINVAL;
>  	}
>  

[ ... ]

[Severity: High]
This is a pre-existing issue, but the driver uses timer_delete() instead of
timer_delete_sync() or timer_shutdown_sync() during teardown paths.

In imx_rpmsg_pcm_close():

imx_rpmsg_pcm_close() {
        ...
        /* Flush work in workqueue to make TX_CLOSE is the last message */
        flush_workqueue(info->rpmsg_wq);
        ...
        info->send_message(msg, info);

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

Because timer_delete() is called after flush_workqueue(), could a concurrently
firing timer insert a new TX_PERIOD_DONE work into the workqueue after the
workqueue is flushed and the close message has been sent?

Furthermore, during device unbind or module removal, the workqueue is
destroyed and the info struct is freed.

If the timer callback fires concurrently during teardown, would it attempt to
queue work on the destroyed workqueue and access freed memory, resulting in a
use-after-free?

A similar pattern exists in imx_rpmsg_terminate_all():

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

        return imx_rpmsg_insert_workqueue(substream, msg, info);
}

Does this non-synchronous cancellation allow out-of-order PERIOD_DONE
messages to be sent after the terminate message?

Should timer_delete_sync() be used in these paths to ensure the callback
is completely finished before proceeding?

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