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

[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

[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

[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

[Bart Van Assche]

On 11/12/26 1:41 AM, liuyong wrote:
 > +		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;

srpt_get_send_ioctx() zeroes ioctx->cmd, hence the current code
triggers a NULL pointer dereference in target_send_busy()
(cmd->se_tfo == NULL) if srpt_get_desc_tbl() fails. That is a separate
bug that can be triggered remotely. Please move this change into a
separate patch, mention the NULL pointer dereference in its description
and explain in a comment why the target_init_cmd() return value is
ignored.

 > +			/* 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);

This is unrelated to the information leak. If a DMA sync is missing
between filling the buffer and posting the RDMA WRITE, then that issue
applies to all data written by the backend and also affects other
users of the RDMA R/W API, e.g. nvmet-rdma. Additionally, this code
relies on how rdma_rw_ctx_init() maps the S/G list. If a sync is
needed, it should be added to the RDMA R/W API. Please leave this out
of this patch.

 > +	/* 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() only zeroes the first S/G element for control CDBs.
I think it's better to fix this in the target core by zeroing the
entire buffer there since that benefits all fabric drivers that provide
their own S/G lists. With that change, srpt only has to zero the part
of the buffer beyond cmd->data_length, for all READ commands.

 > +static void srpt_zero_read_tail(struct srpt_send_ioctx *ioctx)

The S/G lists of the RDMA contexts are chained and are passed to the
target core as cmd->t_data_sg / cmd->t_data_nents. Hence, this function
can be replaced with a single call:

	sg_zero_buffer(cmd->t_data_sg, cmd->t_data_nents,
		       ioctx->buf_len - cmd->data_length, cmd->data_length);

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

Please log the mismatch with pr_err(), similar to the check above it.
That makes it easier to debug interoperability issues with initiators.

Please also mention which tests have been run against the new version.

Do the SRP tests from https://github.com/linux-blktests/blktests still
pass?

Thanks,

Bart.

[PATCH v5 0/3] RDMA/srpt: Fix command initialization and read buffer disclosure

[Yong Liu]

Hi Bart,

Thanks for the review and suggestions. This revision separates the BUSY
initialization fix and the target core control-read buffer fix from the
SRPT descriptor validation and read-tail clearing fix. Together they
address the NULL pointer dereference on descriptor parsing errors and
uninitialized target kernel memory disclosure to an initiator.

Changes in v5:
- Move BUSY command initialization into patch 1 and explain why the
  target_init_cmd() return value is ignored.
- Clear all control-read SG entries in target core in patch 2.
- In patch 3, log inconsistent descriptor lengths and zero the read tail
  with one sg_zero_buffer() call through the command's chained SG list.
  Remove the SRPT-specific DMA synchronization.

> Do the SRP tests from https://github.com/linux-blktests/blktests still
> pass?

I ran the following against this v5 series in an x86-64 QEMU VM using RXE,
with blktests commit 79c32fb31b7bca6a4383b749030128683cf6b6b4:

    USE_RXE=1 ./check srp

Results: 13 passed, 2 skipped, 0 failed. Tests srp/003 and srp/004 require
legacy single-queue device mapper support, which is absent from this
kernel. The multiqueue test, srp/002, passed. The kernel and modules also
build successfully.

This series is based on commit:
f66469d9905b39d2c260a15d7d6db450f4db67c1

Regards,
Yong Liu
360 Vulnerability Research Institute

Yong Liu (3):
  RDMA/srpt: Initialize commands before returning BUSY
  scsi: target: Clear all control read scatterlist entries
  RDMA/srpt: Prevent uninitialized read buffer disclosure

 drivers/infiniband/ulp/srpt/ib_srpt.c  | 24 ++++++++++++++++++++++++
 drivers/infiniband/ulp/srpt/ib_srpt.h  |  2 ++
 drivers/target/target_core_transport.c | 19 ++++++-------------
 3 files changed, 32 insertions(+), 13 deletions(-)

-- 
2.43.0

[PATCH v5 1/3] RDMA/srpt: Initialize commands before returning BUSY

[Yong Liu]

srpt_get_send_ioctx() clears ioctx->cmd, but descriptor parsing can fail
before target_init_cmd() is called. The error path then calls
target_send_busy(), which dereferences the NULL cmd->se_tfo pointer.
A remote initiator can trigger this by submitting a command with an
invalid descriptor table.

Initialize the command before sending BUSY on descriptor parsing errors.
Do not request an acknowledgment reference since the command will not
be submitted to target core.

Fixes: 8b8807b9e982 ("scsi: RDMA/srpt: Fix handling of command / TMF submission failure")
Cc: stable@vger.kernel.org
Signed-off-by: Yong Liu <pkfxxxing@gmail.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 43c3ff1ad8..127f256c6e 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1597,6 +1597,13 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 			pr_err("0x%llx: parsing SRP descriptor table failed.\n",
 			       srp_cmd->tag);
 		}
+		/*
+		 * Even on -ESHUTDOWN, target_init_cmd() initializes the command
+		 * so it can be completed with BUSY without core submission.
+		 */
+		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;
 	}
 
-- 
2.43.0

[PATCH v5 2/3] scsi: target: Clear all control read scatterlist entries

[Yong Liu]

__target_submit() only clears the first scatterlist entry of a control
read buffer supplied by a fabric driver. Control CDB handlers can leave
response bytes untouched, so later entries can retain stale kernel data
that is returned to the initiator.

Clear every supplied scatterlist entry before command execution. Use
sg_zero_buffer() so entries spanning multiple pages are handled as well.
Keep clearing the full entries, including any part beyond data_length,
as the existing code does for the first entry.

Fixes: 944981c7e106 ("target: Add control CDB READ payload zero work-around")
Cc: stable@vger.kernel.org
Suggested-by: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Yong Liu <pkfxxxing@gmail.com>
---
 drivers/target/target_core_transport.c | 19 ++++++-------------
 1 file changed, 6 insertions(+), 13 deletions(-)

diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 927861c30a..267f5e2cc8 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1633,23 +1633,16 @@ static int __target_submit(struct se_cmd *cmd)
 	core_alua_check_nonop_delay(cmd);
 
 	if (cmd->t_data_nents != 0) {
-		/*
-		 * This is primarily a hack for udev and tcm loop which sends
-		 * INQUIRYs with a single page and expects the data to be
-		 * cleared.
-		 */
+		/* Control CDB handlers may leave response bytes untouched. */
 		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;
+			struct scatterlist *sg;
+			unsigned int i;
 
-			BUG_ON(!sgl);
+			BUG_ON(!cmd->t_data_sg);
 
-			buf = kmap_local_page(sg_page(sgl));
-			if (buf) {
-				memset(buf + sgl->offset, 0, sgl->length);
-				kunmap_local(buf);
-			}
+			for_each_sg(cmd->t_data_sg, sg, cmd->t_data_nents, i)
+				sg_zero_buffer(sg, 1, sg->length, 0);
 		}
 	}
 
-- 
2.43.0

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

[Yong Liu]

SRPT allocates read buffers and creates RDMA contexts from the initiator's
descriptors before parsing the CDB. When target core shortens
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, and log the mismatch before allocating RDMA
contexts. Save the original buffer length and zero the tail beyond
cmd->data_length through the chained command scatterlist before posting
RDMA transfers. This applies to both data and control READ commands.

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: Yong Liu <pkfxxxing@gmail.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 17 +++++++++++++++++
 drivers/infiniband/ulp/srpt/ib_srpt.h |  2 ++
 2 files changed, 19 insertions(+)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 127f256c6e..fc530ddf48 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1115,8 +1115,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 +1130,14 @@ 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)) {
+			pr_err("SRP descriptor lengths total %llu does not match advertised length %u\n",
+			       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 +1314,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
@@ -1607,6 +1618,7 @@ static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
 		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);
@@ -2914,6 +2926,11 @@ 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)
+			sg_zero_buffer(cmd->t_data_sg, cmd->t_data_nents,
+				       ioctx->buf_len - cmd->data_length,
+				       cmd->data_length);
+
 		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