[PATCH 0/1] RDMA/srpt: Fix read underflow information disclosure

[PATCH 0/1] RDMA/srpt: Fix read underflow information disclosure

[liuyong]

Hi,

This patch fixes an SRPT information disclosure that allows a remote
initiator with LUN access to read uninitialized target kernel memory,
potentially exposing sensitive data from previous uses of those pages.

SRPT creates RDMA contexts from the initiator's buffer descriptors before
parsing the CDB. For a READ underflow, target core reduces
cmd->data_length, and a backend such as rd_mcp fills only that length.
The prebuilt RDMA WRITE requests still send the full descriptor-sized
buffer, including the uninitialized tail. The underflow itself is valid;
the problem is transferring more data than the command provides.

The fix caps the data SGEs at cmd->data_length in srpt_queue_response().

The patch is based on Linux 7.4.0-rc1 commit
f66469d9905b39d2c260a15d7d6db450f4db67c1. The issue was reproduced on
the same commit using software RoCE and an rd_mcp RAMDISK with an explicit
ACL-LUN mapping. The triggering READ(10) requests one 512-byte block but
provides an 8192-byte data-in descriptor.

Relevant target kernel log excerpt:

[    7.496367] ib_srpt Received SRP_LOGIN_REQ with i_port_id 5352:502d:4556:494c:2d49:4e49:542d:3031, t_port_id 5054:00ff:fe12:3456:5054:00ff:fe12:3456 and it_iu_len 512 on port 1 (guid=fe80:0000:0000:0000:5054:00ff:fe12:3456); pkey 0xffff
[    7.528685] TARGET_CORE[srpt]: Expected Transfer Length: 8192 does not match SCSI CDB Length: 512 for SAM Opcode: 0x28

The patch has been compiled and verified to fix the reported issue.

A reproducer and additional reproduction materials are available.
Please contact me privately if you need them.

The issue was discovered and fixed by Qihoo 360's Tulongfeng multi-agent
system.

Regards,
liuyong
360 Vulnerability Research Institute

liuyong (1):
  RDMA/srpt: Limit read data transfers to the command length

 drivers/infiniband/ulp/srpt/ib_srpt.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

[PATCH 1/1] RDMA/srpt: Limit read data transfers to the command length

[liuyong]

SRPT builds RDMA contexts from the initiator's buffer descriptors before
parsing the CDB. For a READ underflow, target core reduces
cmd->data_length, but the prebuilt RDMA WRITE requests still cover the
full descriptor length. A backend such as rd_mcp fills only the shortened
command length, allowing the uninitialized tail to reach the initiator.

A remote initiator with access to the LUN can therefore read
uninitialized target kernel memory beyond the requested SCSI data,
potentially exposing sensitive data left from previous uses of those
pages.

For a READ underflow, limit the data SGEs in the RDMA WRITE chain to
cmd->data_length before posting it. Preserve memory registration and
invalidation requests, and retain zero-length RDMA WRITEs beyond the
limit so that work-request accounting and resource cleanup remain
unchanged.

The issue was discovered and fixed by Qihoo 360's Tulongfeng multi-agent
system.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Cc: stable@vger.kernel.org
Signed-off-by: liuyong <pkfxxxing@gmail.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad..2f7857ef4 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2913,6 +2913,26 @@ static void srpt_queue_response(struct se_cmd *cmd)
 			first_wr = rdma_rw_ctx_wrs(&ctx->rw, ch->qp,
 					ch->sport->port, NULL, first_wr);
 		}
+
+		if (cmd->se_cmd_flags & SCF_UNDERFLOW_BIT) {
+			struct ib_send_wr *wr;
+			u32 data_len = cmd->data_length;
+
+			/* Keep MR operations and WR accounting unchanged. */
+			for (wr = first_wr; wr != &send_wr; wr = wr->next) {
+				int j;
+
+				if (wr->opcode != IB_WR_RDMA_WRITE)
+					continue;
+				for (j = 0; j < wr->num_sge && data_len; j++) {
+					struct ib_sge *sge = &wr->sg_list[j];
+
+					sge->length = min(sge->length, data_len);
+					data_len -= sge->length;
+				}
+				wr->num_sge = j;
+			}
+		}
 	}
 
 	if (state != SRPT_STATE_MGMT)

Re: [PATCH 1/1] RDMA/srpt: Limit read data transfers to the command length

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Critical] RDMA/srpt: SCF_UNDERFLOW_BIT check in srpt_queue_response()
  misses underflows
- [High] RDMA/srpt: non-NULL sg_list on 0-SGE RDMA WRITE in
  srpt_queue_response()
--

commit e0d34787b9bf3f71d05931ff6cb12b336322ac7c
Author: liuyong <pkfxxxing@gmail.com>

