Re: [PATCH 1/2] scsi: aacraid: bound the SRB sense data copy to the source buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] scsi: aacraid: silent data corruption due to clobbered scsicmd->result
--

commit 41e55cd2865c820cb5e9b169fa01f61a9291992f
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: aacraid: bound the SRB sense data copy to the source buffer

aac_srb_callback() copies the sense data from an adapter SRB reply into
the command's sense buffer, using a length that is only capped at
SCSI_SENSE_BUFFERSIZE (96 bytes). The source, srbreply->sense_data, is
AAC_SENSE_BUFFERSIZE (30 bytes), so an adapter that reports a larger
sense_data_size makes the copy read up to 66 bytes past the sense array.
Cap the copy at the size of the source array, matching the container
sense paths in this driver.

Cc: stable@vger.kernel.org

> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
> @@ -3418,7 +3418,7 @@ static void aac_srb_callback(void *context, struct fib * fibptr)
>  		pr_warn("aac_srb_callback: srb failed, status = %d\n",
>  				le32_to_cpu(srbreply->status));
>  		len = min_t(u32, le32_to_cpu(srbreply->sense_data_size),
> -			    SCSI_SENSE_BUFFERSIZE);
> +			    sizeof(srbreply->sense_data));
>  		scsicmd->result = DID_ERROR << 16 | SAM_STAT_CHECK_CONDITION;
>  		memcpy(scsicmd->sense_buffer,
>  				srbreply->sense_data, len);
>  	}
>  
>  	/*
>  	 * Next check the srb status
>  	 */
>  	switch ((le32_to_cpu(srbreply->srb_status))&0x3f) {
>  	case SRB_STATUS_ERROR_RECOVERY:
>  	case SRB_STATUS_PENDING:
>  	case SRB_STATUS_SUCCESS:
>  		scsicmd->result = DID_OK << 16;

[Severity: Critical]
Does this code unconditionally overwrite the error status set during a FIB
failure?

If the adapter returns a FIB failure (srbreply->status != ST_OK), the block
above sets scsicmd->result to DID_ERROR << 16 | SAM_STAT_CHECK_CONDITION.

It then immediately falls through to the switch on srb_status. For values
like SRB_STATUS_PENDING or SRB_STATUS_SUCCESS, the switch assigns
scsicmd->result = DID_OK << 16.

Could this clobber the error status and discard the SAM_STAT_CHECK_CONDITION
bit? If srbreply->scsi_status happens to be 0, the command would complete as
DID_OK despite the earlier error.

Can this lead to silent data corruption by causing the OS to ignore the
copied sense buffer and incorrectly assume data was safely written?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261013140214.3156938-1-yhlee@isslab.korea.ac.kr?part=1