Re: [PATCH v26 4/7] firmware: imx: device context dedicated to priv

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but `ele_msg_send_rcv` leaves `priv->waiting_rsp_clbk_hdl.rx_msg` dangling if `ele_msg_send` fails, risking a Use-After-Free memory corruption.
- [High] This problem wasn't introduced by this patch, but there is a silent truncation of a 64-bit physical address in `ele_service_swap()`.
- [High] This problem wasn't introduced by this patch, but there is a double free of `dmam_alloc_coherent` memory during devres teardown.
--

commit 9576b0c1969c96e947c2d66ab7fdb7aa48c35c5d
Author: Pankaj Gupta <pankaj.gupta@nxp.com>

firmware: imx: device context dedicated to priv
    
This commit adds a private device context (priv_dev_ctx) to the se_if_priv
structure. This prepares the driver to enable a misc-device context-based
send-receive path for communicating with the firmware.

> diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
> index 54d79c3d75af6..66bae4c7d4648 100644
> --- a/drivers/firmware/imx/ele_base_msg.c
> +++ b/drivers/firmware/imx/ele_base_msg.c
[ ... ]
> @@ -156,7 +156,7 @@ int ele_service_swap(struct se_if_priv *priv,
>  	if (!tx_msg->data[4])
>  		return -EINVAL;
>  
> -	ret = ele_msg_send_rcv(priv, tx_msg, ELE_SERVICE_SWAP_REQ_MSG_SZ,
> +	ret = ele_msg_send_rcv(priv->priv_dev_ctx, tx_msg, ELE_SERVICE_SWAP_REQ_MSG_SZ,
>  			       rx_msg, ELE_SERVICE_SWAP_RSP_MSG_SZ);
>  	if (ret < 0)
>  		return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does ele_service_swap() silently
truncate 64-bit physical addresses?

If the physical address allocated for addr is above the 4GB boundary,
tx_msg->data[3] only takes the lower 32 bits without validation:

    tx_msg->data[3] = lower_32_bits(addr);

Since the upper 32 bits are ignored without checking if they are zero, could
this result in the firmware accessing an incorrect physical address?

> diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
> index ba606f4e8be83..b37ea1f14f75f 100644
> --- a/drivers/firmware/imx/ele_common.c
> +++ b/drivers/firmware/imx/ele_common.c
[ ... ]
> @@ -135,33 +136,37 @@ int ele_msg_send(struct se_if_priv *priv,
>  }
>  
>  /* API used for send/receive blocking call. */
> -int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
> -		     void *rx_msg, int exp_rx_msg_sz)
> +int ele_msg_send_rcv(struct se_if_device_ctx *dev_ctx, void *tx_msg,
> +		     int tx_msg_sz, void *rx_msg, int exp_rx_msg_sz)
>  {
> +	struct se_if_priv *priv = dev_ctx->priv;
>  	int err;
>  
>  	guard(mutex)(&priv->se_if_cmd_lock);
>  
>  	if (atomic_read(&priv->fw_busy)) {
> -		dev_dbg(priv->dev, "ELE became unresponsive.\n");
> +		dev_dbg(priv->dev, "%s: ELE became unresponsive.\n", dev_ctx->devname);
>  		return -EBUSY;
>  	}
>  	reinit_completion(&priv->waiting_rsp_clbk_hdl.done);
> +	priv->waiting_rsp_clbk_hdl.dev_ctx = dev_ctx;
>  	priv->waiting_rsp_clbk_hdl.rx_msg_sz = exp_rx_msg_sz;
>  	priv->waiting_rsp_clbk_hdl.rx_msg = rx_msg;
>  
> -	err = ele_msg_send(priv, tx_msg, tx_msg_sz);
> +	err = ele_msg_send(dev_ctx, tx_msg, tx_msg_sz);
>  	if (err < 0)
>  		return err;

[Severity: High]
This isn't a bug introduced by this patch, but does this early error return
leave a dangling pointer in rx_msg?

If ele_msg_send() fails, ele_msg_send_rcv() returns early without clearing
priv->waiting_rsp_clbk_hdl.rx_msg. Because this caller-provided buffer is often
freed upon an error return, the pointer left behind points to freed memory.

Later, an unsolicited or delayed firmware response handled by
se_if_rx_callback() will see the non-null pointer:

    spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
    if (!se_clbk_hdl->rx_msg) {
        ...
    }
    ...
    memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);

Can this cause a use-after-free memory corruption when data is copied into
the freed buffer?

> diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
> index 9a2c3c6111462..a4823f485f88a 100644
> --- a/drivers/firmware/imx/se_ctrl.c
> +++ b/drivers/firmware/imx/se_ctrl.c
[ ... ]
> @@ -262,6 +292,12 @@ static void se_if_probe_cleanup(void *plat_dev)
>  	 */
>  	of_reserved_mem_device_release(dev);
>  	dev_set_drvdata(dev, NULL);
> +
> +	if (priv->priv_dev_ctx) {
> +		kfree(priv->priv_dev_ctx->devname);
> +		kfree(priv->priv_dev_ctx);
> +		priv->priv_dev_ctx = NULL;
> +	}
>  	kfree(priv);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does se_if_probe_cleanup()
double-free memory allocated by dmam_alloc_coherent()?

In this function, memory allocated by dmam_alloc_coherent is explicitly freed:

    if (load_fw && load_fw->imem.buf) {
        dmam_free_coherent(dev, ELE_IMEM_SIZE, load_fw->imem.buf,
                           load_fw->imem.phyaddr);
        load_fw->imem.buf = NULL;
    }

Since dmam_alloc_coherent registers its own devres release action, this action
runs on driver unbind and frees the memory. Because devres actions run in
LIFO order, the release action runs before se_if_probe_cleanup() and frees
the memory without nulling the pointer.

When se_if_probe_cleanup() eventually runs, does it observe a non-null
pointer and call dmam_free_coherent() on already freed memory, resulting in
a double free?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260629-imx-se-if-v26-0-146446285744@nxp.com?part=4