Re: [PATCH v4 30/40] scsi: ibmvscsi_tgt: use 16-bits defined sense codes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Sense data built on the `srp_transfer_data` error path is discarded because `se_cmd->scsi_status` is not set, causing a false SUCCESS response to be sent.
--

commit 7b949e6613d985b08c96328371e1a1b2e1e41b6d
Author: Damien Le Moal <dlemoal@kernel.org>

scsi: ibmvscsi_tgt: use 16-bits defined sense codes

This commit refactors the IBM virtual SCSI target driver to use
scsi_set_sense_buffer() and replaces all hard-coded additional sense
codes with enum values. It aims to improve code clarity and testing
without any functional changes.

> diff --git a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> index 61f6828007654..e2af90b6da39b 100644
> --- a/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> +++ b/drivers/scsi/ibmvscsi_tgt/ibmvscsi_tgt.c
> @@ -3757,9 +3757,8 @@ static int ibmvscsis_queue_data_in(struct se_cmd *se_cmd)
>  		dev_err(&vscsi->dev, "srp_transfer_data failed: %d\n", rc);
>  		se_cmd->scsi_sense_length = 18;
>  		memset(se_cmd->sense_buffer, 0, se_cmd->scsi_sense_length);
> -		/* Logical Unit Communication Time-out asc/ascq = 0x0801 */
> -		scsi_build_sense_buffer(0, se_cmd->sense_buffer, MEDIUM_ERROR,
> -					0x08, 0x01);
> +		scsi_set_sense_buffer(0, se_cmd->sense_buffer, MEDIUM_ERROR,
> +				      LU_COMMUNICATION_TIMEOUT);

[Severity: Critical]
This isn't a bug introduced by this patch, but it appears there is a
pre-existing flaw in how the sense data is handled here.

Because se_cmd->scsi_status is not updated to SAM_STAT_CHECK_CONDITION
when srp_transfer_data() fails, srp_build_response() will call
ibmvscsis_fast_fail() which sees the unchanged status of 0. As a result,
the block responsible for handling sense data in srp_build_response() is
skipped:

	rsp->status = ibmvscsis_fast_fail(vscsi, cmd);
	if (rsp->status) {
		...
		if (se_cmd->scsi_sense_length && se_cmd->sense_buffer) {
			rsp->sense_data_len = cpu_to_be32(se_cmd->scsi_sense_length);

This causes a SUCCESS response to be sent instead of a CHECK CONDITION,
which could lead the initiator to consume uninitialized or stale data from
its receive buffer. Would it make sense to update se_cmd->scsi_status to
SAM_STAT_CHECK_CONDITION here to ensure the failure is reported properly?

>  	}
>  
>  	srp_build_response(vscsi, cmd, &len);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260905032224.509860-1-dlemoal@kernel.org?part=30