Re: [PATCH] accel/qaic: use sizeof(*trans_hdr) for transaction length check

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Missing transaction-specific size validation in encode_message and decode_message allows kernel heap out-of-bounds access.
- [Critical] Incorrect remaining payload length calculation in decode_message allows reading past the end of the device response buffer.
- [High] Missing `kfree(out_buf)` in `msg_xfer()` error path.
- [High] `decode_deactivate()` advances the message parsing length by a hardcoded size instead of the transaction header length.
- [High] `encode_activate` overwrites `resources->buf` with a new DMA coherent allocation without checking for or freeing any pre-existing allocation.
- [High] `save_dbc_buf` improperly ignores the return value of `wait_event_interruptible`, leading to severe race conditions if interrupted by a signal.
- [Medium] Malformed packet creation in `encode_status()` due to unvalidated user-provided length.
--

commit 5efe51dab6d84a356365c2dd664f57b3a5563a27
Author: Muhammad Bilal <meatuni001@gmail.com>

accel/qaic: use sizeof(*trans_hdr) for transaction length check

The commit fixes a bug in encode_message() where the transaction length
was being compared against the pointer size instead of the struct size.
It correctly replaces sizeof(trans_hdr) with sizeof(*trans_hdr).

> diff --git a/drivers/accel/qaic/qaic_control.c b/drivers/accel/qaic/qaic_control.c
> index bb94d3556904a..50bf3340e49ce 100644
> --- a/drivers/accel/qaic/qaic_control.c
> +++ b/drivers/accel/qaic/qaic_control.c
> @@ -786,7 +786,7 @@ static int encode_message(struct qaic_device *qdev, struct manage_msg *user_msg,
>  			break;
>  		}
>  		trans_hdr = (struct qaic_manage_trans_hdr *)(user_msg->data + user_len);
> -		if (trans_hdr->len < sizeof(trans_hdr) ||
> +		if (trans_hdr->len < sizeof(*trans_hdr) ||
>  		    size_add(user_len, trans_hdr->len) > user_msg->len) {
>  			ret = -EINVAL;
>  			break;

[Severity: Critical]
This is a pre-existing issue, but does `encode_message()` validate that the
transaction length is sufficient for the specific transaction structure?

For example, in `encode_dma()`:

drivers/accel/qaic/qaic_control.c:encode_dma() {
    ...
    struct qaic_manage_trans_dma_xfer *in_trans = trans;
    if (check_add_overflow(in_trans->addr, resources->xferred_dma_size, &xfer_start_addr))
    ...
}

If a user supplies a valid transaction near the end of the buffer with a
length of 8, is it possible for `encode_dma()` to read `in_trans->addr` out
of bounds since the generic checks only verify the 8-byte generic header?

Similarly, in `decode_activate()`:

drivers/accel/qaic/qaic_control.c:decode_activate() {
    ...
    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);
    ...
}

Can this write out of bounds past the kernel allocation if the device
supplies a truncated transaction?

[Severity: High]
This is a pre-existing issue, but are we unconditionally leaking `out_buf`
in the `msg_xfer()` error path?

drivers/accel/qaic/qaic_control.c:msg_xfer() {
    ...
    if (ret) {
        mutex_unlock(&qdev->cntl_mutex);
        return ERR_PTR(ret);
    }
    ...
}

If `mhi_queue_buf()` fails to queue the buffer, this error path unlocks the
mutex and returns immediately. Shouldn't we be freeing the buffer here?

[Severity: Medium]
This is a pre-existing issue, but could `encode_status()` send a malformed
packet?

drivers/accel/qaic/qaic_control.c:encode_status() {
    ...
    trans_wrapper->len = sizeof(*out_trans);
    out_trans = (struct wire_trans_status_to_dev *)&trans_wrapper->trans;

    out_trans->hdr.type = cpu_to_le32(QAIC_TRANS_STATUS_TO_DEV);
    out_trans->hdr.len = cpu_to_le32(in_trans->hdr.len);
    msg->hdr.len = cpu_to_le32(msg_hdr_len + in_trans->hdr.len);
    ...
}

The user-provided length is injected into the wire header, but the actual
wrapper transfer length is clamped to the structure size. Will MHI transmit
fewer bytes than the wire packet header claims if `in_trans->hdr.len` is
greater than 8?

[Severity: High]
This is a pre-existing issue, but does `decode_deactivate()` advance the
message parsing length correctly?

drivers/accel/qaic/qaic_control.c:decode_deactivate() {
    ...
    *msg_len += sizeof(*in_trans);
    ...
}

If the firmware sends a larger transaction structure, will the parser lose
synchronization by advancing by a hardcoded size instead of the parsed
dynamic header length?

[Severity: High]
This is a pre-existing issue, but might `encode_activate()` overwrite
`resources->buf` and leak the previous allocation?

drivers/accel/qaic/qaic_control.c:encode_activate() {
    ...
    buf = dma_alloc_coherent(&qdev->pdev->dev, size, &dma_addr, GFP_KERNEL);
    if (!buf)
        return -ENOMEM;
    ...
    resources->buf = buf;
    ...
}

If userspace provides a message with multiple activation transactions,
could a local user loop this to exhaust contiguous physical memory by
continuously overwriting the tracking pointer without freeing?

[Severity: High]
This is a pre-existing issue, but are we at risk of corrupting hardware
state tracking if interrupted by a signal in `save_dbc_buf()`?

drivers/accel/qaic/qaic_control.c:save_dbc_buf() {
    ...
    wait_event_interruptible(qdev->dbc[dbc_id].dbc_release, !qdev->dbc[dbc_id].in_use);
    qdev->dbc[dbc_id].req_q_base = resources->buf;
    ...
}

If the process receives a signal, the wait returns `-ERESTARTSYS`
immediately. By ignoring this return value, does the driver proceed to
overwrite the DBC queues and forcibly set `in_use = true` while the DBC
is still actively used by another context?

[Severity: Critical]
This is a pre-existing issue, but wouldn't the bounds check in
`decode_message()` allow reading past the end of the device response buffer?

drivers/accel/qaic/qaic_control.c:decode_message() {
    ...
    if (hdr_len < sizeof(*trans_hdr) ||
        size_add(msg_len, hdr_len) > msg_hdr_len)
        return -EINVAL;
    ...
}

The variable `msg_hdr_len` includes the 32-byte header itself, while
`msg_len` tracks the offset in `msg->data`. By comparing against
`msg_hdr_len` directly instead of `msg_hdr_len - sizeof(msg->hdr)`, does
this allow `hdr_len` to exceed the actual remaining bytes in `msg->data` by
32 bytes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617212520.59801-1-meatuni001@gmail.com?part=1