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

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

Pre-existing issues:
- [High] A Time-of-Check to Time-of-Use (TOCTOU) race condition in `mana_gd_publish_cq()` and `mana_gd_unpublish_cq()` allows a malicious device to cause out-of-bounds array reads and writes, bypassing the patch's protection.
- [High] The untouched `WARN_ON(err)` in `mana_hwc_establish_channel()` still allows a maliciously crafted concurrent device response to trigger a kernel panic, defeating the purpose of the patch.
- [High] Multiple other `WARN_ON_ONCE` statements evaluating device-controlled inputs remain in the code, leaving the guest vulnerable to a device-induced panic.
--

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

net: mana: keep max_num_cqs immutable once cq_table is allocated

This commit addresses an issue where an untrusted device event can update the
max_num_cqs bound after cq_table allocation, leading to out-of-bounds
accesses. It fixes this by snapshotting the bound in hwc_init_max_num_cqs and
reading it once in mana_hwc_establish_channel(). It also replaces a WARN_ON
with an error return so that a malicious device cannot panic a panic_on_warn
guest.

> diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
> index b1269f7da0563..d9bff4634dc35 100644
> --- a/drivers/net/ethernet/microsoft/mana/hw_channel.c
> +++ b/drivers/net/ethernet/microsoft/mana/hw_channel.c

[ ... ]

> @@ -209,7 +209,11 @@ 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 only; establish_channel() commits it to
> +			 * max_num_cqs once, so a later event cannot grow the
> +			 * bound past the allocation.  Pairs with its READ_ONCE().
> +			 */
> +			WRITE_ONCE(hwc->hwc_init_max_num_cqs, val);
>  			break;
>  

[Severity: High]
This is a pre-existing issue, but does this vulnerability still exist for the
CQ ID?

In mana_hwc_init_event_handler(), the untrusted device event can concurrently
overwrite the queue ID without synchronization:

		case HWC_INIT_DATA_CQID:
			hwc->cq->gdma_cq->id = val;
			break;

Meanwhile, mana_gd_publish_cq() reads queue->id multiple times, first to
validate the bound and then to index the array:

	if (!cq_table || queue->id >= gc->max_num_cqs)
		return -EINVAL;

	/* Sharing a CQ between WQs is not supported. */
	if (rcu_access_pointer(cq_table[queue->id]))
		return -EINVAL;
	...
	rcu_assign_pointer(cq_table[queue->id], queue);

If the device sends a malformed CQID event that arrives after the bounds
check but before the array indexing, couldn't this force an out-of-bounds
array write of the queue pointer? The same Time-of-Check to Time-of-Use race
condition appears to exist in mana_gd_unpublish_cq().

[ ... ]

> @@ -810,17 +816,32 @@ 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 count and id once, so the same value
> +	 * sizes, bounds and indexes cq_table even across the sleeping
> +	 * vcalloc() and a concurrent init event.
> +	 */
> +	num_cqs = READ_ONCE(hwc->hwc_init_max_num_cqs);
> +	cq_id = READ_ONCE(cq->id);
> +
> +	/* Both operands come from untrusted HWC bootstrap events; a missing
> +	 * MAX_NUM_CQS leaves num_cqs at 0.  Reject rather than WARN_ON() so a
> +	 * malformed device response cannot panic a panic_on_warn guest.
> +	 */
> +	if (cq_id >= num_cqs) {
> +		dev_err_ratelimited(hwc->dev,
> +				    "HWC: bad CQ id %u >= max %u\n",
> +				    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;
>  
> -	/* Publish the initialised table; pairs with smp_load_acquire()
> -	 * in mana_gd_get_cq().
> +	/* Publish the bound and the initialised table together; the release
> +	 * pairs with smp_load_acquire() in mana_gd_get_cq().
>  	 */
> +	gc->max_num_cqs = num_cqs;
>  	smp_store_release(&gc->cq_table, cq_table);
>  
>  	/* Publish the HWC CQ now that the table is in place. */

[Severity: High]
This is a pre-existing issue, but does a maliciously crafted concurrent device
response still have the ability to trigger a kernel panic here?

Just below this context in mana_hwc_establish_channel(), there is an
untouched WARN_ON():

	err = mana_gd_publish_cq(gc, cq);
	if (WARN_ON(err))
		return err;

If a HWC_INIT_DATA_CQID event is processed after the READ_ONCE(cq->id)
snapshot earlier in this function, mana_gd_publish_cq() will correctly reject
the modified ID and return -EINVAL. However, won't that returned error
trigger this WARN_ON(), crashing a panic_on_warn guest anyway?

[Severity: High]
This is a pre-existing issue, but are there other WARN_ON_ONCE() statements
evaluating device-controlled inputs that could lead to a device-induced panic?

For example, in mana_hwc_rx_event_handler():

	if (WARN_ON_ONCE(hwc_rxq->gdma_wq->id != gdma_rxq_id))
		return;

In mana_hwc_tx_event_handler():

	WARN_ON_ONCE(!hwc_txq || hwc_txq->gdma_wq->id != gdma_txq_id);

And in mana_gd_read_cqe():

	new_bits = (cq->head / num_cqe) & GDMA_CQE_OWNER_MASK;
	/* Return -1 if overflow detected. */
	if (WARN_ON_ONCE(owner_bits != new_bits))
		return -1;

Since these validate untrusted values from the DMA completion ring, could a
malformed response trigger these warnings and panic a guest?

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