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
