RE: [PATCH v31 4/7] firmware: imx: device context dedicated to priv

> - [High] se_if_probe_cleanup() frees priv->priv_dev_ctx before freeing the
> mailbox channels, creating a race condition where the mailbox RX callback can
> access a freed devname string on the error logging path.

Confirmed. Freeing priv_dev_ctx (hence priv_dev_ctx->devname) while the rx
channel is still live leaves the sz_mismatch dev_err() in se_if_rx_callback()
free to dereference a dangling devname. Fix is a reorder in
se_if_probe_cleanup() (see the last hunk).

> > @@ -294,13 +301,14 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> >  		se_clbk_hdl->rx_msg_sz = min_t(u32, rx_msg_sz, MAX_NVM_MSG_LEN);
> > +		devname = se_clbk_hdl->dev_ctx->devname;
> >  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
> >  		complete(&se_clbk_hdl->done);
> >  		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
> >  		if (sz_mismatch)
> >  			dev_err(dev,
> > -				"CMD-RCVER NVM: hdr(0x%x) ...",
> > +				"%s: CMD-RCVER NVM: hdr(0x%x) ...",
> > +				devname, *(u32 *)header,

The cache-then-log pattern here is intentional: devname is read under
clbk_rx_lock, and the dev_err() is deliberately emitted after dropping the
spinlock so we do not log under the lock. The pattern is safe as long as the
priv_dev_ctx that devname points into is not freed while a callback may still
run. That guarantee is missing in this commit's teardown, not in this hunk. No
change here.

> > @@ -335,14 +343,15 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
> > +		devname = se_clbk_hdl->dev_ctx->devname;
> >  		memcpy(se_clbk_hdl->rx_msg, msg, se_clbk_hdl->rx_msg_sz);
> >  		complete(&se_clbk_hdl->done);
> >  		spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
> >  		if (sz_mismatch)
> >  			dev_err(dev,
> > -				"Rsp to CMD: hdr(0x%x) ...",
> > +				"%s: Rsp to CMD: hdr(0x%x) ...",
> > +				devname, *(u32 *)header,

Same disposition. On the rsp_tag path devname is
priv->waiting_rsp_clbk_hdl.dev_ctx->devname, which for a probe transaction is
priv->priv_dev_ctx->devname - exactly the object teardown frees too early. Note
complete() here wakes the probe waiter, which can drive devres unwind and enter
se_if_probe_cleanup() while this callback is still preempted between the
spin_unlock and the dev_err(). Lifetime must be enforced in the cleanup path. No
change here.

> > @@ -309,6 +339,11 @@ static void se_if_probe_cleanup(void *plat_dev)
> >  	if (!priv)
> >  		return;
> > +	if (priv->priv_dev_ctx) {
> > +		kfree(priv->priv_dev_ctx->devname);
> > +		kfree(priv->priv_dev_ctx);
> > +	}
> > +
> >  	if (priv->rx_chan)
> >  		mbox_free_channel(priv->rx_chan);
> >  	if (priv->tx_chan)
> >  		mbox_free_channel(priv->tx_chan);

This is the defect. priv_dev_ctx (and devname) are freed while priv->rx_chan is
still registered, so a se_if_rx_callback() that has already cached devname and
is about to call dev_err() can dereference freed memory.

Fix: move the priv_dev_ctx/devname free below mbox_free_channel(). Freeing the
rx channel first quiesces the rx path (no new callbacks, and any in-flight one
has completed), so devname is no longer reachable by the callback when it is
freed. Placing it just before kfree(priv) at the end of the function:

	diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
	--- a/drivers/firmware/imx/se_ctrl.c
	+++ b/drivers/firmware/imx/se_ctrl.c
	@@ -339,11 +339,6 @@ static void se_if_probe_cleanup(void *plat_dev)
	 	if (!priv)
	 		return;

	-	if (priv->priv_dev_ctx) {
	-		kfree(priv->priv_dev_ctx->devname);
	-		kfree(priv->priv_dev_ctx);
	-	}
	-
	 	if (priv->rx_chan)
	 		mbox_free_channel(priv->rx_chan);
	 	if (priv->tx_chan)
	@@ -363,6 +358,11 @@ static void se_if_probe_cleanup(void *plat_dev)

	 	dev_set_drvdata(dev, NULL);

	+	if (priv->priv_dev_ctx) {
	+		kfree(priv->priv_dev_ctx->devname);
	+		kfree(priv->priv_dev_ctx);
	+	}
	+
	 	kfree(priv);
	 }

This gives the required ordering:

	mbox_free_channel(rx)  ->  kfree(priv_dev_ctx->devname)  ->  kfree(priv)

Because the rx channel is torn down before devname is freed, a
se_if_rx_callback() can neither start nor still be running when devname goes
away, closing the window you describe.

> Would it be safer to free priv_dev_ctx after the mailbox channels are freed?

Yes. The reorder above does exactly that. Will fold it into the next respin of this patch.
Next re-spin depends of this patch depends if there are any more comments in 5/7, require code changes.
If not then there will be no respin plan, as this change is replaced and cleaned in 5/7.

--