RDMA/srpt: Limit read data transfers to the command length

This commit truncates the SGEs of prebuilt RDMA WRITE work requests to
cmd->data_length in srpt_queue_response() when SCF_UNDERFLOW_BIT is set.

> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 43c3ff1ad8e7e..2f7857ef4face 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -2913,6 +2913,26 @@ static void srpt_queue_response(struct se_cmd *cmd)
>  			first_wr = rdma_rw_ctx_wrs(&ctx->rw, ch->qp,
>  					ch->sport->port, NULL, first_wr);
>  		}
> +
> +		if (cmd->se_cmd_flags & SCF_UNDERFLOW_BIT) {

[Severity: Critical]
Can the prebuilt RDMA WRITE work requests still exceed cmd->data_length
without SCF_UNDERFLOW_BIT being set?

For SRP_DATA_DESC_INDIRECT, srpt_get_desc_tbl() sets *data_len (which
initializes cmd->data_length) from idb->len, while srpt_alloc_rw_ctxs()
allocates uninitialized scatterlist pages and builds RDMA WRITE work
requests from each idb->desc_list[i].len without checking that their sum
matches idb->len:

drivers/infiniband/ulp/srpt/ib_srpt.c:srpt_get_desc_tbl() {
    ...
		*data_len = be32_to_cpu(idb->len);
		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
				sg, sg_cnt);
    ...
}

If an initiator sends an indirect descriptor table where idb->len matches
the CDB transfer length while idb->desc_list[i].len is larger,
target_cmd_size_check() sees size == cmd->data_length and never sets
SCF_UNDERFLOW_BIT.  Similarly, on a READ command with a non-zero PROTECT
field when T10-PI is enabled, sbc_check_prot() overwrites cmd->data_length
with sectors * block_size before target_cmd_size_check() runs, so
SCF_UNDERFLOW_BIT is not set there either.  In both cases,
srpt_queue_response() skips this block and RDMA-writes uninitialized
target kernel pages to the initiator.

Should srpt_queue_response() clamp the RDMA WRITE SGEs to cmd->data_length
unconditionally (and srpt_get_desc_tbl() validate that the descriptor
lengths match idb->len)?

> +			struct ib_send_wr *wr;
> +			u32 data_len = cmd->data_length;
> +
> +			/* Keep MR operations and WR accounting unchanged. */
> +			for (wr = first_wr; wr != &send_wr; wr = wr->next) {
> +				int j;
> +
> +				if (wr->opcode != IB_WR_RDMA_WRITE)
> +					continue;
> +				for (j = 0; j < wr->num_sge && data_len; j++) {
> +					struct ib_sge *sge = &wr->sg_list[j];
> +
> +					sge->length = min(sge->length, data_len);
> +					data_len -= sge->length;
> +				}
> +				wr->num_sge = j;

[Severity: High]
When a read underflow spans multiple IB_WR_RDMA_WRITE work requests, can
leaving wr->sg_list non-NULL on trailing work requests where wr->num_sge
becomes 0 still transfer uninitialized memory on providers such as irdma?

Once data_len reaches 0, subsequent IB_WR_RDMA_WRITE work requests get
wr->num_sge = 0, but wr->sg_list remains non-NULL and wr->sg_list[0].length
keeps its original non-zero length.  In irdma_uk_rdma_write(), the 0-SGE
path still passes op_info->lo_sg_list to iw_set_fragment() for fragment 0,
and irdma_set_fragment() programs fragment 0 whenever sge is non-NULL:

drivers/infiniband/hw/irdma/uk.c:irdma_set_fragment() {
	if (sge) {
		set_64bit_val(wqe, offset,
			      FIELD_PREP(IRDMAQPSQ_FRAG_TO, sge->addr));
		set_64bit_val(wqe, offset + 8,
			      FIELD_PREP(IRDMAQPSQ_VALID, valid) |
			      FIELD_PREP(IRDMAQPSQ_FRAG_LEN, sge->length) |
			      FIELD_PREP(IRDMAQPSQ_FRAG_STAG, sge->lkey));
	} else {
    ...
}

The hardware then DMAs and RDMA-writes wr->sg_list[0].length bytes of
uninitialized target kernel memory for each trailing work request.
Clearing wr->sg_list = NULL when wr->num_sge == 0 (matching
srpt_zerolength_write()) avoids programming that fragment.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110030809.913063-1-pkfxxxing@gmail.com?part=1

[PATCH v2] RDMA/srpt: Limit read data transfers to the command length

[liuyong]

SRPT builds RDMA contexts from the initiator's buffer descriptors before
parsing the CDB. The resulting RDMA WRITE requests can exceed
cmd->data_length after target core shortens the command or when an
indirect descriptor's buffers exceed its advertised total length.
A backend such as rd_mcp fills only cmd->data_length bytes, leaving an
uninitialized tail that is still transferred to the initiator.

A remote initiator with access to the LUN can therefore read
uninitialized target kernel memory beyond the requested SCSI data,
potentially exposing sensitive data left from previous uses of those
pages.

Limit the data SGEs in every read response to cmd->data_length without
depending on SCF_UNDERFLOW_BIT. Keep MR operations and WR accounting
unchanged by retaining excess RDMA WRITEs as zero-length requests.
Clear sg_list when num_sge becomes zero so that providers such as irdma
do not program a non-zero first fragment from the original SGE list.

The issue was discovered and fixed by Qihoo 360's Tulongfeng multi-agent
system.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Cc: stable@vger.kernel.org
Signed-off-by: liuyong <pkfxxxing@gmail.com>
---

Changes in v2 (addressing the two issues reported by sashiko-bot):
- Limit every read response to cmd->data_length without relying on
  SCF_UNDERFLOW_BIT.
- Clear sg_list when num_sge is zero to avoid stale fragments on irdma.

This revision has been compiled and verified to fix the reported issue.

 drivers/infiniband/ulp/srpt/ib_srpt.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad..9257c6beb 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -2907,12 +2907,32 @@ static void srpt_queue_response(struct se_cmd *cmd)
 	if (ioctx->cmd.data_direction == DMA_FROM_DEVICE &&
 	    ioctx->cmd.data_length &&
 	    !ioctx->queue_status_only) {
+		struct ib_send_wr *wr;
+		u32 data_len = cmd->data_length;
+
 		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, NULL, first_wr);
 		}
+
+		/* Do not expose uninitialized data beyond cmd->data_length. */
+		for (wr = first_wr; wr != &send_wr; wr = wr->next) {
+			int j;
+
+			if (wr->opcode != IB_WR_RDMA_WRITE)
+				continue;
+			for (j = 0; j < wr->num_sge && data_len; j++) {
+				struct ib_sge *sge = &wr->sg_list[j];
+
+				sge->length = min(sge->length, data_len);
+				data_len -= sge->length;
+			}
+			wr->num_sge = j;
+			if (!j)
+				wr->sg_list = NULL;
+		}
 	}
 
 	if (state != SRPT_STATE_MGMT)

