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

[Bart Van Assche <bvanassche@acm.org>]

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.