[PATCH 0/2] scsi: aacraid: bound adapter-declared sense lengths

[PATCH 0/2] scsi: aacraid: bound adapter-declared sense lengths

[Yehyeong Lee]

Two spots in aacraid size a sense-data copy by the destination buffer
(SCSI_SENSE_BUFFERSIZE) rather than the source array, so an adapter that
over-reports its sense length makes the copy read past the source and
hand the trailing bytes -- the previous request's scatter-gather list
in the shared FIB area -- to user space.  Bound each copy at the source
array size, as the container sense paths already do.

The two sites have different origins (one is as old as the driver, the
other came with the native HBA path), so they are split for independent
backporting.

Yehyeong Lee (2):
  scsi: aacraid: bound the SRB sense data copy to the source buffer
  scsi: aacraid: bound the HBA sense response copy to the source buffer

 drivers/scsi/aacraid/aachba.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)


base-commit: 24a19b953e2a3d0ea5a8c4951a0ab8f32720c907
-- 
2.43.0

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

[Yehyeong Lee]

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.

The reply is placed in the FIB data area that the outgoing SRB request
was built in, and that area is not cleared between uses, so the bytes
past the sense array are the previous request's scatter-gather list --
DMA addresses and lengths -- which are then handed to user space in the
sense buffer of a pass-through command.

Cap the copy at the size of the source array, matching the container
sense paths in this driver.

Fixes: 1da177e4c3f4 ("Linux-2.6.12-rc2")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/aacraid/aachba.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index df9492247631f..89d8e33517584 100644
--- 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);
@@ -3544,7 +3544,7 @@ static void aac_srb_callback(void *context, struct fib * fibptr)
 
 		scsicmd->result |= SAM_STAT_CHECK_CONDITION;
 		len = min_t(u32, le32_to_cpu(srbreply->sense_data_size),
-			    SCSI_SENSE_BUFFERSIZE);
+			    sizeof(srbreply->sense_data));
 #ifdef AAC_DETAILED_STATUS_INFO
 		pr_warn("aac_srb_callback: check condition, status = %d len=%d\n",
 					le32_to_cpu(srbreply->status), len);
-- 
2.43.0

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

[sashiko-bot]

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

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

[Yehyeong Lee]

This patch only bounds the sense-data copy; it does not touch
scsicmd->result or the srb_status switch.  The result set in the
status != ST_OK block is in fact overwritten unconditionally by that
switch (every case assigns scsicmd->result with '='), so it is
pre-existing and independent of this length change; I have left it alone.
Resent as v2 (with the HBA patch dropped):
https://lore.kernel.org/linux-scsi/20261013150420.3168223-1-yhlee@isslab.korea.ac.kr/

[PATCH 2/2] scsi: aacraid: bound the HBA sense response copy to the source buffer

[Yehyeong Lee]

hba_resp_task_complete() copies the sense data from a native HBA
response into the command's sense buffer, using a length that is only
capped at SCSI_SENSE_BUFFERSIZE (96 bytes).  The source,
err->sense_response_buf, is HBA_SENSE_DATA_LEN_MAX (32 bytes), so an
adapter that reports a larger sense_response_data_len makes the copy
read up to 64 bytes past the array and hand that memory to user space
in the sense buffer.

Cap the copy at the size of the source array.

Fixes: ab5d129f93c9 ("scsi: aacraid: Add task management functionality")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/scsi/aacraid/aachba.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 89d8e33517584..23512648e389e 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -3579,7 +3579,7 @@ static void hba_resp_task_complete(struct aac_dev *dev,
 		int len;
 
 		len = min_t(u8, err->sense_response_data_len,
-			SCSI_SENSE_BUFFERSIZE);
+			sizeof(err->sense_response_buf));
 		if (len)
 			memcpy(scsicmd->sense_buffer,
 				err->sense_response_buf, len);
-- 
2.43.0

Re: [PATCH 2/2] scsi: aacraid: bound the HBA sense response copy to the source buffer

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] scsi: aacraid: truncation of HBA sense data
--

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

scsi: aacraid: bound the HBA sense response copy to the source buffer

This commit restricts the amount of sense data copied from the native HBA
response into the command's sense buffer. It caps the copy at the size of
the sense_response_buf array to prevent a potential out-of-bounds read.

> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
> index 89d8e33517584..23512648e389e 100644
> --- a/drivers/scsi/aacraid/aachba.c
> +++ b/drivers/scsi/aacraid/aachba.c
[ ... ]
> @@ -3579,7 +3579,7 @@ static void hba_resp_task_complete(struct aac_dev *dev,
>  		int len;
>  
>  		len = min_t(u8, err->sense_response_data_len,
> -			SCSI_SENSE_BUFFERSIZE);
> +			sizeof(err->sense_response_buf));

[Severity: Medium]
In hba_resp_task_complete(), does this change actually truncate valid
extended sense data returned by the firmware?

Looking at struct aac_native_hba, the err object is backed by a 512-byte
buffer:

union {
	struct aac_hba_resp err;
	u8 resp_bytes[FW_ERROR_BUFFER_SIZE];
} resp;

Because of this union, the original copy of up to SCSI_SENSE_BUFFERSIZE (96
bytes) appears to have been memory-safe. Capping the length to the size of
sense_response_buf limits the copy to 32 bytes. This would discard diagnostic
information whenever the HBA firmware returns a CHECK CONDITION status with
more than 32 bytes of sense data.

>  		if (len)
>  			memcpy(scsicmd->sense_buffer,
>  				err->sense_response_buf, len);

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

Re: [PATCH 2/2] scsi: aacraid: bound the HBA sense response copy to the source buffer

[Yehyeong Lee]

You are right.  struct aac_native_hba backs err with a 512-byte union
(resp.err / resp_bytes[FW_ERROR_BUFFER_SIZE]), so the copy stayed within
that buffer -- it is not an out-of-bounds read -- and capping the length
to the 32-byte sense_response_buf would have truncated valid sense data.
I have dropped this patch; v2 keeps only the SRB bound:
https://lore.kernel.org/linux-scsi/20261013150420.3168223-1-yhlee@isslab.korea.ac.kr/