Re: [PATCH v2] RDMA/srpt: Limit read data transfers to the command length

[sashiko-bot]

> SRPT builds RDMA contexts from the initiator's buffer descriptors before
> parsing the CDB. The resulting RDMA WRITE requests can exceed
> cmd->data_length after target core shortens the command or when an
> indirect descriptor's buffers exceed its advertised total length.
> A backend such as rd_mcp fills only cmd->data_length bytes, leaving an
> uninitialized tail that is still transferred to the initiator.
> 
> A remote initiator with access to the LUN can therefore read
> uninitialized target kernel memory beyond the requested SCSI data,
> potentially exposing sensitive data left from previous uses of those
> pages.
> 
> Limit the data SGEs in every read response to cmd->data_length without
> depending on SCF_UNDERFLOW_BIT. Keep MR operations and WR accounting
> unchanged by retaining excess RDMA WRITEs as zero-length requests.
> Clear sg_list when num_sge becomes zero so that providers such as irdma
> do not program a non-zero first fragment from the original SGE list.
> 
> The issue was discovered and fixed by Qihoo 360's Tulongfeng multi-agent
> system.
> 
> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
> Cc: stable@vger.kernel.org
> Signed-off-by: liuyong <pkfxxxing@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110052827.963143-1-pkfxxxing@gmail.com?part=1

Re: [PATCH v2] RDMA/srpt: Limit read data transfers to the command length

[Bart Van Assche]

On 11/9/26 9:28 PM, liuyong wrote:
 > SRPT builds RDMA contexts from the initiator's buffer descriptors before
 > parsing the CDB. The resulting RDMA WRITE requests can exceed
 > cmd->data_length after target core shortens the command or when an
 > indirect descriptor's buffers exceed its advertised total length.

Agreed that both cases need to be addressed. For the second case,
srpt_get_desc_tbl() should reject SRP_CMD requests for which the sum
of the direct descriptor lengths does not match the total length in
the indirect buffer descriptor instead of building RDMA contexts for
an inconsistent descriptor table. That is a protocol violation by the
initiator and it is better to catch it while parsing the request than
when sending the response.

 > +		/* Do not expose uninitialized data beyond cmd->data_length. */
 > +		for (wr = first_wr; wr != &send_wr; wr = wr->next) {
 > +			int j;
 > +
 > +			if (wr->opcode != IB_WR_RDMA_WRITE)
 > +				continue;
 > +			for (j = 0; j < wr->num_sge && data_len; j++) {
 > +				struct ib_sge *sge = &wr->sg_list[j];
 > +
 > +				sge->length = min(sge->length, data_len);
 > +				data_len -= sge->length;
 > +			}
 > +			wr->num_sge = j;
 > +			if (!j)
 > +				wr->sg_list = NULL;
 > +		}

