Re: [PATCH] net: mana: cap HWC init max message size to HW_CHANNEL_MAX_REQUEST_SIZE

[Simon Horman <horms@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260711150628.2914205-1-michael.bommarito@gmail.com?part=1
---
--- Patch [1]: [PATCH] net: mana: cap HWC init max message size to HW_CHANNEL_MAX_REQUEST_SIZE ---
commit ea80048daa839e9f12f06618139e15fddebc0ac5
Author: Michael Bommarito <michael.bommarito@gmail.com>

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?