Re: [PATCH v6 25/56] scsi: qla2xxx: Handle sts_cont_entry_ext_t for 29xx adapters

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] FORTIFY_SOURCE Buffer Over-Read Panic
- [High] DMA Synchronization Hazard
--

--- Patch [25]: [PATCH v6 25/56] scsi: qla2xxx: Handle sts_cont_entry_ext_t for 29xx adapters ---
commit 90b126fb190842b2de74935641902992115ca941
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Handle sts_cont_entry_ext_t for 29xx adapters

29xx adapters use 128-byte response queue entries
(sts_cont_entry_ext_t) instead of 64-byte (sts_cont_entry_t).  Update
all status continuation IOCB processing paths to branch on
IS_QLA29XX() and use the correct entry type and data payload size.

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202607142020.gqDaApes-lkp@intel.com/

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -271,10 +276,12 @@ int __qla_copy_purex_to_buffer(struct scsi_qla_host *vha,
>  
>  	do {
>  		while ((total_bytes > 0) && (entry_count_remaining > 0)) {
> -			new_pkt = (sts_cont_entry_t *)rsp_q->ring_ptr;
> -			*pkt = new_pkt;
> +			*pkt = rsp_q->ring_ptr;
> +			data = ((sts_cont_entry_t *)*pkt)->data;

[Severity: High]
Will this unconditional cast trigger a buffer over-read panic on QLA29XX
adapters?

When CONFIG_FORTIFY_SOURCE is enabled, the compiler tracks object bounds to
prevent buffer overflows. Here, the packet is cast to sts_cont_entry_t,
which limits the compiler's view of the data array to 60 bytes.

Since qla_sts_cont_data_size() returns 124 bytes for 29xx adapters, a
subsequent memcpy() using this data pointer could exceed the 60-byte known
bound and trigger a fortify_panic().

Could this dynamically select the correct pointer type, such as casting to
struct sts_cont_entry_ext, based on the adapter type?

> +			data_sz = qla_sts_cont_data_size(ha);

[ ... ]

> @@ -906,28 +915,24 @@ qla27xx_copy_multiple_pkt(struct scsi_qla_host *vha, void **pkt,
>  				continue;

[Severity: High]
Can this loop condition result in silent data corruption by copying stale
memory?

Looking at qla27xx_copy_multiple_pkt(), the driver polls the signature field
to wait for continuation IOCBs from the hardware:

if (rsp_q->ring_ptr->signature == RESPONSE_PROCESSED) {
    ...
    cpu_relax();
    continue;
}

The signature field is located at offset 60, which falls within the first
64-byte cacheline. Since 29xx adapters use 128-byte extended IOCBs, the
data payload extends into the second cacheline (bytes 64-127).

PCIe root complexes and CPU memory controllers do not guarantee ordering
between multiple cacheline writes within a single DMA transaction. If the
CPU observes the updated signature in the first cacheline, could it proceed
to copy the entire 124-byte payload while the second cacheline still
contains stale data?

>  			}
>  
> -			new_pkt = (sts_cont_entry_t *)rsp_q->ring_ptr;
> -			*pkt = new_pkt;
> +			*pkt = rsp_q->ring_ptr;
> +			data = ((sts_cont_entry_t *)*pkt)->data;

[Severity: High]
Similarly, does this cast also restrict the object bounds for FORTIFY_SOURCE
and risk a panic when copying up to 124 bytes during
qla27xx_copy_multiple_pkt()?

> +			data_sz = qla_sts_cont_data_size(ha);

[ ... ]

> @@ -1190,10 +1206,12 @@ qla27xx_copy_fpin_pkt(struct scsi_qla_host *vha, void **pkt,
>  				continue;

[Severity: High]
Does this exhibit the same DMA synchronization hazard when polling the
signature field in qla27xx_copy_fpin_pkt() while waiting for 128-byte
entries?

>  			}
>  
> -			new_pkt = (sts_cont_entry_t *)rsp_q->ring_ptr;
> -			*pkt = new_pkt;
> +			*pkt = rsp_q->ring_ptr;
> +			data = ((sts_cont_entry_t *)*pkt)->data;

[Severity: High]
Does this cast also restrict the object bounds for FORTIFY_SOURCE and risk
a panic when copying up to 124 bytes during qla27xx_copy_fpin_pkt()?

> +			data_sz = qla_sts_cont_data_size(ha);

[ ... ]

> @@ -3691,6 +3711,7 @@ qla2x00_status_cont_entry(struct rsp_que *rsp, sts_cont_entry_t *pkt)
>  	struct scsi_cmnd *cp;
>  	uint32_t sense_len;
>  	uint8_t *sense_ptr;
> +	u8 *data = ((sts_cont_entry_t *)pkt)->data;
>  
>  	if (!sp || !GET_CMD_SENSE_LEN(sp))
>  		return;
> @@ -3707,15 +3728,15 @@ qla2x00_status_cont_entry(struct rsp_que *rsp, sts_cont_entry_t *pkt)
>  		return;
>  	}
>  
> -	if (sense_len > sizeof(pkt->data))
> -		sense_sz = sizeof(pkt->data);
> +	if (sense_len > data_sz)
> +		sense_sz = data_sz;
>  	else
>  		sense_sz = sense_len;
>  
>  	/* Move sense data. */
>  	if (IS_FWI2_CAPABLE(ha))
> -		host_to_fcp_swap(pkt->data, sizeof(pkt->data));
> -	memcpy(sense_ptr, pkt->data, sense_sz);
> +		host_to_fcp_swap(data, data_sz);
> +	memcpy(sense_ptr, data, sense_sz);

[Severity: High]
Will this trigger a fortify_panic() when handling 128-byte payloads?

The function qla2x00_status_cont_entry() now accepts a void pointer but
hardcodes the data pointer cast to sts_cont_entry_t. This destroys the
compiler's view of the 124-byte payload.

Since sense_sz can evaluate to 124 bytes for 29xx adapters, calling memcpy()
with a data pointer bounded to 60 bytes by the compiler will result in a
buffer over-read panic.

Could the data pointer be derived using the appropriate extended structure
when handling 29xx adapters?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723050413.3897522-1-njavali@marvell.com?part=25