[PATCH] RDMA/srpt: widen the RDMA context counters

[PATCH] RDMA/srpt: widen the RDMA context 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;
	}

srpt_alloc_rw_ctxs() then builds that many contexts but counts them in
ioctx->n_rw_ctx and ioctx->n_rdma, which are u8.  At 256 descriptors
n_rw_ctx wraps to zero, and srpt_release_cmd() frees the contexts only
when it is nonzero:

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

A command carrying 256 descriptors therefore leaks every scatterlist
target_alloc_sgl() allocated, every registration rdma_rw_ctx_init() set
up, and the rw_ctxs array as well, since the kfree() for it sits inside
srpt_free_rw_ctxs().  ioctx is a ring entry that the next command on the
same tag overwrites, so the array pointer is lost too.  The descriptor
payload sizes are the initiator's to choose and the command can be
repeated, so this grows without bound.  A count between 257 and 510 leaks
256 contexts and frees the remainder.

Widen both counters so they hold what the guard already admits, rather
than refusing counts the guard accepts.  int suits every existing user:
the two 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(), which take an int, and the three places that print it
use %d.  The WARN_ON(ioctx->n_rdma <= 0) in srpt_rdma_read_done() becomes
a real signed test instead of a test for zero.  The error unwind path
walks by index and restores both counters, so it was never affected.

There is no splat to quote, because the result is a leak rather than an
access past an object.  A KUnit harness that is not part of this patch
calls srpt_get_desc_tbl() with a fabricated SRP_CMD and counts the
rdma_rw_ctx_init() and rdma_rw_ctx_destroy() calls.  With u8 counters, 256
descriptors build 256 contexts and destroy none, and 510 build 510 and
destroy 254.  With int counters, 1, 128, 255, 256 and 510 descriptors all
build and destroy the same number.

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 | 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 7197d95f2216b..b174d58ca98a3 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 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] RDMA/srpt: widen the RDMA context counters

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] RDMA/srpt: out-of-bounds read in srpt_alloc_rw_ctxs() via unvalidated nbufs
--

commit 59a5707e41afe2c64f3f73bdc878e66abce7dc02
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

RDMA/srpt: widen the RDMA context counters

This commit widens the n_rdma and n_rw_ctx counters in struct
srpt_send_ioctx and srpt_alloc_rw_ctxs() from u8 to int so that SRP_CMD
requests carrying up to 510 indirect data buffer descriptors do not
wrap the counters and leak scatterlists and RDMA contexts.

> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 7197d95f2216b..b174d58ca98a3 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) {

[Severity: High]
By widening n_rw_ctx and n_rdma to accept up to 510 indirect descriptors
rather than rejecting counts that exceed the receive buffer, can
srpt_alloc_rw_ctxs() read past the end of recv_ioctx->ioctx.buf and
past recv_ioctx->byte_len when walking idb->desc_list?

In drivers/infiniband/ulp/srpt/ib_srpt.c, the call chain
srpt_recv_done() -> srpt_handle_new_iu() -> srpt_handle_cmd() ->
srpt_get_desc_tbl() locates the descriptor buffer using
srpt_get_desc_buf():

	static void *srpt_get_desc_buf(struct srp_cmd *srp_cmd)
	{
		return srp_cmd->add_data + (srp_cmd->add_cdb_len & ~3);
	}

In srpt_get_desc_tbl(), the SRP_DATA_DESC_INDIRECT branch bounds nbufs
only by srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt (up to
510), without verifying that idb and idb->desc_list[0 .. nbufs - 1] fit
within recv_ioctx->byte_len (the way the SRP_DATA_DESC_IMM branch does):

	} 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) /
				sizeof(struct srp_direct_buf);

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

		*data_len = be32_to_cpu(idb->len);
		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
				sg, sg_cnt);

In drivers/infiniband/ulp/srpt/ib_srpt.h, receive buffers are allocated
from req_buf_cache with size srp_max_req_size, which defaults to
DEFAULT_MAX_REQ_SIZE (8276 bytes, sized for 128 indirect descriptors)
and can be configured down to MIN_MAX_REQ_SIZE (996 bytes):

	MIN_MAX_REQ_SIZE = 996,
	DEFAULT_MAX_REQ_SIZE_1 = sizeof(struct srp_cmd)/*48*/ +
				 SRP_MAX_ADD_CDB_LEN +
				 sizeof(struct srp_indirect_buf)/*20*/ +
				 128 * sizeof(struct srp_direct_buf)/*16*/,

When an initiator sends an SRP_CMD with nbufs up to 510, the loop in
srpt_alloc_rw_ctxs() reads up to:

	sizeof(struct srp_cmd) (48) +
	(srp_cmd->add_cdb_len & ~3) (up to 252) +
	sizeof(struct srp_indirect_buf) (20) +
	510 * sizeof(struct srp_direct_buf) (8160) = 8480 bytes

from srp_cmd, reading past both recv_ioctx->byte_len and the end of the
req_buf_cache slab object whenever add_cdb_len >= 52 (at the default
8276-byte srp_max_req_size) or whenever nbufs exceeds what fits in the
received IU.

Should srpt_get_desc_tbl() validate that the descriptor header and all
nbufs entries in idb->desc_list lie within recv_ioctx->byte_len before
calling srpt_alloc_rw_ctxs()?

[ ... ]

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

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

[Yehyeong Lee]

Thanks for the review.

You are right: the indirect branch of srpt_get_desc_tbl() bounds nbufs only
against data_out_desc_cnt + data_in_desc_cnt, not against
recv_ioctx->byte_len, so a large table_desc.len or add_cdb_len lets
srpt_alloc_rw_ctxs() walk idb->desc_list past the received IU and past the
req_buf_cache object.

v2, which I am sending now, bounds the indirect descriptor table against
recv_ioctx->byte_len the way the immediate-data branch already does, in
addition to widening the counters. Thanks for catching it.

Yehyeong Lee