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.