[PATCH 0/2] RDMA/srp: bound the receive IU the target asks for

[PATCH 0/2] RDMA/srp: bound the receive IU the target asks for

[Yehyeong Lee]

The target decides how large the initiator's receive buffers are, and the
initiator never checks the answer.

srp_cm_rep_handler() takes max_ti_iu_len out of the SRP_LOGIN_RSP and
srp_alloc_iu_bufs() sizes every receive IU with it. Across the whole file
that value is only ever assigned, handed to kzalloc() and passed to two
dma_sync calls. It is never compared against anything. Meanwhile the
initiator parses a 36 byte header out of those buffers, and
srp_process_rsp() does so without looking at how many bytes arrived, unlike
srp_process_cred_req() and srp_process_aer_req() which both refuse a short
message.

The two patches close different halves of that, so they are separate.

The first refuses a login that cannot work. A report below 36 gives
undersized buffers, and a report of zero is worse, because kzalloc()
answers it with ZERO_SIZE_PTR and on a virtually mapped transport
ib_dma_map_single() returns that pointer without looking at the length,
while ib_dma_mapping_error() calls it a success. The buffer then reaches
the receive path and the read of the opcode in srp_recv_done() is already
out of bounds.

The second refuses a truncated response, which is also what makes the
initiator stop parsing whatever an earlier response left in the reused IU.

Both have been exercised against an out of tree KUnit test on x86_64 with
KASAN, one boot per case. On the unpatched arm a target reporting 33 gets
33 byte IUs and srp_process_rsp() reads four bytes at offset 32:

  BUG: KASAN: slab-out-of-bounds in srp_process_rsp+0x757/0xbe0
  Read of size 4 at addr ffff88800243f120 by task kunit_try_catch/42
   srp_process_rsp+0x757/0xbe0
  Allocated by task 42:
   srp_alloc_iu.constprop.0+0x69/0x5c0
   srp_cm_rep_handler+0xc2e/0x1bb0
  The buggy address belongs to the object at ffff88800243f100
   which belongs to the cache kmalloc-64 of size 64
  The buggy address is located 32 bytes inside of
   allocated 33-byte region [ffff88800243f100, ffff88800243f121)

A report of zero logs in on the unpatched arm with the receive IUs pointing
at ZERO_SIZE_PTR and four receive work requests posted against them, and is
refused with -ECONNRESET afterwards. A target reporting 64 logs in and
completes a full response identically either way. The test is not part of
these patches.

This is an out of bounds read, and on a virtually mapped transport a
read of a wild address. Neither patch is about a write or a use after
free.

Yehyeong Lee (2):
  RDMA/srp: refuse a target that cannot receive an SRP_RSP
  RDMA/srp: drop a truncated SRP_RSP

 drivers/infiniband/ulp/srp/ib_srp.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)


base-commit: 330517c63aa9b48227b6ab45f01003effac77ae4
-- 
2.43.0

[PATCH 1/2] RDMA/srp: refuse a target that cannot receive an SRP_RSP

[Yehyeong Lee]

srp_cm_rep_handler() takes max_ti_iu_len straight from the target's
SRP_LOGIN_RSP and srp_alloc_iu_bufs() then sizes every receive IU with it,
so the target alone decides how large the initiator's receive buffers are.
Nothing enforces a lower bound.

The initiator parses a 36 byte header out of those buffers. A target that
reports a smaller max_ti_iu_len gets undersized buffers, and the header
reads in srp_process_rsp() then run past the allocation. A report of zero
is worse, because kzalloc() answers it with ZERO_SIZE_PTR, and on a
transport that maps addresses virtually ib_dma_map_single() returns that
pointer without looking at the length and ib_dma_mapping_error() reports
success, so the buffer reaches srp_recv_done() and the read of the opcode
is already out of bounds.

  BUG: KASAN: slab-out-of-bounds in srp_process_rsp+0x757/0xbe0
  Read of size 4 at addr ffff88800243f120 by task kunit_try_catch/42

  CPU: 0 UID: 0 PID: 42 Comm: kunit_try_catch Tainted: G                 N   7.3.0-13249-gcea86339412a-dirty #2 PREEMPT(lazy)
  Tainted: [N]=TEST
  Call Trace:
   <TASK>
   dump_stack_lvl+0x3f/0x50
   print_report+0xcd/0x620
   ? vprintk_emit+0x21d/0x380
   ? srp_process_rsp+0x757/0xbe0
   kasan_report+0xde/0x110
   ? srp_process_rsp+0x757/0xbe0
   srp_process_rsp+0x757/0xbe0
   srp_oob33_test+0x305/0x700
   ? __pfx_srp_oob33_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>

  Allocated by task 42:
   kasan_save_stack+0x33/0x60
   kasan_save_track+0x14/0x30
   __kasan_kmalloc+0x8f/0xa0
   __kmalloc_noprof+0x179/0x3b0
   srp_alloc_iu.constprop.0+0x69/0x5c0
   srp_cm_rep_handler+0xc2e/0x1bb0
   srp_test_login+0xc8/0x630
   srp_oob33_test+0xaa/0x700
   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 ffff88800243f100
   which belongs to the cache kmalloc-64 of size 64
  The buggy address is located 32 bytes inside of
   allocated 33-byte region [ffff88800243f100, ffff88800243f121)

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

Refuse the login instead. Both the largest messages the initiator parses,
SRP_RSP and SRP_AER_REQ, are 36 bytes, and a target that cannot send one
cannot speak the protocol, so nothing that works today is turned away.

