[PATCH v5 0/3] RDMA/srpt: fix the descriptor table checks and the context counters

[PATCH v5 0/3] RDMA/srpt: fix the descriptor table checks and the context counters

[Yehyeong Lee]

The first two patches bound initiator supplied descriptor data, and the
third widens the counters that hold what the first one admits.

A 510 entry table fits in the default 8276 byte IU when the additional CDB
is empty, so the bound accepts it and the counters then have to hold it.
With only the bound, a conforming command carrying 256 or more descriptors
still wraps n_rw_ctx and leaks every context it allocated. With only the
wider counters, a command that sets ADDITIONAL CDB LENGTH to its maximum
still walks a descriptor table that runs past the receive buffer.

The first fix dates from the driver's merge and the other two from the
conversion to the generic RDMA READ/WRITE API, which is why they are split.

Changes since v4, all in 1/3 and all from Bart's review.

The span check now uses struct_size(idb, desc_list, nbufs) rather than an
open-coded sizeof and multiplication, and the size_t casts are gone. nbufs
is at most 510 by the time the span is computed, because the existing count
check runs first, so struct_size() cannot saturate.

nbufs == 0 is refused explicitly. Bart and Sashiko both pointed out that a
table_desc.len below one descriptor still got through while the v4 commit
message claimed otherwise, so the check is there now and the message
describes what the code does.

2/3 is new, and it has not been reviewed yet. While working out why
nbufs == 0 reaches ib_post_send() I read the rest of srpt_alloc_rw_ctxs()
and found that a descriptor length of zero is not checked either. It
reaches target_alloc_sgl(), which has no length check of its own, and ends
in sg_init_marker() indexing a zero entry table at nents - 1, which is
unsigned, so the write lands far outside any allocation. DIRECT requests
are affected as well as INDIRECT tables, so the check sits in the shared
loop, and the patch carries its own Fixes tag.

3/3 is unchanged and carries Bart's Reviewed-by.

Link: https://lore.kernel.org/linux-rdma/20261024162852.3528224-1-yhlee@isslab.korea.ac.kr

Yehyeong Lee (3):
  RDMA/srpt: bound the indirect descriptor table to the IU
  RDMA/srpt: reject zero-length buffer descriptors
  RDMA/srpt: widen the indirect RDMA context counters

 drivers/infiniband/ulp/srpt/ib_srpt.c | 21 ++++++++++++++++++++-
 drivers/infiniband/ulp/srpt/ib_srpt.h |  4 ++--
 2 files changed, 22 insertions(+), 3 deletions(-)


base-commit: 6fba331225f5040e93d0947d1027e9712238291f
-- 
2.43.0

[PATCH v5 1/3] RDMA/srpt: bound the indirect descriptor table to the IU

[Yehyeong Lee]

srpt_get_desc_tbl() takes the number of INDIRECT descriptors from
idb->table_desc.len, which the initiator controls, and checks it only
against data_out_desc_cnt + data_in_desc_cnt, two more initiator fields.
Both are u8, so the pair admits up to 510 descriptors, and nothing
compares the table against the receive IU it has to live in.
srpt_alloc_rw_ctxs() then walks idb->desc_list for that many entries.

With the default srp_max_req_size of 8276 the table fits as long as the
additional CDB is short, but ADDITIONAL CDB LENGTH is an initiator field
too, and at its maximum of 252 bytes 510 descriptors end at
48 + 252 + 20 + 510 * 16 = 8480, which is past the end of the object.

  BUG: KASAN: slab-out-of-bounds in srpt_alloc_rw_ctxs+0x631/0x870
  Read of size 4 at addr ffff888002d6a05c by task kunit_try_catch/43

  CPU: 0 UID: 0 PID: 43 Comm: kunit_try_catch Tainted: G                 N   7.3.0-12378-g6fba331225f5-dirty #1 PREEMPT(lazy)
  Tainted: [N]=TEST
  Call Trace:
   <TASK>
   dump_stack_lvl+0x3f/0x50
   print_report+0xcd/0x620
   ? alloc_pages_node_noprof+0xa2/0x140
   ? srpt_alloc_rw_ctxs+0x631/0x870
   kasan_report+0xde/0x110
   ? srpt_alloc_rw_ctxs+0x631/0x870
   srpt_alloc_rw_ctxs+0x631/0x870
   srpt_test_parse+0x114/0x580
   ? __pfx_srpt_test_ctx_common.constprop.0+0x10/0x10
   ? __pfx_srpt_test_parse+0x10/0x10
   ? srpt_test_ctx_init.constprop.0+0x4cb/0x6c0
   srpt_test_run.constprop.0+0xc1/0xa60
   ? pick_task_fair+0x126/0x1950
   ? __pfx_srpt_test_run.constprop.0+0x10/0x10
   ? update_curr.constprop.0+0x4a/0x390
   ? __set_cpus_allowed_ptr+0x181/0x240
   ? __pfx_read_tsc+0x10/0x10
   ? ktime_get_ts64+0x192/0x250
   kunit_try_run_case+0x1ca/0x4b0
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? kthread_affine_node+0x24b/0x370
   ? __pfx_kthread_affine_node+0x10/0x10
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x36e/0x500
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x34d/0x620
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x22/0xdd0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x19/0x30
   </TASK>

  Allocated by task 43:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_slab_alloc+0x6e/0x70
   kmem_cache_alloc_noprof+0xfc/0x2b0
   srpt_test_ctx_common.constprop.0+0x46f/0x9b0
   srpt_test_ctx_init.constprop.0+0x30/0x6c0
   srpt_test_run.constprop.0+0xb6/0xa60
   kunit_try_run_case+0x1ca/0x4b0
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x36e/0x500
   ret_from_fork+0x34d/0x620
   ret_from_fork_asm+0x19/0x30

  The buggy address belongs to the object at ffff888002d68000
   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 [ffff888002d68000, ffff888002d6a054)

