RE: [PATCH v32 3/7] firmware: imx: add driver for NXP EdgeLock Enclave

> > @@ -65,15 +65,15 @@ int ele_get_info(struct se_if_priv *priv, struct
> ele_dev_info *s_info)
> >  	se_fill_cmd_msg_hdr(priv, (struct se_msg_hdr *)&tx_msg->header,
> >  			    ELE_GET_INFO_REQ, ELE_GET_INFO_REQ_MSG_SZ,
> true);
> >
> >  	tx_msg->data[0] = upper_32_bits(get_info_addr);
> >  	tx_msg->data[1] = lower_32_bits(get_info_addr);
> >  	tx_msg->data[2] = sizeof(*s_info);
> >  	ret = ele_msg_send_rcv(priv, tx_msg, ELE_GET_INFO_REQ_MSG_SZ,
> rx_msg,
> >  			       ELE_GET_INFO_RSP_MSG_SZ);
> >  	if (ret < 0) {
> >  		ele_get_info_cleanup(priv, get_info_data, get_info_addr,
> > get_info_len);
> 
> ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> ^^^^^^
> 
> [Severity: High]
> If the transaction times out and ele_msg_send_rcv() returns -ETIMEDOUT,
> does freeing the DMA buffer here risk memory corruption?
> 
> The commit message mentions that "DMA free on timeout: -ETIMEDOUT
> means FW is fenced by fw_busy, so the free is safe", but while fw_busy
> prevents the host from sending new commands, it does not stop the firmware
> from finishing the current command late and writing to the DMA address.
> 
> If the buffer is freed and reallocated elsewhere, a late DMA write could silently
> corrupt memory.

FIXED LATER in the same series:

- Patch "firmware: imx: device context dedicated to priv" adds priv->priv_dev_ctx.

- Patch "firmware: imx: adds miscdev" (PATCH v32 5/7) adds
  se_mark_fw_busy() (records priv->fw_busy_dev_ctx = dev_ctx) and
  se_is_fw_busy_ctx(), and rewrites ele_get_info_cleanup() to
  quarantine (return without free) when the owning context is busy;

Disposition: no change in 3/7; resolved by 5/7.

> 
> >  		return ret;
> >  	}
> >
> >  	ret = se_val_rsp_hdr_n_status(priv, rx_msg, ELE_GET_INFO_REQ,
> >  				      ELE_GET_INFO_RSP_MSG_SZ, true);
> 
> [ ... ]
> 
> > diff --git a/drivers/firmware/imx/ele_common.c
> > b/drivers/firmware/imx/ele_common.c
> > index 0000000000000..f40eb4a07982d 100644
> > --- a/drivers/firmware/imx/ele_common.c
> > +++ b/drivers/firmware/imx/ele_common.c
> 
> [ ... ]
> 
> > @@ -61,16 +61,16 @@ int ele_msg_rcv(struct se_if_priv *priv, struct
> > se_clbk_handle *se_clbk_hdl)
> >
> >  	do {
> >  		if (is_rsp_wait_with_timeout) {
> >  			if (time_after_eq(jiffies, deadline_jiffies)) {
> >  				/* Deadline hit: fence hung FW, like the ret==0
> path. */
> >  				spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock,
> flags);
> >  				se_clbk_hdl->rx_msg = NULL;
> >  				if (!completion_done(&se_clbk_hdl->done))
> >  					atomic_set(&priv->fw_busy, 1);
> >  				spin_unlock_irqrestore(&se_clbk_hdl-
> >clbk_rx_lock, flags);
> >  				ret = -ETIMEDOUT;
> >  				break;
> >  			}
> >  			remaining_jiffies = deadline_jiffies - jiffies;
>                                             ^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> [Severity: High]
> Since jiffies is volatile, the compiler will read it again here after the
> time_after_eq() check. If a timer tick increments jiffies exactly between these
> two reads, could this subtraction underflow?
> 
> An underflow would result in a massive timeout value near ULONG_MAX
> being passed to the completion wait functions below, hanging the thread
> indefinitely.

a. ele_common.c ele_msg_rcv(): Added a new variable now.
    With variable now = jiffies, jiffies is sampled once at the top
    of the loop body; time_after_eq(now, deadline_jiffies) gates the
    -ETIMEDOUT branch; remaining_jiffies = deadline_jiffies - now uses the
    same snapshot. 
b. remaining_jiffies is consumed by wait_for_completion_timeout() / wait_for_completion_interruptible_timeout().

This is the one code fix in v33.