The work requests and SGEs returned by rdma_rw_ctx_wrs() are owned by
the RDMA R/W API. This loop relies on several implementation details
of that API: that data is transferred with IB_WR_RDMA_WRITE work
requests, that the remote address of each work request has been
computed in advance, that the work requests occur in data order and
that the work requests and SGE arrays may be modified by the caller.
Any future change in drivers/infiniband/core/rw.c that invalidates one
of these assumptions would silently reintroduce the information leak.

The irdma issue mentioned in the patch description shows another
problem with this approach: it causes work requests to be posted that
the RDMA R/W API never generates and hence that RDMA drivers have not
been tested with. If irdma does not handle num_sge == 0 correctly for
RDMA WRITE requests then that should be fixed in the irdma driver
rather than worked around in SRPT. Additionally, there is no guarantee
that other RDMA drivers handle such requests correctly.

Please consider the following approach instead:
- Reject inconsistent indirect descriptor tables in
   srpt_get_desc_tbl().
- In srpt_queue_response(), zero the part of the data buffer beyond
   cmd->data_length, e.g. with sg_zero_buffer(), if the data buffer is
   larger than cmd->data_length.

This is simpler, does not depend on RDMA R/W API internals or RDMA
driver behavior, only adds overhead for commands with a residual and
is easy to backport.

Thanks,

Bart.

Re: [PATCH v2] RDMA/srpt: Limit read data transfers to the command length

[liuyong]

Hi Bart,

> For the second case,
> srpt_get_desc_tbl() should reject SRP_CMD requests for which the sum
> of the direct descriptor lengths does not match the total length in
> the indirect buffer descriptor instead of building RDMA contexts for
> an inconsistent descriptor table.

Agreed. In v3, srpt_get_desc_tbl() sums the direct descriptor lengths in
u64 and rejects a total that differs from idb->len before allocating
RDMA contexts. The existing descriptor-count check is preserved.
I also initialize the command before returning BUSY for a parsing error,
so the response and completion paths have a valid command reference.

> - In srpt_queue_response(), zero the part of the data buffer beyond
>   cmd->data_length, e.g. with sg_zero_buffer(), if the data buffer is
>   larger than cmd->data_length.

I have replaced the WR/SGE modification loop with tail zeroing using
sg_zero_buffer(). v3 saves the original buffer length and clears the
unused tail before posting the existing RDMA transfers. Each separately
mapped SG list is synchronized for device access after zeroing. The work
requests and SGEs returned by the RDMA R/W API are left unchanged.

Thanks for the review and suggestions.

Regards,
liuyong
360 Vulnerability Research Institute

[PATCH v3] RDMA/srpt: Prevent uninitialized read buffer disclosure

[liuyong]

SRPT allocates read buffers and creates RDMA contexts from the
initiator's descriptors before parsing the CDB. When target core reduces
cmd->data_length, a backend such as rd_mcp fills only the shorter
length, but the prebuilt RDMA transfer still sends the full buffer. This
can disclose stale target kernel memory to a remote initiator with
access to the LUN.

Reject indirect descriptor tables whose direct descriptor lengths do not
sum to the advertised total before allocating RDMA contexts. Also
initialize commands rejected during descriptor parsing so the BUSY
response path has valid callbacks and a command reference.

Save the original buffer length and zero any read tail beyond
cmd->data_length before posting the existing RDMA transfers. Synchronize
each mapped SG list for device access after zeroing.

The issue was discovered by Qihoo 360's Tulongfeng multi-agent system.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Cc: stable@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: liuyong <pkfxxxing@gmail.com>
---
Developed against f66469d9905b39d2c260a15d7d6db450f4db67c1 (7.4-rc1).

 drivers/infiniband/ulp/srpt/ib_srpt.c | 41 +++++++++++++++++++++++++++
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 43 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad8..0bef01e20c 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -41,6 +41,7 @@
 #include <linux/kthread.h>
 #include <linux/string.h>
 #include <linux/delay.h>
+#include <linux/dma-mapping.h>
 #include <linux/atomic.h>
 #include <linux/inet.h>
 #include <rdma/ib_cache.h>
@@ -1115,8 +1116,10 @@ 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);
+		u64 total_len = 0;
 		int nbufs = be32_to_cpu(idb->table_desc.len) /
 				sizeof(struct srp_direct_buf);