The KUnit harness that forces this is not part of this patch.

A table_desc.len below one descriptor is the other way in. It gives nbufs
zero, which the count check admits and a span check cannot catch either,
since an empty table fits in any IU. srpt_alloc_rw_ctxs() then returns
success with n_rw_ctx zero, no scatterlist is chained, and
srpt_write_pending() reaches ib_post_send() with an empty work request
list. rvt_post_send() evaluates !wr->next before the loop that tests wr
against NULL, so an rdmavt device faults there whenever its send queue is
idle.

Reject a table that does not fit by comparing the span it occupies
against byte_len, and reject an empty one. The span is the table offset
plus struct_size(idb, desc_list, nbufs), and nbufs is at most 510 once
the existing count check has passed, so it cannot saturate.

The header always lies inside the allocated buffer, since the largest
offset srpt_get_desc_buf() can return is 48 + 252 and the header is 20
bytes, so it ends at 320 while srp_max_req_size is at least
MIN_MAX_REQ_SIZE, which is 996. It does not always lie inside the bytes
the initiator actually sent, which is why the span starts at the table
offset and covers the header as well.

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>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad8e7e..40d796840befc 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1117,6 +1117,7 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 		struct srp_indirect_buf *idb = srpt_get_desc_buf(srp_cmd);
 		int nbufs = be32_to_cpu(idb->table_desc.len) /
 				sizeof(struct srp_direct_buf);
+		size_t desc_off = (uintptr_t)idb - (uintptr_t)srp_cmd;
 
 		if (nbufs >
 		    (srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt)) {
@@ -1128,6 +1129,18 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 			return -EINVAL;
 		}
 
+		if (nbufs == 0) {
+			pr_err("received SRP_CMD with an empty indirect descriptor table\n");
+			return -EINVAL;
+		}
+
+		if (desc_off + struct_size(idb, desc_list, nbufs) >
+		    recv_ioctx->byte_len) {
+			pr_err("received SRP_CMD indirect table (%d descriptors) that does not fit the %d-byte IU\n",
+			       nbufs, recv_ioctx->byte_len);
+			return -EINVAL;
+		}
+
 		*data_len = be32_to_cpu(idb->len);
 		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
 				sg, sg_cnt);
-- 
2.43.0

[PATCH v5 2/3] RDMA/srpt: reject zero-length buffer descriptors

[Yehyeong Lee]

srpt_alloc_rw_ctxs() takes each buffer descriptor length straight from the
wire and hands it to target_alloc_sgl(), which has no length check of its
own.

