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

[Long Li <longli@microsoft.com>]

mana_hwc_init_event_handler() applied every HWC_INIT_DATA_MAX_NUM_CQS
event straight to gc->max_num_cqs.  That handler stays live for the whole
channel lifetime -- it also services runtime reconfig and link events --
so it is not confined to the initial bootstrap.

gc->cq_table is allocated once, sized to the max_num_cqs seen at bootstrap,
and every reader (mana_gd_process_eqe(), mana_create_rxq() and
mana_create_txq()) bounds-checks a CQ index against gc->max_num_cqs before
indexing gc->cq_table.  A device -- or a malicious host in a confidential
VM -- that sends a later HWC_INIT_DATA_MAX_NUM_CQS with a larger value
inflates the bound past the allocation.  This includes an event timed to
land while mana_hwc_establish_channel() is between reading the count and
publishing cq_table.  A subsequent out-of-range CQ id then passes the
bounds check and indexes cq_table out of bounds: an out-of-bounds read in
the EQ fast path, or an out-of-bounds pointer write in
mana_create_rxq()/mana_create_txq(), corrupting guest kernel memory.

Stop writing gc->max_num_cqs from the event handler.  Store the reported
value in hwc_init_max_num_cqs, and let mana_hwc_establish_channel() commit
it to gc->max_num_cqs once, from the same snapshot that sizes cq_table.
The handler store uses WRITE_ONCE() and the establish-time read uses
READ_ONCE(), since the two run concurrently (EQ interrupt vs process
context); the single, non-reloadable read is what guarantees the value
that sizes cq_table is the same one published as the bound, even across
the sleeping vcalloc().  gc->max_num_cqs then always matches the
allocation and no later event can change the bound after the table is
published, so the existing bounds checks are sufficient.

Fixes: ca9c54d2d6a5 ("net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)")
Signed-off-by: Long Li <longli@microsoft.com>
---
Changes in v4:
 - New patch in v4, split out of the v3 teardown-safety work in
   response to review: gc->max_num_cqs is set once when cq_table is
   allocated and never reset, so a spoofed post-init HWC event cannot
   inflate the bound past the allocation.

 .../net/ethernet/microsoft/mana/hw_channel.c  | 34 ++++++++++++++++---
 include/net/mana/hw_channel.h                 |  1 +
 2 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/microsoft/mana/hw_channel.c b/drivers/net/ethernet/microsoft/mana/hw_channel.c
index 2f0dae353955..03b5e2f02e35 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;
 
 		case HWC_INIT_DATA_PDID:
@@ -852,6 +860,8 @@ static int mana_hwc_establish_channel(struct gdma_context *gc, u16 *q_depth,
 	struct gdma_queue *eq = hwc->cq->gdma_eq;
 	struct gdma_queue *cq = hwc->cq->gdma_cq;
 	struct gdma_queue __rcu **cq_table;
+	u32 num_cqs;
+	u32 cq_id;
 	int err;
 
 	init_completion(&hwc->hwc_init_eqe_comp);
@@ -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;
+
+	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().
 	 */
diff --git a/include/net/mana/hw_channel.h b/include/net/mana/hw_channel.h
index 23bf83e2a3ec..c275150baf49 100644
--- a/include/net/mana/hw_channel.h
+++ b/include/net/mana/hw_channel.h
@@ -203,6 +203,7 @@ struct hw_channel_context {
 	u16 hwc_init_q_depth_max;
 	u32 hwc_init_max_req_msg_size;
 	u32 hwc_init_max_resp_msg_size;
+	u32 hwc_init_max_num_cqs;
 
 	struct completion hwc_init_eqe_comp;
 
-- 
2.43.0