+		int i;
 
 		if (nbufs >
 		    (srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt)) {
@@ -1128,6 +1131,11 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 			return -EINVAL;
 		}
 
+		for (i = 0; i < nbufs; i++)
+			total_len += be32_to_cpu(idb->desc_list[i].len);
+		if (total_len != be32_to_cpu(idb->len))
+			return -EINVAL;
+
 		*data_len = be32_to_cpu(idb->len);
 		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
 				sg, sg_cnt);
@@ -1304,6 +1312,7 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 	WARN_ON_ONCE(ioctx->recv_ioctx);
 	ioctx->n_rdma = 0;
 	ioctx->n_rw_ctx = 0;
+	ioctx->buf_len = 0;
 	ioctx->queue_status_only = false;
 	/*
 	 * transport_init_se_cmd() does not initialize all fields, so do it
@@ -1597,9 +1606,13 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 			pr_err("0x%llx: parsing SRP descriptor table failed.\n",
 			       srp_cmd->tag);
 		}
+		target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
+				scsilun_to_int(&srp_cmd->lun), 0,
+				cmd->sam_task_attr, dir, 0);
 		goto busy;
 	}
 
+	send_ioctx->buf_len = data_len;
 	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
 			     scsilun_to_int(&srp_cmd->lun), data_len,
 			     cmd->sam_task_attr, dir, TARGET_SCF_ACK_KREF);
@@ -2866,6 +2879,31 @@ static u8 tcm_to_srp_tsk_mgmt_status(const int tcm_mgmt_status)
 	return SRP_TSK_MGMT_FAILED;
 }
 
+static void srpt_zero_read_tail(struct srpt_send_ioctx *ioctx)
+{
+	struct ib_device *dev = ioctx->ch->sport->sdev->device;
+	u32 data_len = ioctx->cmd.data_length;
+	int i;
+
+	for (i = 0; i < ioctx->n_rw_ctx; i++) {
+		struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
+		struct scatterlist *sg;
+		unsigned int j;
+
+		for_each_sg(ctx->sg, sg, ctx->nents, j) {
+			u32 skip = min(data_len, sg->length);
+
+			data_len -= skip;
+			if (skip < sg->length)
+				sg_zero_buffer(sg, 1, sg->length - skip, skip);
+		}
+
+		if (!ib_uses_virt_dma(dev))
+			dma_sync_sg_for_device(dev->dma_device, ctx->sg,
+					       ctx->nents, DMA_TO_DEVICE);
+	}
+}
+
 /**
  * srpt_queue_response - transmit the response to a SCSI command
  * @cmd: SCSI target command.
@@ -2907,6 +2945,9 @@ static void srpt_queue_response(struct se_cmd *cmd)
 	if (ioctx->cmd.data_direction == DMA_FROM_DEVICE &&
 	    ioctx->cmd.data_length &&
 	    !ioctx->queue_status_only) {
+		if (ioctx->buf_len > cmd->data_length)
+			srpt_zero_read_tail(ioctx);
+
 		for (i = ioctx->n_rw_ctx - 1; i >= 0; i--) {
 			struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
 
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 506c11b843..78bba74148 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -204,6 +204,7 @@ struct srpt_rw_ctx {
  * @rdma_cqe:    RDMA completion queue element.
  * @state:       I/O context state.
  * @cmd:         Target core command data structure.
+ * @buf_len:     Original SRP data buffer length.
  * @sense_data:  SCSI sense data.
  * @n_rdma:      Number of work requests needed to transfer this ioctx.
  * @n_rw_ctx:    Size of rw_ctxs array.
@@ -223,6 +224,7 @@ struct srpt_send_ioctx {
 	struct ib_cqe		rdma_cqe;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;
+	u32			buf_len;
 	u8			n_rdma;
 	u8			n_rw_ctx;
 	bool			queue_status_only;
-- 
2.43.0

Re: [PATCH v3] RDMA/srpt: Prevent uninitialized read buffer disclosure

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Critical] RDMA/srpt: uninitialized read buffer leak in
  srpt_queue_response()
- [High] RDMA/srpt: Signed-off-by trailer uses handle instead of full
  real name
- [Low] RDMA/srpt: missing Fixes trailer for srpt_handle_cmd() busy path
  fix
--

commit 399b6a57609f765986bec0107faa12263b13a643
Author: liuyong <pkfxxxing@gmail.com>

> RDMA/srpt: Prevent uninitialized read buffer disclosure
>
> SRPT allocates read buffers and creates RDMA contexts from the
> initiator's descriptors before parsing the CDB. When target core reduces
> cmd->data_length, a backend such as rd_mcp fills only the shorter
> length, but the prebuilt RDMA transfer still sends the full buffer. This
> can disclose stale target kernel memory to a remote initiator with
> access to the LUN.
>
> Reject indirect descriptor tables whose direct descriptor lengths do not
> sum to the advertised total before allocating RDMA contexts. Also
> initialize commands rejected during descriptor parsing so the BUSY
> response path has valid callbacks and a command reference.
>
> Save the original buffer length and zero any read tail beyond
> cmd->data_length before posting the existing RDMA transfers. Synchronize
> each mapped SG list for device access after zeroing.
>
> The issue was discovered by Qihoo 360's Tulongfeng multi-agent system.
>
> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
> Cc: stable@vger.kernel.org
> Suggested-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: liuyong <pkfxxxing@gmail.com>

[Severity: High]
Documentation/process/submitting-patches.rst requires a full real name in
the Author header and Signed-off-by trailer rather than a single-word
handle or username ("liuyong").

> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 43c3ff1ad8e7e..0bef01e20ce57 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c

[ ... ]

> @@ -1597,9 +1606,13 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
>  			pr_err("0x%llx: parsing SRP descriptor table failed.\n",
>  			       srp_cmd->tag);
>  		}
> +		target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
> +				scsilun_to_int(&srp_cmd->lun), 0,
> +				cmd->sam_task_attr, dir, 0);
>  		goto busy;
>  	}

[Severity: Low]
Calling target_send_busy() on an uninitialized se_cmd when
srpt_get_desc_tbl() fails was introduced in 8b8807b9e982 ("scsi:
RDMA/srpt: Fix handling of command / TMF submission failure").  Should the
commit message also include a Fixes tag for that commit?

> @@ -2866,6 +2879,31 @@ static u8 tcm_to_srp_tsk_mgmt_status(const int tcm_mgmt_status)
>  	return SRP_TSK_MGMT_FAILED;
>  }
>  
> +static void srpt_zero_read_tail(struct srpt_send_ioctx *ioctx)
> +{
> +	struct ib_device *dev = ioctx->ch->sport->sdev->device;
> +	u32 data_len = ioctx->cmd.data_length;
> +	int i;
> +
> +	for (i = 0; i < ioctx->n_rw_ctx; i++) {
> +		struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
> +		struct scatterlist *sg;
> +		unsigned int j;
> +
> +		for_each_sg(ctx->sg, sg, ctx->nents, j) {
> +			u32 skip = min(data_len, sg->length);
> +
> +			data_len -= skip;
> +			if (skip < sg->length)
> +				sg_zero_buffer(sg, 1, sg->length - skip, skip);
> +		}
> +
> +		if (!ib_uses_virt_dma(dev))
> +			dma_sync_sg_for_device(dev->dma_device, ctx->sg,
> +					       ctx->nents, DMA_TO_DEVICE);
> +	}
> +}
> +
>  /**
>   * srpt_queue_response - transmit the response to a SCSI command
>   * @cmd: SCSI target command.
> @@ -2907,6 +2945,9 @@ static void srpt_queue_response(struct se_cmd *cmd)
>  	if (ioctx->cmd.data_direction == DMA_FROM_DEVICE &&
>  	    ioctx->cmd.data_length &&
>  	    !ioctx->queue_status_only) {
> +		if (ioctx->buf_len > cmd->data_length)
> +			srpt_zero_read_tail(ioctx);
> +
>  		for (i = ioctx->n_rw_ctx - 1; i >= 0; i--) {
>  			struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];

[Severity: Critical]
Can srpt_queue_response() still send uninitialized page memory to the
initiator within the first cmd->data_length bytes?

srpt_alloc_rw_ctxs() allocates ctx->sg with zero_page set to false before
the CDB is parsed, and __target_submit() only zeroes the first SG entry
(cmd->t_data_sg[0]) for non-SCF_SCSI_DATA_CDB commands:

drivers/target/target_core_transport.c:__target_submit() {
    ...
		if (!(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) &&
		    cmd->data_direction == DMA_FROM_DEVICE) {
			struct scatterlist *sgl = cmd->t_data_sg;
			unsigned char *buf = NULL;

			BUG_ON(!sgl);

			buf = kmap_local_page(sg_page(sgl));
			if (buf) {
				memset(buf + sgl->offset, 0, sgl->length);
				kunmap_local(buf);
			}
		}
    ...
}

When a command has multiple SG entries (such as a multi-segment indirect
descriptor or a direct descriptor larger than PAGE_SIZE), entries after the
first remain uninitialized.  If the initiator sends a command whose handler
does not overwrite or shrink cmd->data_length to the bytes actually written
(for example spc_emulate_request_sense(), which copies at most 96 bytes and
calls target_complete_cmd() with cmd->data_length up to 255, or a
control CDB handler that leaves reserved bytes untouched assuming a
zeroed buffer), srpt_zero_read_tail() skips the first cmd->data_length
bytes and the subsequent RDMA WRITE transfers stale kernel memory to the
remote initiator.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111065342.1282709-1-pkfxxxing@gmail.com?part=1

[PATCH v4] RDMA/srpt: Prevent uninitialized read buffer disclosure

[liuyong]

From: Yong Liu <pkfxxxing@gmail.com>

SRPT allocates read buffers and creates RDMA contexts from the
initiator's descriptors before parsing the CDB. When target core reduces
cmd->data_length, a backend such as rd_mcp fills only the shorter
length, but the prebuilt RDMA transfer still sends the full buffer.
Control CDB handlers can also leave bytes within cmd->data_length
unwritten, while target core only clears the first SG entry of these
preallocated buffers. Both cases can disclose stale target kernel memory
to a remote initiator with access to the LUN.

Reject indirect descriptor tables whose direct descriptor lengths do not
sum to the advertised total before allocating RDMA contexts. Also
initialize commands rejected during descriptor parsing so the BUSY
response path has valid callbacks and a command reference.

Save the original buffer length and zero the entire control read buffer
before command execution. For data READs, zero any tail beyond
cmd->data_length before posting the existing RDMA transfers. Synchronize
each mapped SG list for device access in the response path.

The issue was discovered by Qihoo 360's Tulongfeng multi-agent system.

Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF submission failure")
Cc: stable@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Yong Liu <pkfxxxing@gmail.com>
---
Changes in v4 (addressing the three issues reported by sashiko-bot):
- Zero the entire original control read buffer before command execution.
  Limit tail zeroing to data READs and synchronize mapped SG lists in
  the RDMA submission loop.
- Use Yong Liu in the author and Signed-off-by fields.
- Add the Fixes tag for the descriptor-parsing BUSY path initialization.

Developed against f66469d9905b39d2c260a15d7d6db450f4db67c1 (7.4-rc1).

 drivers/infiniband/ulp/srpt/ib_srpt.c | 48 +++++++++++++++++++++++++++
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 50 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad8..0ec230950b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -41,6 +41,7 @@
 #include <linux/kthread.h>
 #include <linux/string.h>
 #include <linux/delay.h>
+#include <linux/dma-mapping.h>
 #include <linux/atomic.h>
 #include <linux/inet.h>
 #include <rdma/ib_cache.h>
@@ -1115,8 +1116,10 @@ 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);
+		u64 total_len = 0;
 		int nbufs = be32_to_cpu(idb->table_desc.len) /
 				sizeof(struct srp_direct_buf);
+		int i;
 
 		if (nbufs >
 		    (srp_cmd->data_out_desc_cnt + srp_cmd->data_in_desc_cnt)) {
@@ -1128,6 +1131,11 @@ static int srpt_get_desc_tbl(struct srpt_recv_ioctx *recv_ioctx,
 			return -EINVAL;
 		}
 
+		for (i = 0; i < nbufs; i++)
+			total_len += be32_to_cpu(idb->desc_list[i].len);
+		if (total_len != be32_to_cpu(idb->len))
+			return -EINVAL;
+
 		*data_len = be32_to_cpu(idb->len);
 		return srpt_alloc_rw_ctxs(ioctx, idb->desc_list, nbufs,
 				sg, sg_cnt);
@@ -1304,6 +1312,7 @@ static struct srpt_send_ioctx *srpt_get_send_ioctx(struct srpt_rdma_ch *ch)
 	WARN_ON_ONCE(ioctx->recv_ioctx);
 	ioctx->n_rdma = 0;
 	ioctx->n_rw_ctx = 0;
+	ioctx->buf_len = 0;
 	ioctx->queue_status_only = false;
 	/*
 	 * transport_init_se_cmd() does not initialize all fields, so do it
@@ -1597,9 +1606,13 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 			pr_err("0x%llx: parsing SRP descriptor table failed.\n",
 			       srp_cmd->tag);
 		}
+		target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
+				scsilun_to_int(&srp_cmd->lun), 0,
+				cmd->sam_task_attr, dir, 0);
 		goto busy;
 	}
 
+	send_ioctx->buf_len = data_len;
 	rc = target_init_cmd(cmd, ch->sess, &send_ioctx->sense_data[0],
 			     scsilun_to_int(&srp_cmd->lun), data_len,
 			     cmd->sam_task_attr, dir, TARGET_SCF_ACK_KREF);
@@ -1613,6 +1626,11 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 			       GFP_KERNEL))
 		return;
 
+	/* Control CDB handlers may leave part of the response untouched. */
+	if (cmd->data_direction == DMA_FROM_DEVICE &&
+	    !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB))
+		sg_zero_buffer(sg, sg_cnt, send_ioctx->buf_len, 0);
+
 	target_submit(cmd);
 	return;
 
