Re: [PATCH 1/1] scsi: target: Skip zero-length protection SG allocation

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] scsi: target: single-word handle in Author and Signed-off-by
  trailers
- [High] scsi: target: non-normal prot_op left set on zero-length
  commands
--

commit 7f7b93239c5a185f4d00d18137620ec5fea68e08
Author: liuyong <pkfxxxing@gmail.com>

> scsi: target: Skip zero-length protection SG allocation
>
> A READ(10) command with its TRANSFER LENGTH field set to zero, sent to a
> LUN with protection information enabled, can select TARGET_PROT_DIN_STRIP
> while leaving cmd->prot_length at zero. transport_generic_new_cmd() then
> passes that zero length to target_alloc_sgl(). This reaches
> sg_init_table() with no entries, so sg_init_marker() accesses an invalid
> last entry and causes a target-side Oops, which can also lead to
> use-after-free during subsequent iSCSI connection cleanup. A logged-in
> iSCSI initiator can trigger this on a FILEIO LUN with protection
> information enabled.
>
> Require a nonzero protection length before allocating its SG list,
> allowing commands with no protection payload to proceed without an
> empty allocation.
>
> These issues were discovered and fixed by Qihoo 360's Tulongfeng
> multi-agent system.
>
> Fixes: 5835812f13e5 ("target: ensure se_cmd->t_prot_sg is allocated when required")
> Cc: stable@vger.kernel.org
> Signed-off-by: liuyong <pkfxxxing@gmail.com>

[Severity: High]
The Author header and Signed-off-by trailer use a single-word handle
(liuyong) instead of a full real name as required by the Developer's
Certificate of Origin.

> diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
> index 9819515ce9674..67f71d739b158 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -2839,7 +2839,7 @@ transport_generic_new_cmd(struct se_cmd *cmd)
>  	int ret = 0;
>  	bool zero_flag = !(cmd->se_cmd_flags & SCF_SCSI_DATA_CDB);
>  
> -	if (cmd->prot_op != TARGET_PROT_NORMAL &&
> +	if (cmd->prot_length && cmd->prot_op != TARGET_PROT_NORMAL &&
>  	    !(cmd->se_cmd_flags & SCF_PASSTHROUGH_PROT_SG_TO_MEM_NOALLOC)) {
>  		ret = target_alloc_sgl(&cmd->t_prot_sg, &cmd->t_prot_nents,
>  				       cmd->prot_length, true, false);

[Severity: High]
Does skipping only the scatterlist allocation leave cmd->prot_op set to a
non-normal value while cmd->t_prot_sg and cmd->t_data_sg are NULL?

sbc_check_prot() sets cmd->prot_op and cmd->prot_type before computing
cmd->prot_length = dev->prot_length * sectors, even when sectors is 0.
When the zero-length READ completes, fabric drivers that branch on
cmd->prot_op in queue_data_in() assume protection and data scatterlists
exist:

drivers/scsi/qla2xxx/qla_target.c:qlt_xmit_response() {
    ...
	if (cmd->se_cmd.prot_op && (xmit_type & QLA_TGT_XMIT_DATA))
		res = qlt_build_ctio_crc2_pkt(qpair, &prm);
	else
		res = qlt_24xx_build_ctio_pkt(qpair, &prm);
    ...
	if (qlt_has_data(cmd)) {
        ...
	} else
		qlt_24xx_init_ctio_to_isp((struct ctio7_to_24xx *)pkt,
		    &prm);
    ...
}

In qla2xxx, qlt_build_ctio_crc2_pkt() allocates cmd->ctx from
ha->dl_dma_pool (which leaks because cmd->sg_mapped is 0) and builds a
CTIO_CRC2 IOCB that qlt_24xx_init_ctio_to_isp() then overwrites as a Mode 1
status IOCB before submitting it to firmware.  In ib_isert,
isert_rdma_rw_ctx_post() sees isert_prot_cmd() as true and passes NULL
scatterlists to rdma_rw_ctx_signature_init().

Would it be better to skip setting cmd->prot_op in sbc_check_prot() when
sectors is 0?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261109092744.822646-1-pkfxxxing@gmail.com?part=1