Re: [PATCH v2 08/11] ASoC: imx-audio-rpmsg: Use guard() for 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] Processing unexpected or malicious RPMSG messages before the stream or driver is fully initialized triggers a divide-by-zero or NULL pointer dereference.
- [High] Missing length validation on incoming RPMSG messages allows out-of-bounds reads.
- [High] An error pointer in `rpmsg_pdev` can be dereferenced if the platform device registration fails.
--

commit 61d5b2e3f34cf71daf31644fa46cc656a302b3a3
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;

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for the remote
processor to send a message smaller than sizeof(struct rpmsg_r_msg)?

There does not appear to be any validation of the len parameter before casting
data to a struct pointer and later unconditionally copying
sizeof(struct rpmsg_r_msg) bytes. Could this lead to an out-of-bounds read
if a truncated message is received?

>  	struct rpmsg_info *info;
>  	struct rpmsg_msg *msg;
> -	unsigned long flags;
>  
>  	if (!rpmsg->rpmsg_pdev)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does this check adequately handle platform
device registration failures?

If platform_device_register_data() fails during probe, it returns an ERR_PTR
which would pass this NULL check. If an incoming message arrives during this
race window, could this lead to an invalid pointer dereference when
platform_get_drvdata() is called below?

[ ... ]
> @@ -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 are we certain that info and its fields
are fully initialized before this callback can be invoked?

The RPMSG endpoint is created in rpmsg_dev_probe() before the driver's own
probe completes. If the remote processor sends a TX_PERIOD_DONE message
immediately upon endpoint creation, could this race cause a NULL pointer
dereference on info->lock if platform_get_drvdata() returned NULL?

> +				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 is also pre-existing, but if the stream is uninitialized due to the same
race condition mentioned above, could info->num_period[TX] be zero and
trigger a divide-by-zero exception here?

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

[Severity: High]
Similarly, as a pre-existing issue, could info->callback[TX] be uninitialized
and lead to a NULL function pointer dereference if the stream is not yet
prepared?

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