Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
New issues: - [Medium] The commit message describes an integer overflow vulnerability in `mana_hwc_alloc_dma_buf()` caused by using unbounded device-advertised message sizes, but this vulnerability is entirely hallucinated and the modified variables are dead code. - [Low] The patch incorrectly clamps `HWC_INIT_DATA_MAX_RESPONSE` using `HW_CHANNEL_MAX_REQUEST_SIZE` instead of `HW_CHANNEL_MAX_RESPONSE_SIZE`. Pre-existing issues: - [High] An unchecked `eq_db.doorbell` index from a hardware event causes an out-of-bounds MMIO write during doorbell ringing. - [High] An unchecked `max_num_cqs` value retrieved from the hardware event allows a malicious device to trigger enormous kernel memory allocations. -- --- Patch [1]: [PATCH] net: mana: cap HWC init max message size to HW_CHANNEL_MAX_REQUEST_SIZE --- commit ea80048daa839e9f12f06618139e15fddebc0ac5 Author: Michael Bommarito <[email protected]> net: mana: cap HWC init max message size to HW_CHANNEL_MAX_REQUEST_SIZE The commit adds bounds checking for max message size during HWC initialization. It claims this prevents an out-of-bounds DMA buffer write in mana_hwc_alloc_dma_buf(). > mana_hwc_init_event_handler() in hw_channel.c stores device-advertised > HWC_INIT_DATA_MAX_REQUEST and HWC_INIT_DATA_MAX_RESPONSE values > without bounds checking. mana_hwc_alloc_dma_buf() later computes the > DMA buffer size as MANA_PAGE_ALIGN(q_depth * max_msg_size) in 32-bit > arithmetic. A malicious device returning a large max_msg_size causes > the product to wrap, allocating a small buffer while laying out > q_depth request slots at the unwrapped stride, placing slots outside > the allocation. [Severity: Medium] Is this description accurate? The modified variables (hwc->hwc_init_max_req_msg_size and hwc->hwc_init_max_resp_msg_size) appear to be dead code that do not affect memory allocation. mana_hwc_alloc_dma_buf() is invoked before the event handler runs (via mana_hwc_init_queues()) using hardcoded sizes (4096), not the hardware-provided ones. > diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c > b/drivers/net/ethernet/microsoft/mana/hw_channel.c > index e3c24d50dad07..94594fba112f9 100644 > --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c > +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c > @@ -150,10 +150,14 @@ static void mana_hwc_init_event_handler(void *ctx, > struct gdma_queue *q_self, > break; > > case HWC_INIT_DATA_MAX_REQUEST: > + if (val == 0 || val > HW_CHANNEL_MAX_REQUEST_SIZE) > + val = HW_CHANNEL_MAX_REQUEST_SIZE; > hwc->hwc_init_max_req_msg_size = val; > break; > > case HWC_INIT_DATA_MAX_RESPONSE: > + if (val == 0 || val > HW_CHANNEL_MAX_REQUEST_SIZE) > + val = HW_CHANNEL_MAX_REQUEST_SIZE; [Severity: Low] Does this incorrectly clamp HWC_INIT_DATA_MAX_RESPONSE using HW_CHANNEL_MAX_REQUEST_SIZE instead of HW_CHANNEL_MAX_RESPONSE_SIZE? > hwc->hwc_init_max_resp_msg_size = val; > break; > [Severity: High] This is a pre-existing issue, but is it possible for a malicious device to trigger an out-of-bounds MMIO write via an unchecked doorbell index? Earlier in mana_hwc_init_event_handler(): case GDMA_EQE_HWC_INIT_EQ_ID_DB: eq_db.as_uint32 = event->details[0]; hwc->cq->gdma_eq->id = eq_db.eq_id; gd->doorbell = eq_db.doorbell; When the driver posts WQEs and rings the doorbell via mana_gd_ring_doorbell(): drivers/net/ethernet/microsoft/mana/gdma_main.c:mana_gd_ring_doorbell() { ... void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index; ... writeq(e.as_uint64, addr); ... } If eq_db.doorbell is excessively large, could it calculate a memory-mapped address that exceeds gc->bar0_size, resulting in an out-of-bounds writeq() that corrupts adjacent mappings? [Severity: High] This is a pre-existing issue, but does the unchecked max_num_cqs value allow a malicious device to exhaust system memory? Later in mana_hwc_init_event_handler(): case HWC_INIT_DATA_MAX_NUM_CQS: gd->gdma_context->max_num_cqs = val; break; This unbounded max_num_cqs value is later used for memory allocation: drivers/net/ethernet/microsoft/mana/hw_channel.c:mana_hwc_establish_channel() { ... gc->cq_table = vcalloc(gc->max_num_cqs, sizeof(struct gdma_queue *)); ... } If max_num_cqs is an exceptionally large 32-bit value (e.g., 0x1FFFFFFF), could vcalloc() attempt to allocate multiple gigabytes of memory and cause a host-level denial of service? -- Sashiko AI review ยท https://sashiko.dev/#/patchset/[email protected]?part=1
