Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
New issues: - [High] Missing `READ_ONCE()` for `sge->address` risks Time-of-Check to Time-of-Use (TOCTOU) bypass of the newly added bounds check. Pre-existing issues: - [High] Missing bounds check on `dma_oob->inline_oob_size_div4` allows an out-of-bounds memory read past the DMA WQE ring buffer. - [High] Returning early without reposting the RX WQE buffer leads to RX queue starvation. -- commit 4f8436d78a8ca78fb96f7252f756af92fc42ccaa Author: Aditya Garg <[email protected]> net: mana: validate rx_req_idx to prevent out-of-bounds array access This commit adds a bounds check on rx_req_idx in mana_hwc_rx_event_handler() to prevent an out-of-bounds access into the reqs[] array. The index is derived from unencrypted DMA-coherent memory which can be modified by the hardware at any time in Confidential VMs. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -258,11 +258,17 @@ static void mana_hwc_rx_event_handler(void *ctx, u32 > gdma_rxq_id, > wqe = mana_gd_get_wqe_ptr(rq, rx_oob->wqe_offset / GDMA_WQE_BU_SIZE); > dma_oob = (struct gdma_wqe *)wqe; > > sge = (struct gdma_sge *)(wqe + 8 + dma_oob->inline_oob_size_div4 * 4); [Severity: High] This isn't a bug introduced by this patch, but is there a missing bounds check on dma_oob->inline_oob_size_div4? Since dma_oob->inline_oob_size_div4 is read directly from unencrypted, shared DMA memory, a potentially malicious host could manipulate it. It is a 3-bit field, so an attacker could set it to 7. Could this cause the sge pointer calculation to point 36 bytes into the 32-byte WQE buffer, resulting in an out-of-bounds read of up to 12 bytes when reading sge->address? If the WQE is at the end of the page-aligned queue memory buffer, could this read past the allocated DMA region and trigger a fatal page fault? > > /* Select the RX work request for virtual address and for reposting. */ > rq_base_addr = hwc_rxq->msg_buf->mem_info.dma_handle; > rx_req_idx = (sge->address - rq_base_addr) / hwc->max_req_msg_size; [Severity: High] Does sge->address need to be read using READ_ONCE()? Since sge->address resides in unencrypted, shared DMA memory, the compiler might fetch its value multiple times. If the division by hwc->max_req_msg_size is optimized into a bitwise shift, the compiler might re-read sge->address from DMA memory after the bounds check has passed when indexing the array. Could a malicious host concurrently modify the memory to bypass the new bounds check completely? > > + if (rx_req_idx >= hwc_rxq->msg_buf->num_reqs) { > + dev_err(hwc->dev, "HWC RX: wrong rx_req_idx=%llu, > num_reqs=%u\n", > + rx_req_idx, hwc_rxq->msg_buf->num_reqs); > + return; > + } > + [Severity: High] This is a pre-existing issue, but does returning early here (and further below for invalid msg_id) drop the request without reposting the RX WQE buffer? If a malicious host repeatedly sends valid CQEs with an invalid msg_id or an out-of-bounds rx_req_idx, it seems the function drops the response without replenishing the queue via mana_hwc_post_rx_wqe(). Could this permanently leak the RX WQE buffers and starve the queue, causing a denial of service for the hardware channel? > rx_req = &hwc_rxq->msg_buf->reqs[rx_req_idx]; > resp = (struct gdma_resp_hdr *)rx_req->buf_va; -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
