[PATCH v2] accel/qaic: Address potential out-of-bounds read in resp_worker()

[PATCH v2] accel/qaic: Address potential out-of-bounds read in resp_worker()

[Youssef Samir]

Although 'commit 2feec5ae5df7 ("accel/qaic: Handle DBC deactivation if the
owner went away")' fixes the scenario it was intended for by walking the
message and only decoding QAIC_TRANS_DEACTIVATE_FROM_DEV, if present, it
skipped over the bounds checking code that is included in decode_message().
This could lead to issues such as reading past the slab allocation's end,
infinite loops or kernel panics. For those issues to happen, a malformed
wire message is needed to be sent from the device.

Instead of duplicating the bounds checking code already present in
decode_message(), use the function inside resp_worker().

Reported-by: Ruikai Peng <ruikai@pwno.io>
Fixes: 2feec5ae5df7 ("accel/qaic: Handle DBC deactivation if the owner went away")
Reviewed-by: Jeff Hugo <jeff.hugo@oss.qualcomm.com>
Reviewed-by: Lizhi Hou <lizhi.hou@amd.com>
Signed-off-by: Youssef Samir <youssef.abdulrahman@oss.qualcomm.com>
---
Changes in V2:
- Remove references to tracepoints that are not upstreamed yet, which
  caused merge conflicts
- Link to V1: https://lore.kernel.org/all/20260414173249.3672053-1-youssef.abdulrahman@oss.qualcomm.com
---
 drivers/accel/qaic/qaic_control.c | 46 ++++++++++++++++---------------
 1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
index bb94d3556904..ecbafab7187c 100644
--- a/drivers/accel/qaic/qaic_control.c
+++ b/drivers/accel/qaic/qaic_control.c
@@ -963,11 +963,13 @@ static int decode_status(struct qaic_device *qdev, void *trans, struct manage_ms
 
 static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
 			  struct wire_msg *msg, struct ioctl_resources *resources,
-			  struct qaic_user *usr)
+			  struct qaic_user *usr, bool orphaned_deactivate)
 {
+	u32 msg_hdr_count = le32_to_cpu(msg->hdr.count);
 	u32 msg_hdr_len = le32_to_cpu(msg->hdr.len);
 	struct wire_trans_hdr *trans_hdr;
 	u32 msg_len = 0;
+	int trans_type;
 	int ret;
 	int i;
 
@@ -975,10 +977,12 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
 	    msg_hdr_len > QAIC_MANAGE_MAX_MSG_LENGTH)
 		return -EINVAL;
 
-	user_msg->len = 0;
-	user_msg->count = le32_to_cpu(msg->hdr.count);
+	if (user_msg) {
+		user_msg->len = 0;
+		user_msg->count = msg_hdr_count;
+	}
 
