Re: [PATCH net v5 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated

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

Pre-existing issues:
- [Critical] A spoofed GDMA_EQE_HWC_INIT_EQ_ID_DB event changes gd->doorbell without bounds checking, leading to an out-of-bounds MMIO write.
--

--- Patch [7]: [PATCH net v5 7/7] net: mana: keep max_num_cqs immutable once cq_table is allocated ---
commit e3524562bdca9a596ad21cc819f86df8b083a5c4
Author: Long Li <[email protected]>
Subject: net: mana: keep max_num_cqs immutable once cq_table is allocated

This commit prevents a spoofed HWC_INIT_DATA_MAX_NUM_CQS event from bypassing
allocation bounds checks. It modifies the initialization event handler to store
the reported value rather than applying it directly, committing it only once
when sizing the table.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index 2f0dae3539552..03b5e2f02e352 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> @@ -246,7 +246,15 @@ static void mana_hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
>  			break;
>  
>  		case HWC_INIT_DATA_MAX_NUM_CQS:
> -			gd->gdma_context->max_num_cqs = val;
> +			/* Store, don't apply: mana_hwc_establish_channel()
> +			 * commits this to gc->max_num_cqs once, together
> +			 * with sizing cq_table, so a spoofed post-init event
> +			 * cannot inflate the bound past the allocation.
> +			 * WRITE_ONCE() pairs with the READ_ONCE() there:
> +			 * this store runs in EQ interrupt context,
> +			 * concurrently with that process-context read.
> +			 */
> +			WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
>  			break;

[Severity: Critical]
This is a pre-existing issue, but while this patch protects against spoofed
CQ counts from untrusted environments, could a similar out-of-bounds memory
access happen with the doorbell index?

Looking earlier in mana_hwc_init_event_handler(), a spoofed doorbell index
appears to be accepted blindly:

mana_hwc_init_event_handler() {
    ...
    switch (event->type) {
    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;
        break;
    ...
}

If a malicious host sends a GDMA_EQE_HWC_INIT_EQ_ID_DB event with an
arbitrarily large doorbell index, it is updated without bounds validation
against gc->bar0_size.

Would this lead to an out-of-bounds MMIO write when later ringing the
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);
}

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=7