@@ -2866,6 +2884,26 @@ static u8 tcm_to_srp_tsk_mgmt_status(const int tcm_mgmt_status)
 	return SRP_TSK_MGMT_FAILED;
 }
 
+static void srpt_zero_read_tail(struct srpt_send_ioctx *ioctx)
+{
+	u32 data_len = ioctx->cmd.data_length;
+	int i;
+
+	for (i = 0; i < ioctx->n_rw_ctx; i++) {
+		struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
+		struct scatterlist *sg;
+		unsigned int j;
+
+		for_each_sg(ctx->sg, sg, ctx->nents, j) {
+			u32 skip = min(data_len, sg->length);
+
+			data_len -= skip;
+			if (skip < sg->length)
+				sg_zero_buffer(sg, 1, sg->length - skip, skip);
+		}
+	}
+}
+
 /**
  * srpt_queue_response - transmit the response to a SCSI command
  * @cmd: SCSI target command.
@@ -2907,9 +2945,19 @@ static void srpt_queue_response(struct se_cmd *cmd)
 	if (ioctx->cmd.data_direction == DMA_FROM_DEVICE &&
 	    ioctx->cmd.data_length &&
 	    !ioctx->queue_status_only) {
+		if ((cmd->se_cmd_flags & SCF_SCSI_DATA_CDB) &&
+		    ioctx->buf_len > cmd->data_length)
+			srpt_zero_read_tail(ioctx);
+
 		for (i = ioctx->n_rw_ctx - 1; i >= 0; i--) {
 			struct srpt_rw_ctx *ctx = &ioctx->rw_ctxs[i];
 
+			/* Make CPU writes and zeroed data visible to the RDMA device. */
+			if (!ib_uses_virt_dma(sdev->device))
+				dma_sync_sg_for_device(sdev->device->dma_device,
+						       ctx->sg, ctx->nents,
+						       DMA_TO_DEVICE);
+
 			first_wr = rdma_rw_ctx_wrs(&ctx->rw, ch->qp,
 					ch->sport->port, NULL, first_wr);
 		}
diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.h b/drivers/infiniband/ulp/srpt/ib_srpt.h
index 506c11b843..78bba74148 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.h
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.h
@@ -204,6 +204,7 @@ struct srpt_rw_ctx {
  * @rdma_cqe:    RDMA completion queue element.
  * @state:       I/O context state.
  * @cmd:         Target core command data structure.
+ * @buf_len:     Original SRP data buffer length.
  * @sense_data:  SCSI sense data.
  * @n_rdma:      Number of work requests needed to transfer this ioctx.
  * @n_rw_ctx:    Size of rw_ctxs array.
@@ -223,6 +224,7 @@ struct srpt_send_ioctx {
 	struct ib_cqe		rdma_cqe;
 	enum srpt_command_state	state;
 	struct se_cmd		cmd;
+	u32			buf_len;
 	u8			n_rdma;
 	u8			n_rw_ctx;
 	bool			queue_status_only;
-- 
2.43.0

Re: [PATCH v4] RDMA/srpt: Prevent uninitialized read buffer disclosure

[sashiko-bot]

> From: Yong Liu <pkfxxxing@gmail.com>
> 
> SRPT allocates read buffers and creates RDMA contexts from the
> initiator's descriptors before parsing the CDB. When target core reduces
> cmd->data_length, a backend such as rd_mcp fills only the shorter
> length, but the prebuilt RDMA transfer still sends the full buffer.
> Control CDB handlers can also leave bytes within cmd->data_length
> unwritten, while target core only clears the first SG entry of these
> preallocated buffers. Both cases can disclose stale target kernel memory
> to a remote initiator with access to the LUN.
> 
> Reject indirect descriptor tables whose direct descriptor lengths do not
> sum to the advertised total before allocating RDMA contexts. Also
> initialize commands rejected during descriptor parsing so the BUSY
> response path has valid callbacks and a command reference.
> 
> Save the original buffer length and zero the entire control read buffer
> before command execution. For data READs, zero any tail beyond
> cmd->data_length before posting the existing RDMA transfers. Synchronize
> each mapped SG list for device access in the response path.
> 
> The issue was discovered by Qihoo 360's Tulongfeng multi-agent system.
> 
> Fixes: b99f8e4d7bcd ("IB/srpt: convert to the generic RDMA READ/WRITE API")
> Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF submission failure")
> Cc: stable@vger.kernel.org
> Suggested-by: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Yong Liu <pkfxxxing@gmail.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112094138.1390409-1-pkfxxxing@gmail.com?part=1