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 <[email protected]> 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/[email protected]?part=1
