[PATCH v4 0/2] RDMA/srpt: fix the indirect descriptor table bound and the context counters

[PATCH v4 0/2] RDMA/srpt: fix the indirect descriptor table bound and the context counters

[Yehyeong Lee]

Both changes are needed, and they fix different things.

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 two defects also come from different commits, which is why they are
split here.

Changes since v3, both from the review of 1/2.

desc_off is a uintptr_t subtraction now, as Bart asked.

The room check is gone. It divided the bytes left after the header by the
descriptor size, and the division truncates toward zero, so a byte_len one
to fifteen bytes short of the header gave room zero rather than negative.
A table_desc.len below one descriptor gives nbufs zero at the same time, so
nbufs > room was false and a truncated request was accepted. That is
Sashiko's [Medium], and the v3 commit message claimed the opposite. v4
compares the span the table occupies against byte_len directly, which needs
no division and no sign, and the wrong paragraph is gone.

2/2 is unchanged. Bart asked why int rather than an unsigned type. The
values are counts and never negative, so unsigned fits, but
rdma_rw_ctx_init() returns int and the WARN_ON(ioctx->n_rdma <= 0) in
srpt_rdma_read_done() is a signed test, so int leaves the surrounding code
alone. I am happy to make both unsigned int and turn that test into == 0
instead.

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

Yehyeong Lee (2):
  RDMA/srpt: bound the indirect descriptor table to the IU
  RDMA/srpt: widen the indirect RDMA context counters

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


base-commit: 5b0764a53235d3c77e1ed674e836e1b3fe1a4b1f
-- 
2.43.0

[PATCH v4 1/2] 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+0x660/0x860
  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-11999-g5b0764a53235-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+0x660/0x860
   kasan_report+0xde/0x110
   ? srpt_alloc_rw_ctxs+0x660/0x860
   srpt_alloc_rw_ctxs+0x660/0x860
   ? __dequeue_entity+0xb12/0x1ba0
   srpt_test_run.constprop.0+0x135/0xc50
   ? __pfx_srpt_test_run.constprop.0+0x10/0x10
   ? pick_task_fair+0x126/0x1950
   ? 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_init.constprop.0+0x481/0xfa0
   srpt_test_run.constprop.0+0xf1/0xc50
   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.

Compare the span the table occupies, which is the table offset plus the
descriptor header plus nbufs descriptors, against byte_len directly. A
short IU that cannot hold even the header is refused, and so is a
table_desc.len below one descriptor, with no division and no dependence on
a sign. nbufs is at most 510 once the existing count check has passed, so
the multiplication cannot overflow.

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 | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad8e7e..21264264356a2 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,14 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 			return -EINVAL;
 		}
 
+		if (desc_off + sizeof(*idb) +
+		    (size_t)nbufs * sizeof(struct srp_direct_buf) >
+		    (size_t)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

Re: [PATCH v4 1/2] RDMA/srpt: bound the indirect descriptor table to the IU

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/srpt: unrejected zero-descriptor indirect table in srpt_get_desc_tbl()
--

commit 8189d07cfa54a6ae2404e7715f0d334dafe9c38a
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

RDMA/srpt: bound the indirect descriptor table to the IU

This commit bounds the indirect descriptor table in srpt_get_desc_tbl()
against the received IU length (recv_ioctx->byte_len) to prevent an
out-of-bounds read in srpt_alloc_rw_ctxs().

> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 43c3ff1ad8e7e..21264264356a2 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,14 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
>  			return -EINVAL;
>  		}
>  
> +		if (desc_off + sizeof(*idb) +
> +		    (size_t)nbufs * sizeof(struct srp_direct_buf) >
> +		    (size_t)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;
> +		}

