[PATCH v2] RDMA/srpt: bound the indirect descriptor table and widen the counters

[PATCH v2] RDMA/srpt: bound the indirect descriptor table and widen the counters

[Yehyeong Lee]

srpt_get_desc_tbl() bounds the number of indirect data buffer descriptors
by the two counts the initiator put in the SRP_CMD, and both are u8, so it
admits an nbufs of up to 510:

	if (nbufs >
	    (srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt)) {
		...
		return -EINVAL;
	}

Nothing checks that the header and those nbufs entries are inside the
information unit that was received.  srpt_alloc_rw_ctxs() walks
idb->desc_list for nbufs entries ending at

	sizeof(struct srp_cmd) + (srp_cmd->add_cdb_len & ~3) +
	sizeof(struct srp_indirect_buf) +
	nbufs * sizeof(struct srp_direct_buf)

which is 8480 bytes for an add_cdb_len of 252 and an nbufs of 510, past
recv_ioctx->byte_len and, at the default srp_max_req_size of 8276, past
the req_buf_cache object.  An add_cdb_len of 52 already suffices.  The
immediate data branch of this same function bounds its length against
srp_max_req_size and against recv_ioctx->byte_len, so bring the indirect
branch in line.

The count also outruns the counters that track the contexts, which are u8,
so at 256 descriptors n_rw_ctx wraps to zero and srpt_release_cmd() skips
the free:

	if (ioctx->n_rw_ctx) {
		srpt_free_rw_ctxs(ch, ioctx);
		ioctx->n_rw_ctx = 0;
	}

Such a command leaks every scatterlist target_alloc_sgl() allocated, every
registration rdma_rw_ctx_init() set up, and the rw_ctxs array, whose
kfree() sits inside srpt_free_rw_ctxs().  ioctx is a ring entry the next
command on the same tag overwrites, so the array pointer is lost too.  A
count between 257 and 510 leaks 256 contexts and frees the remainder.

Both changes are needed: a 510 entry table fits a default 8276 byte IU
when add_cdb_len is zero, so the new bound accepts it and the counters
still have to hold it, and widening the counters does nothing about a
table reaching past the IU.

Check the header against byte_len before reading the table length, and the
table after the existing count guard has bounded nbufs at 510.  The bound
is the number of descriptors the remaining bytes hold, so there is no
multiplication to overflow and a negative byte_len is refused.  Widen both
counters to int, which suits every user: the loops that walk the contexts
backwards use an int index and stop at -1, the sq_wr_avail accounting
passes the value to atomic_add() and atomic_sub_return(), and the three
places that print it use %d.

The KUnit harness below is not part of this patch.  It calls
srpt_get_desc_tbl() with a fabricated SRP_CMD in a buffer from a
kmem_cache shaped like req_buf_cache, and stubs only rdma_rw_ctx_init()
and rdma_rw_ctx_destroy(), the two helpers that need a live QP.  An
add_cdb_len of 252 with an nbufs of 510 gives

  BUG: KASAN: slab-out-of-bounds in srpt_alloc_rw_ctxs+0x6ee/0x950
  Read of size 4 at addr 000000006318205c by task kunit_try_catch/33

  CPU: 0 UID: 0 PID: 33 Comm: kunit_try_catch Tainted: G                 N  7.3.0-dirty #7 VOLUNTARY
  Tainted: [N]=TEST
  Call Trace:
   [<600a43cf>] ? dump_stack_lvl+0x5e/0x7a
   [<600450d2>] ? _printk+0x0/0xd7
   [<60051cce>] ? print_report+0x14b/0x517
   [<6039355e>] ? alloc_pages_node_noprof+0xae/0x120
   [<600450d2>] ? _printk+0x0/0xd7
   [<60a5d70e>] ? srpt_alloc_rw_ctxs+0x6ee/0x950
   [<603c78eb>] ? kasan_report+0x11b/0x160
   [<60a5d70e>] ? srpt_alloc_rw_ctxs+0x6ee/0x950
   [<60a5d70e>] ? srpt_alloc_rw_ctxs+0x6ee/0x950
   [<6008fdc9>] ? srpt_test_run.constprop.0.cold+0x177/0xd8f
   [<60a5fd30>] ? srpt_test_run.constprop.0+0x0/0xea0
    [ ... ]
   [<6072718c>] ? kunit_try_run_case+0x1bc/0x310
   [<60726fd0>] ? kunit_try_run_case+0x0/0x310
   [<6072d250>] ? kunit_generic_run_threadfn_adapter+0x0/0x100
   [<6072d2cc>] ? kunit_generic_run_threadfn_adapter+0x7c/0x100
   [<6014750e>] ? kthread+0x36e/0x560
   [<601471a0>] ? kthread+0x0/0x560
   [<600a90de>] ? new_thread_handler+0xce/0x130
  The buggy address belongs to the object at 0000000063180000
   which belongs to the cache srpt-kunit-iu of size 8276
  The buggy address is located 8 bytes to the right of
   allocated 8276-byte region [0000000063180000, 0000000063182054)

  Memory state around the buggy address:
   0000000063181f00: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
   0000000063181f80: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  >0000000063182000: 00 00 00 00 00 00 00 00 00 00 04 fc fc fc fc fc
                                                      ^
   0000000063182080: fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc fc

