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

[sashiko-bot@kernel.org]

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