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

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

Pre-existing issues:
- [High] Missing reader-side acquire barrier and missing reset of `gc->max_num_cqs` on teardown allows a malicious host to bypass the `cq_table` bounds check on resume, leading to an out-of-bounds read and potential control flow hijack.
- [High] A spoofed `GDMA_EQE_HWC_INIT_EQ_ID_DB` event overwrites the hardware channel doorbell ID without bounds checking, leading to a guest kernel panic.
--

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

net: mana: keep max_num_cqs immutable once cq_table is allocated

This commit prevents post-init HWC events from inflating the CQ count
bound past the cq_table allocation size. It stores the reported max_num_cqs 
in the channel context and commits it once during channel establishment. 
This ensures the allocation size and the bounds check remain consistent.

> 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:

[Severity: High]
This is a pre-existing issue, but can a spoofed GDMA_EQE_HWC_INIT_EQ_ID_DB
event cause an out-of-bounds MMIO write?

Looking at mana_hwc_init_event_handler(), it blindly trusts the doorbell ID
provided by the host:

    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;

The next time the driver rings the doorbell in mana_gd_ring_doorbell(), it
uses this unvalidated spoofed ID to calculate the MMIO address:

    void __iomem *addr = gc->db_page_base + gc->db_page_size * db_index;
    ...
    writeq(e.as_uint64, addr);

Could this result in writing outside the mapped bar0_va region and cause a
guest kernel panic?

> -			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;

[ ... ]

> @@ -881,15 +891,29 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
>  	*max_req_msg_size = hwc->hwc_init_max_req_msg_size;
>  	*max_resp_msg_size = hwc->hwc_init_max_resp_msg_size;
>  
> -	/* Both were set in mana_hwc_init_event_handler(). */
> -	if (WARN_ON(cq->id >= gc->max_num_cqs))
> +	/* Snapshot the device-reported CQ count and CQ id into locals and
> +	 * use only the locals below, so the same value that sizes cq_table
> +	 * also bounds and indexes it -- even across the sleeping vcalloc().
> +	 * Both fields are written by mana_hwc_init_event_handler() from EQ
> +	 * interrupt context: hwc_init_max_num_cqs under WRITE_ONCE() (paired
> +	 * here), and cq->id as an ordinary store.  READ_ONCE() keeps each
> +	 * read tear-free and, crucially, non-reloadable, so a spoofed
> +	 * post-init event cannot make the WARN_ON() pass against one value
> +	 * while the allocation or the index uses another.
> +	 */
> +	num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
> +	cq_id = READ_ONCE(cq->id);
> +
> +	if (WARN_ON(cq_id >= num_cqs))
>  		return -EPROTO;
>  
> -	cq_table = vcalloc(gc->max_num_cqs, sizeof(*cq_table));
> +	cq_table = vcalloc(num_cqs, sizeof(*cq_table));
>  	if (!cq_table)
>  		return -ENOMEM;
>  
> -	rcu_assign_pointer(cq_table[cq->id], cq);
> +	gc->max_num_cqs = num_cqs;
> +

[Severity: High]
This is a pre-existing issue, but does teardown leave gc->max_num_cqs at a 
stale value?

Looking at mana_hwc_destroy_channel() in
drivers/net/ethernet/microsoft/mana/hw_channel.c, it replaces the
cq_table pointer but doesn't reset gc->max_num_cqs:

    old_cq_table = rcu_replace_pointer(gc->cq_table, NULL, true);
    synchronize_rcu();

During a device resume, mana_hwc_establish_channel() will publish a new,
potentially smaller cq_table and update gc->max_num_cqs. 

If a malicious host sends a GDMA_EQE_COMPLETION event concurrently, the
reader in mana_gd_process_eqe() in
drivers/net/ethernet/microsoft/mana/gdma_main.c checks the bounds:

    cq_table = rcu_dereference(gc->cq_table);
    if (WARN_ON_ONCE(cq_id >= gc->max_num_cqs || !cq_table))
        break;

Since there is no acquire barrier between reading gc->cq_table and
gc->max_num_cqs, could weakly ordered CPUs read the stale, large
max_num_cqs bound and the new, smaller cq_table pointer?

This seems like it could allow a crafted cq_id to pass the bounds
check and cause an out-of-bounds memory read.

> +	rcu_assign_pointer(cq_table[cq_id], cq);
>  	/* Publish the fully-initialised table last; pairs with the
>  	 * rcu_dereference(gc->cq_table) in mana_gd_process_eqe().
>  	 */

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