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

[sashiko-bot@kernel.org]

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