With 256 descriptors inside the IU there is no splat, because that case is
a leak rather than an access past an object: the harness counts 256
rdma_rw_ctx_init() calls and no rdma_rw_ctx_destroy() call.  After this
patch both shapes are refused, and 1, 128, 255, 256 and 510 descriptors
that fit the IU are accepted with every context freed again.

Fixes: a42d985bd5b2 ("ib_srpt: Initial SRP Target merge for v3.3-rc1")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
v2: also bound the indirect descriptor walk against recv_ioctx->byte_len,
as raised in review; widen the counters as in v1.
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 21 +++++++++++++++++++--
 drivers/infiniband/ulp/srpt/ib_srpt.h |  4 ++--
 2 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 7197d95f2216b..5a12581286194 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -959,7 +959,7 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 	struct srpt_rdma_ch *ch = ioctx->ch;
 	struct scatterlist *prev = NULL;
 	unsigned prev_nents;
-	u8 n_rdma, n_rw_ctx;
+	int n_rdma, n_rw_ctx;
 	int ret, i;
 
 	if (nbufs == 1) {
@@ -1115,7 +1115,16 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 	} else if (((srp_cmd->buf_fmt & 0xf) == SRP_DATA_DESC_INDIRECT) ||
 		   ((srp_cmd->buf_fmt >> 4) == SRP_DATA_DESC_INDIRECT)) {
 		struct srp_indirect_buf *idb = srpt_get_desc_buf(srp_cmd);
-		int nbufs = be32_to_cpu(idb->table_desc.len) /
+		int desc_off = (u8 *)idb - (u8 *)srp_cmd;
+		int nbufs, room;
+
+		if (desc_off > recv_ioctx->byte_len - (int)sizeof(*idb)) {
+			pr_err("received SRP_CMD with a truncated indirect descriptor header (%d + %zu > %d)\n",
+			       desc_off, sizeof(*idb), recv_ioctx->byte_len);
+			return -EINVAL;
+		}
+
+		nbufs = be32_to_cpu(idb->table_desc.len) /
 				sizeof(struct srp_direct_buf);
 
 		if (nbufs >
@@ -1128,6 +1137,14 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 			return -EINVAL;
 		}
 
+		room = (recv_ioctx->byte_len - desc_off - (int)sizeof(*idb)) /
+			(int)sizeof(struct srp_direct_buf);
+		if (nbufs > room) {
+			pr_err("received SRP_CMD with %d descriptors but room for %d in the IU\n",
+			       nbufs, room);
+			return -EINVAL;
+		}
+
 		*data_len = be32_to_cpu(idb->len);
 		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
 				sg, sg_cnt);
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 4c46b301eea18..1dce9bcb490f0 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -223,8 +223,8 @@ struct srpt_send_ioctx {
 	struct ib_cqe		rdma_cqe;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;
-	u8			n_rdma;
-	u8			n_rw_ctx;
+	int			n_rdma;
+	int			n_rw_ctx;
 	bool			queue_status_only;
 	u8			sense_data[TRANSPORT_SENSE_BUFFER];
 };

base-commit: c03c1a14507dfaf8b856c2608f9ac216021fc222
-- 
2.43.0

Re: [PATCH v2] RDMA/srpt: bound the indirect descriptor table and widen the counters

[Bart Van Assche]

On 10/20/26 5:18 PM, Yehyeong Lee wrote:
>    BUG: KASAN: slab-out-of-bounds in srpt_alloc_rw_ctxs+0x6ee/0x950
>    Read of size 4 at addr 000000006318205c by task kunit_try_catch/33

Some feedback about this patch:
* Since this patch fixes two separate bugs, should it perhaps be split
   into two patches? The fixed issues are:
    - The missing bound on the descriptor table goes back to
      commit a42d985bd5b2. The original code did `memcpy(ioctx->rbufs,
      db, n_rbuf * sizeof *db)` with the same missing check.
    - The `u8` `n_rw_ctx`/`n_rdma` counters came in with commit
      b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE
      API").
* n_rdma can overflow without many descriptors. It is the sum of
   rdma_rw_ctx_init() return values (work requests per context), and that
   can exceed 255 even for a few large descriptors. Widening it fixes
   more than the commit message says; please mention that.
* The `idb` header ends at most at offset 320, and `MIN_MAX_REQ_SIZE` is
   996, so reading it is never out of bounds; at worst it reads stale
   data. The extra check is fine, but both checks could be folded into
   one, placed after the existing descriptor-count check.

Thanks,

Bart.