[Severity: High]
Does this check actually reject an indirect descriptor table when
idb->table_desc.len is smaller than sizeof(struct srp_direct_buf), as
described in the commit message ("and so is a table_desc.len below one
descriptor")?

In include/scsi/srp.h, struct srp_indirect_buf defines desc_list[] as a
flexible array member, so sizeof(*idb) is 20 bytes (only the header). When an
initiator sends an SRP_CMD with SRP_DATA_DESC_INDIRECT and
be32_to_cpu(idb->table_desc.len) < sizeof(struct srp_direct_buf), integer
division in srpt_get_desc_tbl() sets nbufs to 0.

If recv_ioctx->byte_len is at least desc_off + sizeof(*idb) (for example, 68
bytes when add_cdb_len is 0), both the existing descriptor count check and the
new bounds check pass with nbufs == 0, and srpt_get_desc_tbl() calls
srpt_alloc_rw_ctxs() with nbufs == 0:

drivers/infiniband/ulp/srpt/ib_srpt.c:srpt_alloc_rw_ctxs() {
	if (nbufs == 1) {
		ioctx->rw_ctxs = &ioctx->s_rw_ctx;
	} else {
		ioctx->rw_ctxs = kmalloc_objs(*ioctx->rw_ctxs, nbufs);
		if (!ioctx->rw_ctxs)
			return -ENOMEM;
	}
	...
	for (i = ioctx->n_rw_ctx; i < nbufs; i++, db++) {
		...
	}
	return 0;
}

Because kmalloc_objs() with a count of 0 returns ZERO_SIZE_PTR (non-NULL),
srpt_alloc_rw_ctxs() executes zero loop iterations and returns 0 with
ioctx->n_rw_ctx == 0 and *sg_cnt == 0.

For a write command (DMA_TO_DEVICE) with idb->len > 0, target_submit_prep()
then allocates target scatterlists and transport_generic_new_cmd() invokes
srpt_write_pending():

drivers/infiniband/ulp/srpt/ib_srpt.c:srpt_write_pending() {
	...
	cqe->done = srpt_rdma_read_done;
	for (i = ioctx->n_rw_ctx - 1; i >= 0; i--) {
		struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];

		first_wr = rdma_rw_ctx_wrs(&ctx->rw, ch->qp, ch->sport->port,
				cqe, first_wr);
		cqe = NULL;
	}

	ret = ib_post_send(ch->qp, first_wr, NULL);
}

With ioctx->n_rw_ctx == 0, the loop does not run and ib_post_send() is called
with first_wr == NULL. Depending on the RDMA provider, this either triggers a
NULL pointer dereference on wr (for example, in rvt_post_send() at !wr->next)
or returns 0 without posting ioctx->rdma_cqe, leaving the command stuck in
SRPT_STATE_NEED_DATA and hanging session teardown in
target_wait_for_sess_cmds().

Should srpt_get_desc_tbl() also reject !nbufs (or table_desc.len <
sizeof(struct srp_direct_buf))?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024162852.3528224-1-yhlee@isslab.korea.ac.kr?part=1

Re: [PATCH v4 1/2] RDMA/srpt: bound the indirect descriptor table to the IU

[Bart Van Assche]

On 10/24/26 10:06 AM, sashiko-bot@kernel.org wrote:
>> +		if (desc_off + sizeof(*idb) +
>> +		    (size_t)nbufs * sizeof(struct srp_direct_buf) >
>> +		    (size_t)recv_ioctx->byte_len) {

Are the size_t casts really necessary? Please remove these typecasts if
these are not necessary since in C code typecasts reduce the ability of
the compiler to perform type checking.

>> +			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;
>> +		}
> 
> [Severity: High]
> Does this check actually reject an indirect descriptor table when
> idb->table_desc.len is smaller than sizeof(struct srp_direct_buf), as
> described in the commit message ("and so is a table_desc.len below one
> descriptor")?

I think that Sashiko is right that a table_desc.len below sizeof(struct 
srp_direct_buf) still gets through: nbufs becomes 0, the span check 
passes, and srpt_alloc_rw_ctxs() returns success with n_rw_ctx == 0, 
which makes srpt_write_pending() call ib_post_send() with a NULL WR 
list. Please reject nbufs == 0 too, so the code does what the commit 
message says. Also, please consider struct_size(idb, desc_list, nbufs) 
instead of open-coding sizeof(*idb) + nbufs * sizeof(struct srp_direct_buf).

Thanks,

Bart.

Re: [PATCH v4 1/2] RDMA/srpt: bound the indirect descriptor table to the IU

[Yehyeong Lee]

Thanks. v5 removes the size_t casts, uses struct_size(idb, desc_list, nbufs),
and rejects nbufs == 0, so the code does what the commit message says.

v5 is now three patches. While working out why nbufs == 0 reached
ib_post_send() I found that a zero-length descriptor is not rejected either
and faults the same allocator, so I added that as a new 2/3 that still needs
review. The counters patch is 3/3 and carries your Reviewed-by. Both are
described in the cover letter.

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

Thanks,
Yehyeong

[PATCH v4 2/2] 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
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 21264264356a2..17d77818c2bc3 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 v4 2/2] RDMA/srpt: widen the indirect RDMA context counters

[sashiko-bot]

> 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
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261024162852.3528224-1-yhlee@isslab.korea.ac.kr?part=2

Re: [PATCH v4 2/2] RDMA/srpt: widen the indirect RDMA context counters

[Bart Van Assche]

On 10/24/26 9:28 AM, Yehyeong Lee wrote:
> Make both counters int, the type the values they accumulate already have.
Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH v4 2/2] RDMA/srpt: widen the indirect RDMA context counters

[Yehyeong Lee]

Thanks for the review. I carried your Reviewed-by into 3/3 of v5.

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

Thanks,
Yehyeong