From: Pankaj Gupta <[email protected]>
Add priv_dev_ctx to prepare enabling misc-device context based send-receive
path, to communicate with FW.
No functionality change.
Signed-off-by: Pankaj Gupta <[email protected]>
Reviewed-by: Frank Li <[email protected]>
Signed-off-by: Frank Li <[email protected]>
---
Changes from v28 to v29:
Address both findings from the Sashiko AI review of patch 4/7
Addressed (drivers/firmware/imx/ele_common.c):
- [High] Data race / NULL pointer dereference when initializing
waiting_rsp_clbk_hdl in ele_msg_send_rcv():
dev_ctx, rx_msg_sz and rx_msg were published without holding
clbk_rx_lock. A delayed firmware response from a previous command could
run se_if_rx_callback() concurrently and, due to CPU/compiler
reordering, observe the new rx_msg before the new dev_ctx was visible,
then dereference se_clbk_hdl->dev_ctx->devname while dev_ctx was still
NULL from the previous cleanup.
Publish all three fields under clbk_rx_lock so the write of dev_ctx
(always non-NULL) is ordered before rx_msg. ele_msg_send_rcv_cleanup()
already clears dev_ctx, rx_msg and rx_msg_sz together under the same
lock, and se_if_rx_callback() reads them under the lock and only
dereferences dev_ctx after confirming rx_msg != NULL. The lock provides
the memory ordering, so the invariant "rx_msg != NULL implies dev_ctx
!= NULL" holds and the dereference is safe. The RX callback also reads
dev_ctx->devname under the lock (cmd_tag path additionally checks
dev_ctx explicitly) and caches it in a local before dropping the lock.
- [Low] Unused variable flags in ele_msg_send_rcv():
flags is now consumed by the spin_lock_irqsave()/spin_unlock_irqrestore()
pair added for the fix above, so it is no longer unused and the
-Wunused-variable warning is gone.
Follow-up Sashiko AI review of patch 4/7 (4 findings). All already
resolved in this patch; no functional change required beyond a cosmetic
whitespace cleanup:
- [High][NEW] Accessing se_clbk_hdl->dev_ctx->devname outside
clbk_rx_lock in the cmd_tag path (UAF / NULL deref):
Already fixed. In se_if_rx_callback(), the cmd_tag path holds
clbk_rx_lock across the dev_dbg() that reads dev_ctx->devname and only
proceeds after the explicit "!se_clbk_hdl->dev_ctx" check. The devname
used by the post-unlock dev_err() is cached into a local (devname)
while the lock is still held, so no dev_ctx dereference happens outside
the lock.
- [High] Incorrect devm registration order frees priv while the RX
mailbox channel is still active (NULL deref):
Already fixed in drivers/firmware/imx/se_ctrl.c. se_if_probe()
registers devm_add_action_or_reset(dev, se_if_probe_cleanup, pdev)
before requesting the tx/rx mailbox channels, so LIFO teardown frees
the channels (se_if_probe_cleanup) before priv is released.
- [High] Memory leak of priv when early probe steps fail:
Already fixed by the same change. priv is allocated, stored via
dev_set_drvdata(), and its release is tied to se_if_probe_cleanup(),
which is registered early; any subsequent probe failure unwinds through
the devres action that frees priv.
- [High] Unprotected read of rx_msg_sz creates a TOCTOU race leading to
response truncation on late interrupts:
Already fixed. The rsp_tag path in se_if_rx_callback() reads
exp_rx_msg_sz = se_clbk_hdl->rx_msg_sz only after acquiring
clbk_rx_lock (and after confirming rx_msg != NULL), so the size cannot
change between the check and the memcpy().
Reported-by: sashiko-bot <[email protected]>
Closes: https://sashiko.dev/#/patchset/[email protected]?part=4
---
drivers/firmware/imx/ele_base_msg.c | 15 +++++-----
drivers/firmware/imx/ele_common.c | 58 ++++++++++++++++++++++---------------
drivers/firmware/imx/ele_common.h | 8 ++---
drivers/firmware/imx/se_ctrl.c | 36 +++++++++++++++++++++++
drivers/firmware/imx/se_ctrl.h | 9 ++++++
5 files changed, 92 insertions(+), 34 deletions(-)
diff --git a/drivers/firmware/imx/ele_base_msg.c b/drivers/firmware/imx/ele_base_msg.c
index a2ede9c01691..524ebc0f697e 100644
--- a/drivers/firmware/imx/ele_base_msg.c
+++ b/drivers/firmware/imx/ele_base_msg.c
@@ -66,8 +66,9 @@ int ele_get_info(struct se_if_priv *priv, struct ele_dev_info *s_info)
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);
+
+ ret = ele_msg_send_rcv(priv->priv_dev_ctx, 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, ELE_GET_INFO_BUFF_SZ);
return ret;
@@ -112,8 +113,8 @@ int ele_ping(struct se_if_priv *priv)
se_fill_cmd_msg_hdr(priv, (struct se_msg_hdr *)&tx_msg->header,
ELE_PING_REQ, ELE_PING_REQ_SZ, true);
- ret = ele_msg_send_rcv(priv, tx_msg, ELE_PING_REQ_SZ, rx_msg,
- ELE_PING_RSP_SZ);
+ ret = ele_msg_send_rcv(priv->priv_dev_ctx, tx_msg, ELE_PING_REQ_SZ,
+ rx_msg, ELE_PING_RSP_SZ);
if (ret < 0)
return ret;
@@ -160,7 +161,7 @@ int ele_service_swap(struct se_if_priv *priv,
if (ret)
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;
@@ -208,7 +209,7 @@ int ele_fw_authenticate(struct se_if_priv *priv, dma_addr_t contnr_addr,
tx_msg->data[1] = 0;
tx_msg->data[2] = lower_32_bits(img_addr);
- ret = ele_msg_send_rcv(priv, tx_msg, ELE_FW_AUTH_REQ_SZ, rx_msg,
+ ret = ele_msg_send_rcv(priv->priv_dev_ctx, tx_msg, ELE_FW_AUTH_REQ_SZ, rx_msg,
ELE_FW_AUTH_RSP_MSG_SZ);
if (ret < 0)
return ret;
@@ -246,7 +247,7 @@ int ele_debug_dump(struct se_if_priv *priv)
do {
memset(rx_msg, 0x0, ELE_DEBUG_DUMP_RSP_SZ);
- ret = ele_msg_send_rcv(priv, tx_msg, ELE_DEBUG_DUMP_REQ_SZ,
+ ret = ele_msg_send_rcv(priv->priv_dev_ctx, tx_msg, ELE_DEBUG_DUMP_REQ_SZ,
rx_msg, ELE_DEBUG_DUMP_RSP_SZ);
if (ret < 0)
return ret;
diff --git a/drivers/firmware/imx/ele_common.c b/drivers/firmware/imx/ele_common.c
index f9be8b8556ad..3811c37d1754 100644
--- a/drivers/firmware/imx/ele_common.c
+++ b/drivers/firmware/imx/ele_common.c
@@ -46,8 +46,9 @@ int se_update_msg_chksum(u32 *msg, u32 msg_len)
return 0;
}
-int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle *se_clbk_hdl)
+int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk_hdl)
{
+ struct se_if_priv *priv = dev_ctx->priv;
bool is_rsp_wait_with_timeout = false;
bool wait_uninterruptible = false;
unsigned long remaining_jiffies;
@@ -126,7 +127,7 @@ int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle *se_clbk_hdl)
return ret;
}
-int ele_msg_send(struct se_if_priv *priv,
+int ele_msg_send(struct se_if_device_ctx *dev_ctx,
void *tx_msg,
int tx_msg_sz)
{
@@ -138,9 +139,9 @@ int ele_msg_send(struct se_if_priv *priv,
* carried in the message.
*/
if (header->size << 2 != tx_msg_sz) {
- dev_err(priv->dev,
- "User buf hdr: 0x%x, sz mismatced with input-sz (%d != %d).",
- *(u32 *)header, header->size << 2, tx_msg_sz);
+ dev_err(dev_ctx->priv->dev,
+ "%s: User buf hdr: 0x%x, sz mismatched with input-sz (%d != %d).",
+ dev_ctx->devname, *(u32 *)header, header->size << 2, tx_msg_sz);
return -EINVAL;
}
@@ -150,9 +151,10 @@ int ele_msg_send(struct se_if_priv *priv,
* caller-provided tx_msg pointer after mbox_send_message() returns, so
* the caller-owned buffer may be released after a successful send.
*/
- err = mbox_send_message(priv->tx_chan, tx_msg);
+ err = mbox_send_message(dev_ctx->priv->tx_chan, tx_msg);
if (err < 0) {
- dev_err(priv->dev, "Error: mbox_send_message failure.\n");
+ dev_err(dev_ctx->priv->dev,
+ "%s: Error: mbox_send_message failure.", dev_ctx->devname);
return err;
}
@@ -164,34 +166,40 @@ static void ele_msg_send_rcv_cleanup(struct se_if_priv *priv)
unsigned long flags;
spin_lock_irqsave(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
+ priv->waiting_rsp_clbk_hdl.dev_ctx = NULL;
priv->waiting_rsp_clbk_hdl.rx_msg = NULL;
priv->waiting_rsp_clbk_hdl.rx_msg_sz = 0;
spin_unlock_irqrestore(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
}
-/* 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)
+/* API used for send/receive blocking calls. */
+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;
+ unsigned long flags;
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);
+ spin_lock_irqsave(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
+ 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;
+ spin_unlock_irqrestore(&priv->waiting_rsp_clbk_hdl.clbk_rx_lock, flags);
- err = ele_msg_send(priv, tx_msg, tx_msg_sz);
+ err = ele_msg_send(dev_ctx, tx_msg, tx_msg_sz);
if (err < 0) {
ele_msg_send_rcv_cleanup(priv);
return err;
}
- err = ele_msg_rcv(priv, &priv->waiting_rsp_clbk_hdl);
+ err = ele_msg_rcv(dev_ctx, &priv->waiting_rsp_clbk_hdl);
if (priv->waiting_rsp_clbk_hdl.signal_rcvd) {
/*
@@ -202,7 +210,8 @@ int ele_msg_send_rcv(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz,
if (err > 0)
err = -ERESTARTSYS;
priv->waiting_rsp_clbk_hdl.signal_rcvd = false;
- dev_dbg(priv->dev, "Err[0x%x]:Interrupted by signal.", err);
+ dev_dbg(priv->dev, "%s: Err[0x%x]:Interrupted by signal.",
+ dev_ctx->devname, err);
}
ele_msg_send_rcv_cleanup(priv);
@@ -231,6 +240,7 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
{
struct se_clbk_handle *se_clbk_hdl;
struct device *dev = mbox_cl->dev;
+ const char *devname = NULL;
struct se_msg_hdr *header;
bool sz_mismatch = false;
struct se_if_priv *priv;
@@ -254,7 +264,7 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
if (header->tag == priv->if_defs->cmd_tag) {
se_clbk_hdl = &priv->cmd_receiver_clbk_hdl;
spin_lock_irqsave(&se_clbk_hdl->clbk_rx_lock, flags);
- if (!se_clbk_hdl->rx_msg) {
+ if (!se_clbk_hdl->dev_ctx || !se_clbk_hdl->rx_msg) {
spin_unlock_irqrestore(&se_clbk_hdl->clbk_rx_lock, flags);
dev_warn(dev, "No command receiver registered for message: %.8x\n",
*((u32 *)header));
@@ -268,8 +278,8 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
* SE_IOCTL_ENABLE_CMD_RCV and is not subject to the timeout/circuit-
* breaker handling used for rsp_tag messages.
*/
- dev_dbg(dev, "Selecting cmd receiver: for mesg header:0x%x.",
- *(u32 *)header);
+ dev_dbg(dev, "Selecting cmd receiver:%s for mesg header:0x%x.",
+ se_clbk_hdl->dev_ctx->devname, *(u32 *)header);
/*
* Pre-allocated buffer of MAX_NVM_MSG_LEN
@@ -284,13 +294,14 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
* Clamp the copy length to the pre-allocated receiver buffer (MAX_NVM_MSG_LEN).
*/
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) with different sz(%d != %d).\n",
- *(u32 *)header,
+ "%s: CMD-RCVER NVM: hdr(0x%x) with different sz(%d != %d).\n",
+ devname, *(u32 *)header,
(header->size << 2), rx_msg_sz);
} else if (header->tag == priv->if_defs->rsp_tag) {
bool exception_for_sz_mismatch = check_hdr_exception_for_sz(priv, header);
@@ -312,8 +323,8 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
return;
}
exp_rx_msg_sz = se_clbk_hdl->rx_msg_sz;
- dev_dbg(dev, "Selecting resp waiter: for mesg header:0x%x.",
- *(u32 *)header);
+ dev_dbg(dev, "Selecting resp waiter:%s for mesg header:0x%x.",
+ se_clbk_hdl->dev_ctx->devname, *(u32 *)header);
/*
* For rsp_tag traffic, the sender provides the expected response
@@ -325,14 +336,15 @@ void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg)
sz_mismatch = true;
se_clbk_hdl->rx_msg_sz = min(rx_msg_sz, exp_rx_msg_sz);
+ 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) with different sz(%d != %d).\n",
- *(u32 *)header,
+ "%s: Rsp to CMD: hdr(0x%x) with different sz(%d != %d).\n",
+ devname, *(u32 *)header,
(header->size << 2), exp_rx_msg_sz);
} else {
dev_err(dev, "Failed to select a device for message: %.8x\n",
diff --git a/drivers/firmware/imx/ele_common.h b/drivers/firmware/imx/ele_common.h
index 7bf2febefc45..07e6b6a1bafa 100644
--- a/drivers/firmware/imx/ele_common.h
+++ b/drivers/firmware/imx/ele_common.h
@@ -16,12 +16,12 @@
int se_update_msg_chksum(u32 *msg, u32 msg_len);
-int ele_msg_rcv(struct se_if_priv *priv, struct se_clbk_handle *se_clbk_hdl);
+int ele_msg_rcv(struct se_if_device_ctx *dev_ctx, struct se_clbk_handle *se_clbk_hdl);
-int ele_msg_send(struct se_if_priv *priv, void *tx_msg, int tx_msg_sz);
+int ele_msg_send(struct se_if_device_ctx *dev_ctx, void *tx_msg, int tx_msg_sz);
-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);
void se_if_rx_callback(struct mbox_client *mbox_cl, void *msg);
diff --git a/drivers/firmware/imx/se_ctrl.c b/drivers/firmware/imx/se_ctrl.c
index e9e2f48f4f71..b053dd00286c 100644
--- a/drivers/firmware/imx/se_ctrl.c
+++ b/drivers/firmware/imx/se_ctrl.c
@@ -284,6 +284,36 @@ static int get_se_soc_info(struct se_if_priv *priv, const struct se_soc_info *se
return 0;
}
+static int init_misc_device_context(struct se_if_priv *priv, int ch_id,
+ struct se_if_device_ctx **new_dev_ctx)
+{
+ const char *err_str = "Failed to allocate memory";
+ struct se_if_device_ctx *dev_ctx;
+ int ret = -ENOMEM;
+
+ dev_ctx = kzalloc_obj(*dev_ctx, GFP_KERNEL);
+
+ if (!dev_ctx)
+ return ret;
+
+ dev_ctx->devname = kasprintf(GFP_KERNEL, "%s0_ch%d",
+ get_se_if_name(priv->if_defs->se_if_type),
+ ch_id);
+ if (!dev_ctx->devname)
+ goto exit;
+
+ dev_ctx->priv = priv;
+ *new_dev_ctx = dev_ctx;
+
+ return 0;
+exit:
+ *new_dev_ctx = NULL;
+
+ kfree(dev_ctx->devname);
+ kfree(dev_ctx);
+ return dev_err_probe(priv->dev, ret, "%s", err_str);
+}
+
static int se_if_request_channel(struct device *dev, struct mbox_chan **chan,
struct mbox_client *cl, const char *name)
{
@@ -411,6 +441,12 @@ static int se_if_probe(struct platform_device *pdev)
load_fw->is_fw_tobe_loaded = true;
}
+ ret = init_misc_device_context(priv, 0, &priv->priv_dev_ctx);
+ if (ret)
+ return dev_err_probe(dev, ret,
+ "Failed[0x%x] to create device contexts.",
+ ret);
+
/* By default, there is no pending FW to be loaded.*/
if (se_info->imem_state_mgmt) {
load_fw = get_load_fw_instance(priv);
diff --git a/drivers/firmware/imx/se_ctrl.h b/drivers/firmware/imx/se_ctrl.h
index 00ac892997a6..24ef97d01be1 100644
--- a/drivers/firmware/imx/se_ctrl.h
+++ b/drivers/firmware/imx/se_ctrl.h
@@ -20,6 +20,7 @@
#define MESSAGING_VERSION_7 0x7
struct se_clbk_handle {
+ struct se_if_device_ctx *dev_ctx;
struct completion done;
bool signal_rcvd;
u32 rx_msg_sz;
@@ -44,6 +45,12 @@ struct se_imem_buf {
u32 state;
};
+/* Private struct for each char device instance. */
+struct se_if_device_ctx {
+ struct se_if_priv *priv;
+ const char *devname;
+};
+
/* Header of the messages exchange with the EdgeLock Enclave */
struct se_msg_hdr {
u8 ver;
@@ -106,6 +113,8 @@ struct se_if_priv {
struct se_fw_load_info load_fw;
atomic_t fw_busy;
+
+ struct se_if_device_ctx *priv_dev_ctx;
};
char *get_se_if_name(u8 se_if_id);
--
2.43.0
lmpx.com only provides a reader for public news (NNTP) servers. It is not
affiliated with the servers or forums shown here and is not responsible for
the content of articles, which is written by their respective authors.