Re: [PATCH 08/11] ASoC: imx-audio-rpmsg: Use guard() for spin locks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The rpmsg callback `imx_audio_rpmsg_cb` can be invoked before probe initialization completes, leading to a NULL pointer dereference on the `rpmsg` variable.
- [High] Spurious or early messages from the M core cause a NULL pointer dereference on the `info` pointer.
- [High] Spurious or early `TX_PERIOD_DONE` / `RX_PERIOD_DONE` messages cause a divide-by-zero panic due to an uninitialized `num_period` divisor.
- [High] Uninitialized callback execution upon receiving early messages causes a kernel crash.
- [High] The callback `imx_audio_rpmsg_cb` can execute concurrently with or after driver removal, resulting in a Use-After-Free of the `info` and `rpmsg_pdev` structures.
- [High] Platform devices are leaked on the error paths of `imx_audio_rpmsg_probe()` due to missing cleanup.
- [Medium] The callback trusts the length of the incoming rpmsg payload without validation, leading to out-of-bounds reads.
--

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

ASoC: imx-audio-rpmsg: Use guard() for spin locks
    
Clean up the code using guard() for spin locks.
Merely code refactoring, and no behavior change.

> diff --git a/sound/soc/fsl/imx-audio-rpmsg.c b/sound/soc/fsl/imx-audio-rpmsg.c
> index 38aafb8954c76..b55dfbdb45026 100644
> --- a/sound/soc/fsl/imx-audio-rpmsg.c
> +++ b/sound/soc/fsl/imx-audio-rpmsg.c
> @@ -22,7 +22,6 @@ static int imx_audio_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len,
>  	struct rpmsg_r_msg *r_msg = (struct rpmsg_r_msg *)data;
>  	struct rpmsg_info *info;
>  	struct rpmsg_msg *msg;
> -	unsigned long flags;
>  
>  	if (!rpmsg->rpmsg_pdev)

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to a NULL
pointer dereference?

The rpmsg endpoint is created and the callback registered before the driver's
probe function executes. If the remote core sends an early message, this
callback could run while rpmsg is NULL because dev_set_drvdata() hasn't
happened yet.

Additionally, if imx_audio_rpmsg_remove() unregisters the platform device while
the rpmsg endpoint is still active, could an incoming message execute this
callback and dereference freed memory for rpmsg_pdev and info?

>  		return 0;

[ ... ]

> @@ -37,21 +36,21 @@ static int imx_audio_rpmsg_cb(struct rpmsg_device *rpdev, void *data, int len,
>  		/* TYPE C is notification from M core */
>  		switch (r_msg->header.cmd) {
>  		case TX_PERIOD_DONE:
> -			spin_lock_irqsave(&info->lock[TX], flags);
> -			msg = &info->msg[TX_PERIOD_DONE + MSG_TYPE_A_NUM];
> -			msg->r_msg.param.buffer_tail =
> -						r_msg->param.buffer_tail;
> -			msg->r_msg.param.buffer_tail %= info->num_period[TX];
> -			spin_unlock_irqrestore(&info->lock[TX], flags);
> +			scoped_guard(spinlock_irqsave, &info->lock[TX]) {

[Severity: High]
This is a pre-existing issue, but can spurious or early messages cause a NULL
pointer dereference here?

If a message arrives before the child probe completes initialization,
platform_get_drvdata() could return NULL for info, causing scoped_guard() to
dereference a NULL pointer when accessing &info->lock[TX].

> +				msg = &info->msg[TX_PERIOD_DONE + MSG_TYPE_A_NUM];
> +				msg->r_msg.param.buffer_tail =
> +							r_msg->param.buffer_tail;
> +				msg->r_msg.param.buffer_tail %= info->num_period[TX];

[Severity: High]
This isn't a bug introduced by this patch, but does this modulo operation
risk a division-by-zero panic?

If a spurious or early TX_PERIOD_DONE message arrives before the ALSA stream
is prepared, info->num_period[TX] will still be initialized to 0.

> +			}
>  			info->callback[TX](info->callback_param[TX]);

[Severity: High]
This is a pre-existing issue, but could this unconditionally execute an
uninitialized function pointer?

If an early message arrives before the ALSA stream starts, info->callback[TX]
might be NULL, triggering a kernel panic.


[ ... ]


There are a few other pre-existing issues in this file that might be worth
looking into:

[Severity: Medium]
This is a pre-existing issue, but does the callback trust the length of the
incoming rpmsg payload without validation?

In imx_audio_rpmsg_cb() for MSG_TYPE_B:

	memcpy(&info->r_msg, r_msg, sizeof(struct rpmsg_r_msg));

The callback casts data to struct rpmsg_r_msg * but doesn't validate that
len >= sizeof(struct rpmsg_r_msg). If the remote peer sends a smaller payload,
this results in an out-of-bounds read from the IPC buffer.

[Severity: High]
This isn't a bug introduced by this patch, but are platform devices leaked on
the error paths of imx_audio_rpmsg_probe()?

If platform_device_register_data() for data->rpmsg_pdev fails, the code records
the error but unconditionally proceeds to register data->card_pdev. If the
latter succeeds, the function returns the error from the first failure, leaving
card_pdev permanently registered. Conversely, if rpmsg_pdev succeeds but
card_pdev fails, it returns without unregistering rpmsg_pdev.

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