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

[sashiko-bot@kernel.org]

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