Re: [PATCH] firmware: imx: dsp: fix mailbox channel leak on kasprintf failure

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value of `imx_dsp_request_channel()` in `imx_common_resume()` leads to invalid pointer dereferences when an `ERR_PTR` is passed to the mailbox framework.
- [High] Mailbox channels can be double-freed when the SOF driver is unbound while in a PM runtime suspended state, potentially freeing another client's active mailbox channel.
- [High] Mailbox channels are requested and activated before `dsp_ipc->ops` is initialized, creating a race condition where an incoming IPC message triggers a NULL pointer dereference.
--

--- Patch [1]: [PATCH] firmware: imx: dsp: fix mailbox channel leak on kasprintf failure ---
commit 227ebd73d256ff38d002fc31b92731a44c2287e6
Author: Linkai Gong <gonglinkai@kylinos.cn>

firmware: imx: dsp: fix mailbox channel leak on kasprintf failure

This commit fixes a memory leak in imx_dsp_setup_channels(). If kasprintf()
fails to allocate memory for a channel name, the function now properly routes
the failure through the existing cleanup path to free previously requested
channels.

> diff --git a/drivers/firmware/imx/imx-dsp.c b/drivers/firmware/imx/imx-dsp.c
> index ed79e823157af..7e3f45ddf0c18 100644
> --- a/drivers/firmware/imx/imx-dsp.c
> +++ b/drivers/firmware/imx/imx-dsp.c
> @@ -99,8 +99,10 @@ static int imx_dsp_setup_channels(struct imx_dsp_ipc *dsp_ipc)
>  		else
>  			chan_name = kasprintf(GFP_KERNEL, "rxdb%d", i - 2);
>  
> -		if (!chan_name)
> -			return -ENOMEM;
> +		if (!chan_name) {
> +			ret = -ENOMEM;
> +			goto out;
> +		}
>  
>  		dsp_chan = &dsp_ipc->chans[i];
>  		dsp_chan->name = chan_name;

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
return value of imx_dsp_request_channel() in imx_common_resume().

If imx_dsp_request_channel() returns an error pointer during resume, it
appears we ignore the return value:

sound/soc/sof/imx/imx-common.c:imx_common_resume() {
    [ ... ]
	for (i = 0; i < DSP_MU_CHAN_NUM; i++)
		imx_dsp_request_channel(common->ipc_handle, i);
    [ ... ]
	return 0;
}

This can leave an unchecked error pointer in dsp_chan->ch. If the sof core then
attempts to send an ipc message, imx_dsp_ring_doorbell() passes this pointer
to mbox_send_message(). Could this cause a panic in the mailbox framework when
it tries to evaluate the invalid pointer?

[Severity: High]
This is a pre-existing issue, but does this cleanup pattern risk a double-free
of mailbox channels?

When the device goes into pm runtime suspend, we free the channels but don't
clear the pointer:

drivers/firmware/imx/imx-dsp.c:imx_dsp_free_channel() {
    [ ... ]
	dsp_chan = &dsp_ipc->chans[idx];
	mbox_free_channel(dsp_chan->ch);
}

If the driver is then unbound while suspended, imx_dsp_remove() unconditionally
iterates over all channels and frees them again:

drivers/firmware/imx/imx-dsp.c:imx_dsp_remove() {
    [ ... ]
	for (i = 0; i < DSP_MU_CHAN_NUM; i++) {
		dsp_chan = &dsp_ipc->chans[i];
		mbox_free_channel(dsp_chan->ch);
		kfree(dsp_chan->name);
	}
    [ ... ]
}

Could this second mbox_free_channel() call force-free a channel that was
meanwhile requested by another client, or panic if it was left as an error
pointer?

[Severity: High]
This is a pre-existing issue, but is there a race condition during probe with
incoming ipc messages?

During imx_probe(), we register the platform device (which requests the mailbox
channels and activates the rx callback) before assigning the ops:

sound/soc/sof/imx/imx-common.c:imx_probe() {
    [ ... ]
	common->ipc_dev = platform_device_register_data(sdev->dev, "imx-dsp",
							PLATFORM_DEVID_NONE,
							pdev, sizeof(*pdev));
    [ ... ]
	common->ipc_handle->ops = &imx_ipc_ops;
}

If the dsp is already running and sends an ipc message before ops is assigned,
could imx_dsp_handle_rx() trigger a null pointer dereference here?

drivers/firmware/imx/imx-dsp.c:imx_dsp_handle_rx() {
    [ ... ]
	if (chan->idx == 0) {
		chan->ipc->ops->handle_reply(chan->ipc);
	} else {
    [ ... ]
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260813095818.2275666-1-gonglinkai@kylinos.cn?part=1