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

[sashiko-bot@kernel.org]

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