For the single descriptor of a DIRECT request, and for the last descriptor
of an INDIRECT table, chainable is false, so a zero length makes
sgl_alloc_order() compute nalloc zero. kmalloc_objs() answers that with
ZERO_SIZE_PTR, which is not NULL, so the allocation looks like it
succeeded and sg_init_table(sgl, 0) runs. That reaches sg_init_marker(),
which does sg_mark_end(&sgl[nents - 1]) with nents unsigned, so the index
is 4294967295 rather than minus one and the read modify write of page_link
lands far outside any allocation.

  Oops: general protection fault, probably for non-canonical address 0xdffffc03fffffffe: 0000 [#1] KASAN PTI
  KASAN: probably user-memory-access in range [0x0000001ffffffff0-0x0000001ffffffff7]
  CPU: 0 UID: 0 PID: 43 Comm: kunit_try_catch Tainted: G                 N   7.3.0-12378-g6fba331225f5-dirty #1 PREEMPT(lazy)
  Tainted: [N]=TEST
  RIP: 0010:sgl_alloc_order+0xb6/0x370
  Code: ef 01 4c 89 e2 49 89 c4 31 f6 49 c1 e7 05 48 89 c7 e8 ae e0 cb ff 4d 01 e7 48 b8 00 00 00 00 00 fc ff df 4c 89 fa 48 c1 ea 03 <80> 3c 02 00 0f 85 6b 02 00 00 49 bc 00 00 00 00 00 fc ff df 49 8b
  RSP: 0000:ffff888002d4fa38 EFLAGS: 00010202
  RAX: dffffc0000000000 RBX: 0000000000001000 RCX: 0000000000000000
  RDX: 00000003fffffffe RSI: 0000000000000000 RDI: 0000000000000010
  RBP: 0000000000000000 R08: ffff888002b158a0 R09: 0000000000000000
  R10: 0000000000000010 R11: 0000000000000037 R12: 0000000000000010
  R13: ffff888002d6803c R14: 0000000000000000 R15: 0000001ffffffff0
  FS:  0000000000000000(0000) GS:0000000000000000(0000) knlGS:0000000000000000
  CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
  CR2: ffff88802f1c8000 CR3: 000000002e458000 CR4: 00000000000006f0
  Call Trace:
   <TASK>
   target_alloc_sgl+0x2b/0x70
   srpt_alloc_rw_ctxs+0x2ef/0x870
   srpt_test_parse+0x114/0x580
   ? __pfx_srpt_test_parse+0x10/0x10
   ? srpt_test_ctx_init_direct+0x17d/0x280
   srpt_direct_len0_test+0xa4/0x280
   ? __pfx_srpt_direct_len0_test+0x10/0x10
   ? __set_cpus_allowed_ptr+0x181/0x240
   ? __pfx_read_tsc+0x10/0x10
   ? ktime_get_ts64+0x192/0x250
   kunit_try_run_case+0x1ca/0x4b0
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? kthread_affine_node+0x24b/0x370
   ? __pfx_kthread_affine_node+0x10/0x10
   ? __pfx_kunit_try_run_case+0x10/0x10
   ? __pfx_kunit_generic_run_threadfn_adapter+0x10/0x10
   kunit_generic_run_threadfn_adapter+0x7b/0xe0
   kthread+0x36e/0x500
   ? __pfx_kthread+0x10/0x10
   ret_from_fork+0x34d/0x620
   ? __pfx_ret_from_fork+0x10/0x10
   ? __switch_to+0x22/0xdd0
   ? __pfx_kthread+0x10/0x10
   ret_from_fork_asm+0x19/0x30
   </TASK>

The KUnit harness that forces this is not part of this patch.

A zero length descriptor elsewhere in a table is chainable, so one entry
is allocated and that write does not happen, but the entry count comes
back zero and the mapping in rdma_rw_ctx_init() then refuses it.

The target core never hands the same allocator a zero length. Its own call
in transport_generic_new_cmd() sits behind a cmd->data_length test.

Reject a descriptor whose length is zero. The existing unwind label covers
the first iteration, where its loop body does not run because nothing has
been allocated yet.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 40d796840befc..dfcecbe3a47c7 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -979,6 +979,12 @@ static int srpt_alloc_rw_ctxs(struct srpt_send_ioctx *ioctx,
 		u32 size = be32_to_cpu(db->len);
 		u32 rkey = be32_to_cpu(db->key);
 
+		if (size == 0) {
+			pr_err("received SRP_CMD buffer descriptor with zero length\n");
+			ret = -EINVAL;
+			goto unwind;
+		}
+
 		ret = target_alloc_sgl(&ctx->sg, &ctx->nents, size, false,
 				i < nbufs - 1);
 		if (ret)
-- 
2.43.0

[PATCH v5 3/3] RDMA/srpt: widen the indirect RDMA context counters

[Yehyeong Lee]

srpt_send_ioctx::n_rdma and ::n_rw_ctx are u8, and neither of the values
stored in them is bounded by 255.

n_rw_ctx counts the INDIRECT descriptors a command carries, and the count
guard in srpt_get_desc_tbl() admits data_out_desc_cnt plus
data_in_desc_cnt, so up to 510. At 256 descriptors n_rw_ctx wraps to
zero, and srpt_release_cmd() frees the contexts only when it is non-zero,
so every scatterlist and RDMA context the command allocated leaks.

n_rdma is not a descriptor count at all. It is the sum of the
rdma_rw_ctx_init() return values, which is the number of work requests
each context needs, accumulated by ioctx->n_rdma += ret. A handful of
descriptors for large transfers can pass 255 on their own, with no
unusual descriptor count involved. A wrapped n_rdma is then handed to
atomic_sub_return(ioctx->n_rdma, &ch->sq_wr_avail) and tested by
WARN_ON(ioctx->n_rdma <= 0), so the send queue work request accounting
drifts too.

Make both counters int, the type the values they accumulate already have.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Cc: stable@vger.kernel.org
Reviewed-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 2 +-
 drivers/infiniband/ulp/srpt/ib_srpt.h | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index dfcecbe3a47c7..a2af0882eff5a 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) {
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 506c11b843593..d9061aa65c1a8 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];
 };
-- 
2.43.0

Re: [PATCH v5 0/3] RDMA/srpt: fix the descriptor table checks and the context counters

[Bart Van Assche]

On 10/24/26 6:37 PM, Yehyeong Lee wrote:
> The first two patches bound initiator supplied descriptor data, and the
> third widens the counters that hold what the first one admits.
For the series:

Reviewed-by: Bart Van Assche <bvanassche@acm.org>