Fixes: aef9ec39c47f ("IB: Add SCSI RDMA Protocol (SRP) initiator")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index 5811f02df9805..a85d28b95306a 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -2379,6 +2379,13 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
 
 	if (lrsp->opcode == SRP_LOGIN_RSP) {
 		ch->max_ti_iu_len = be32_to_cpu(lrsp->max_ti_iu_len);
+		if (ch->max_ti_iu_len < sizeof(struct srp_rsp)) {
+			shost_printk(KERN_ERR, target->scsi_host, PFX
+				     "Target reported max_ti_iu_len %u, too small for a %zu byte SRP_RSP\n",
+				     ch->max_ti_iu_len, sizeof(struct srp_rsp));
+			ret = -ECONNRESET;
+			goto error;
+		}
 		ch->req_lim       = be32_to_cpu(lrsp->req_lim_delta);
 		ch->use_imm_data  = srp_use_imm_data &&
 			(lrsp->rsp_flags & SRP_LOGIN_RSP_IMMED_SUPP);
-- 
2.43.0

[PATCH 2/2] RDMA/srp: drop a truncated SRP_RSP

[Yehyeong Lee]

srp_process_rsp() reads the whole 36 byte SRP_RSP header, the tag, the
credit delta, the flags, the status, the residual counts and the two data
lengths, without ever checking how many bytes the target actually sent.
Its two siblings, srp_process_cred_req() and srp_process_aer_req(), both
refuse a message shorter than their header.

A short response therefore makes the initiator parse whatever the receive
IU happened to hold from an earlier response, since the IUs are reused, and
act on a stale tag, status and residual count.

Refuse it the same way the siblings do. A conforming target always sends at
least the header, because a response shorter than that carries no tag to
complete a command with.

Fixes: aef9ec39c47f ("IB: Add SCSI RDMA Protocol (SRP) initiator")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index a85d28b95306a..d891029087b7e 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -1946,6 +1946,12 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp,
 	struct scsi_cmnd *scmnd;
 	unsigned long flags;
 
+	if (byte_len < sizeof(*rsp)) {
+		shost_printk(KERN_ERR, target->scsi_host, PFX
+			     "dropping truncated SRP_RSP (%u bytes)\n", byte_len);
+		return;
+	}
+
 	if (unlikely(rsp->tag & SRP_TAG_TSK_MGMT)) {
 		spin_lock_irqsave(&ch->lock, flags);
 		ch->req_lim += be32_to_cpu(rsp->req_lim_delta);
-- 
2.43.0

Re: [PATCH 1/2] RDMA/srp: refuse a target that cannot receive an SRP_RSP

[Bart Van Assche]

On 10/25/26 4:41 AM, Yehyeong Lee wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index 5811f02df9805..a85d28b95306a 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -2379,6 +2379,13 @@ static void srp_cm_rep_handler(struct ib_cm_id *cm_id,
>   
>   	if (lrsp->opcode == SRP_LOGIN_RSP) {
>   		ch->max_ti_iu_len = be32_to_cpu(lrsp->max_ti_iu_len);
> +		if (ch->max_ti_iu_len < sizeof(struct srp_rsp)) {
> +			shost_printk(KERN_ERR, target->scsi_host, PFX
> +				     "Target reported max_ti_iu_len %u, too small for a %zu byte SRP_RSP\n",
> +				     ch->max_ti_iu_len, sizeof(struct srp_rsp));
> +			ret = -ECONNRESET;
> +			goto error;
> +		}
>   		ch->req_lim       = be32_to_cpu(lrsp->req_lim_delta);
>   		ch->use_imm_data  = srp_use_imm_data &&
>   			(lrsp->rsp_flags & SRP_LOGIN_RSP_IMMED_SUPP);

Is this change perhaps incomplete? When reconnecting. 
srp_cm_rep_handler() only allocates the receive buffers when ch->rx_ring
== NULL. srp_rport_reconnect() calls srp_create_ch_ib() and the latter
function keeps rx_ring, but ch->max_ti_iu_len is overwritten from each
new SRP_LOGIN_RSP. If a target reports a larger value on reconnect,
srp_recv_done() calls ib_dma_sync_single_for_{cpu,device}(dev, iu->dma,
ch->max_ti_iu_len, ...) past the mapped length. That breaks the DMA API
rules. The HCA itself can't overrun, because srp_post_recv() uses
iu-> size. Since the series is about bounding the receive IU size,
please make one of these additional changes:
* use iu->size in the two sync calls in srp_recv_done(), or
* on reconnect, refuse (or reallocate for) a max_ti_iu_len larger than 
the existing rx_ring[0]->size.

Thanks,

Bart.

Re: [PATCH 2/2] RDMA/srp: drop a truncated SRP_RSP

[Bart Van Assche]

On 10/25/26 4:41 AM, Yehyeong Lee wrote:
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
> index a85d28b95306a..d891029087b7e 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -1946,6 +1946,12 @@ static void srp_process_rsp(struct srp_rdma_ch *ch, struct srp_rsp *rsp,
>   	struct scsi_cmnd *scmnd;
>   	unsigned long flags;
>   
> +	if (byte_len < sizeof(*rsp)) {
> +		shost_printk(KERN_ERR, target->scsi_host, PFX
> +			     "dropping truncated SRP_RSP (%u bytes)\n", byte_len);
> +		return;
> +	}
> +
>   	if (unlikely(rsp->tag & SRP_TAG_TSK_MGMT)) {
>   		spin_lock_irqsave(&ch->lock, flags);
>   		ch->req_lim += be32_to_cpu(rsp->req_lim_delta);

Dropping a response causes its req_lim_delta credit to be lost. I'm
afraid this will cause trouble.

Thanks,

Bart.