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 --- 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