-	for (i = 0; i < user_msg->count; ++i) {
+	for (i = 0; i < msg_hdr_count; ++i) {
 		u32 hdr_len;
 
 		if (msg_len > msg_hdr_len - sizeof(*trans_hdr))
@@ -990,7 +994,20 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
 		    size_add(msg_len, hdr_len) > msg_hdr_len)
 			return -EINVAL;
 
-		switch (le32_to_cpu(trans_hdr->type)) {
+		trans_type = le32_to_cpu(trans_hdr->type);
+		/*
+		 * orphaned_deactivate is the case where a deactivate response
+		 * is received from the device after the user owning the DBC,
+		 * and the message requesting deactivation, has gone away.
+		 * In this case, only process QAIC_TRANS_DEACTIVATE_FROM_DEV
+		 * transaction and skip the others.
+		 */
+		if (orphaned_deactivate && trans_type != QAIC_TRANS_DEACTIVATE_FROM_DEV) {
+			msg_len += hdr_len;
+			continue;
+		}
+
+		switch (trans_type) {
 		case QAIC_TRANS_PASSTHROUGH_FROM_DEV:
 			ret = decode_passthrough(qdev, trans_hdr, user_msg, &msg_len);
 			break;
@@ -1281,7 +1298,7 @@ static int qaic_manage(struct qaic_device *qdev, struct qaic_user *usr, struct m
 		goto dma_cont_failed;
 	}
 
-	ret = decode_message(qdev, user_msg, rsp, &resources, usr);
+	ret = decode_message(qdev, user_msg, rsp, &resources, usr, false);
 
 dma_cont_failed:
 	free_dbc_buf(qdev, &resources);
@@ -1446,22 +1463,7 @@ static void resp_worker(struct work_struct *work)
 		 * response to the QAIC_TRANS_TERMINATE_TO_DEV transaction,
 		 * otherwise, the user can issue an soc_reset to the device.
 		 */
-		u32 msg_count = le32_to_cpu(msg->hdr.count);
-		u32 msg_len = le32_to_cpu(msg->hdr.len);
-		u32 len = 0;
-		int j;
-
-		for (j = 0; j < msg_count && len < msg_len; ++j) {
-			struct wire_trans_hdr *trans_hdr;
-
-			trans_hdr = (struct wire_trans_hdr *)(msg->data + len);
-			if (le32_to_cpu(trans_hdr->type) == QAIC_TRANS_DEACTIVATE_FROM_DEV) {
-				if (decode_deactivate(qdev, trans_hdr, &len, NULL))
-					len += le32_to_cpu(trans_hdr->len);
-			} else {
-				len += le32_to_cpu(trans_hdr->len);
-			}
-		}
+		decode_message(qdev, NULL, msg, NULL, NULL, true);
 		/* request must have timed out, drop packet */
 		kfree(msg);
 	}
-- 
2.43.0

Re: [PATCH v2] accel/qaic: Address potential out-of-bounds read in resp_worker()

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Insufficient transaction length validation leads to out-of-bounds reads on the wire message and heap buffer overflows when writing to the user message.
- [Critical] Concurrent execution of `release_dbc` causes a double-free of DMA memory.
- [Critical] Concurrent `QAIC_ATTACH_SLICE_BO` ioctls cause list corruption on `dbc->bo_lists`.
- [High] The bounds checking in `decode_message()` fails to account for the message header size, allowing out-of-bounds reads past the slab allocation.
- [High] Uninitialized kernel heap memory can be parsed and leaked to userspace because the MHI download callback trusts the untrusted `msg->hdr.len` field instead of the actual DMA transfer size.
--

--- Patch [1]: [PATCH v2] accel/qaic: Address potential out-of-bounds read in resp_worker() ---
commit 5420c8d76fb8c8bd25b927d7ec9f082dda6e6558
Author: Youssef Samir <youssef.abdulrahman@oss.qualcomm.com>

accel/qaic: Address potential out-of-bounds read in resp_worker()

This commit refactors bounds checking code by using decode_message()
directly inside resp_worker(). It addresses a potential out-of-bounds read
or kernel panic when handling an orphaned DBC deactivation message sent
from the device.

(No Link: tags present in the commit header)

> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index bb94d3556904a..ecbafab7187cb 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
[ ... ]
> @@ -963,11 +963,13 @@ static int decode_status(struct qaic_device *qdev, void *trans, struct manage_ms
>  
>  static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>  			  struct wire_msg *msg, struct ioctl_resources *resources,
> -			  struct qaic_user *usr)
> +			  struct qaic_user *usr, bool orphaned_deactivate)
>  {
> +	u32 msg_hdr_count = le32_to_cpu(msg->hdr.count);
>  	u32 msg_hdr_len = le32_to_cpu(msg->hdr.len);
>  	struct wire_trans_hdr *trans_hdr;
>  	u32 msg_len = 0;
[ ... ]
> @@ -990,7 +994,20 @@ static int decode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>  		    size_add(msg_len, hdr_len) > msg_hdr_len)
>  			return -EINVAL;

[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check allow
the offset to exceed the physical buffer size?

Since size_add(msg_len, hdr_len) is compared to msg_hdr_len instead of
(msg_hdr_len - sizeof(msg->hdr)), msg_len can reach the total message length.
However, the payload buffer msg->data only has msg_hdr_len - sizeof(msg->hdr)
bytes.

Because the 32-byte header size is not subtracted, msg->data + msg_len can
point 32 bytes past the end of the out_buf slab allocation when msg_hdr_len
is QAIC_MANAGE_MAX_MSG_LENGTH.

Could this out-of-bounds memory then be copied to the user-facing message
buffer in decode_passthrough():

drivers/accel/qaic/qaic_control.c:decode_passthrough() {
    ...
    memcpy(out_trans->data, in_trans->data, len - sizeof(in_trans->hdr));
    ...
}

facilitating an information leak?


[Severity: Critical]
This is a pre-existing issue, but does this loop verify the required size for
the specific transaction type?

The bounds check only verifies that hdr_len is >= 8 (sizeof(*trans_hdr)). 

If a malicious or malfunctioning device sends a QAIC_TRANS_ACTIVATE_FROM_DEV 
transaction with a truncated hdr_len (e.g., 8 bytes), could decode_activate()
unconditionally write 24 bytes of fields (status, dbc_id, options) to
out_trans?

drivers/accel/qaic/qaic_control.c:decode_activate() {
    ...
    out_trans->hdr.type = le32_to_cpu(in_trans->hdr.type);
    out_trans->hdr.len = len;
    out_trans->status = le32_to_cpu(in_trans->status);
    out_trans->dbc_id = le32_to_cpu(in_trans->dbc_id);
    out_trans->options = le64_to_cpu(in_trans->options);
    ...
}

Since decode_activate() checks the remaining capacity in user_msg using the
8-byte hdr_len, could this result in writing 16 bytes past the end of the
user_msg heap allocation?

[ ... ]

Additionally, there are a few other pre-existing issues in this subsystem:

[Severity: High]
This isn't a bug introduced by this patch, but can uninitialized kernel heap
memory be parsed and leaked to userspace?

In qaic_mhi_dl_xfer_cb(), if a device sends a short MHI packet but with a
large msg->hdr.len field, the actual DMA transfer size does not appear to be
validated against msg->hdr.len:

drivers/accel/qaic/qaic_control.c:qaic_mhi_dl_xfer_cb() {
    ...
    if (mhi_result->transaction_status || msg->hdr.magic_number != MANAGE_MAGIC_NUMBER) {
        kfree(msg);
        return;
    }

    resp = kmalloc_obj(*resp, GFP_ATOMIC);
    ...
}

Could decode_message() subsequently parse the uninitialized kmalloc memory as
valid transactions up to the length dictated by the untrusted msg->hdr.len?


[Severity: Critical]
This is a pre-existing issue, but does concurrent execution of release_dbc()
cause a double-free of DMA memory?

If a user issues a deactivate ioctl which gets interrupted, and resp_worker()
processes the orphaned response, it calls release_dbc(). Concurrently, the
user retrying the deactivate ioctl could also successfully trigger
release_dbc():

drivers/accel/qaic/qaic_data.c:release_dbc() {
    ...
    if (!dbc->in_use)
        return;

    wakeup_dbc(qdev, dbc_id);

    dma_free_coherent(&qdev->pdev->dev, dbc->total_size, dbc->req_q_base, dbc->dma_addr);
    ...
}

Since dbc->in_use is checked and modified without mutual exclusion, can both
threads pass the !dbc->in_use check simultaneously and call
dma_free_coherent() on the same dbc->req_q_base pointer?


[Severity: Critical]
This is a pre-existing issue, but can concurrent QAIC_ATTACH_SLICE_BO ioctls
cause list corruption on dbc->bo_lists?

When multiple threads issue QAIC_ATTACH_SLICE_BO ioctls concurrently for
different BOs on the same DBC, the only locks held during list_add_tail() are
the per-BO mutex (bo->lock) and SRCU read locks:

drivers/accel/qaic/qaic_data.c:qaic_attach_slice_bo_ioctl() {
    ...
    bo->sliced = true;
    list_add_tail(&bo->bo_list, &bo->dbc->bo_lists);
    srcu_read_unlock(&dbc->ch_lock, rcu_id);
    mutex_unlock(&bo->lock);
    ...
}

Because SRCU read locks do not provide mutual exclusion between readers, can
multiple threads concurrently execute list_add_tail() on the shared
dbc->bo_lists?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731152344.1905882-1-youssef.abdulrahman@oss.qualcomm.com?part=1