Re: [PATCH v3 28/88] scsi: qla2xxx: Enhance purex_entry handling for 29xx series

[sashiko-bot@kernel.org]

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

New issues:
- [High] Copying continuation IOCBs will trigger a runtime panic under CONFIG_FORTIFY_SOURCE=y on 29xx hardware because the memcpy source bound is evaluated against a 64-byte struct while copying up to 124 bytes.

Pre-existing issues:
- [High] Non-atomic memset of the locklessly shared `default_item` creates a race condition that corrupts payload and control structures for concurrent PUREX allocations.
--

--- Patch [28]: [PATCH v3 28/88] scsi: qla2xxx: Enhance purex_entry handling for 29xx series ---
commit 085db48bd7b4d1648d53d6f0f6d5ff0614740cce
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enhance purex_entry handling for 29xx series

Update function signatures and internal logic across qla_edif.c, qla_isr.c,
and qla_os.c to accept a generic pointer for packet data. This differentiates
between standard purex_entry_24xx and the extended purex_entry_24xx_ext
structures based on IS_QLA29XX(). It ensures proper initialization and
processing of command and response data for both 64-byte and 128-byte
PUREX IOCBs.

> 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

[ ... ]

> @@ -262,14 +275,24 @@ int __qla_copy_purex_to_buffer(struct scsi_qla_host *vha,
>  	}
>  
>  	pending_bytes = total_bytes = tpad;
> -	no_bytes = (pending_bytes > sizeof(purex->els_frame_payload))  ?
> -	    sizeof(purex->els_frame_payload) : pending_bytes;
> +	no_bytes = (pending_bytes > payload_size) ?
> +		payload_size : pending_bytes;
> +	if (IS_QLA29XX(ha)) {
> +		struct purex_entry_24xx_ext *purex_ext = *pkt;
>  
> -	memcpy(buf, &purex->els_frame_payload[0], no_bytes);
> +		memcpy(buf, &purex_ext->els_frame_payload[0], no_bytes);
> +	} else {
> +		memcpy(buf, &purex->els_frame_payload[0], no_bytes);
> +	}
>  	buffer_copy_offset += no_bytes;
>  	pending_bytes -= no_bytes;
>  	--entry_count_remaining;
>  
> +	/*
> +	 * response_t::signature and struct response_ext::signature are both u32
> +	 * at offset 60 (handle:4 + data[52]:60), so the 24xx view writes
> +	 * the right slot regardless of stride.
> +	 */
>  	((response_t *)purex)->signature = RESPONSE_PROCESSED;
>  	/* flush signature */
>  	wmb();

[Severity: High]
Will this cause a runtime panic under CONFIG_FORTIFY_SOURCE=y on 29xx hardware
when processing continuation IOCBs?

Later in __qla_copy_purex_to_buffer() (and similarly in
qla27xx_copy_multiple_pkt() and qla27xx_copy_fpin_pkt()), the continuation
IOCB data is extracted using the 64-byte struct view:

do {
    while ((total_bytes > 0) && (entry_count_remaining > 0)) {
        *pkt = rsp_q->ring_ptr;
        data = ((sts_cont_entry_t *)*pkt)->data;
        data_sz = qla_sts_cont_data_size(ha);
        ...
        memcpy((buf + buffer_copy_offset), data, no_bytes);

The compiler tracks the object size of sts_cont_entry_t::data as exactly
60 bytes. On 29xx hardware, data_sz can be up to 124 bytes. Is it possible
that FORTIFY_SOURCE will intercept this as a deterministic out-of-bounds
read (124 > 60) and panic the kernel?

[ ... ]

> @@ -1075,9 +1110,9 @@ qla24xx_alloc_purex_item(scsi_qla_host_t *vha, uint16_t size)
>  	struct purex_item *item = NULL;
>  	uint8_t item_hdr_size = sizeof(*item);
>  
> -	if (size > QLA_DEFAULT_PAYLOAD_SIZE) {
> +	if (size > QLA_MAX_IOCB_SIZE) {
>  		item = kzalloc(item_hdr_size +
> -		    (size - QLA_DEFAULT_PAYLOAD_SIZE), GFP_ATOMIC);
> +		    (size - QLA_MAX_IOCB_SIZE), GFP_ATOMIC);
>  	} else {
>  		if (atomic_inc_return(&vha->default_item.in_use) == 1) {
>  			item = &vha->default_item;

[Severity: High]
This is a pre-existing issue, but can the non-atomic memset of default_item
create a race condition here?

If heavy I/O exhausts standard PUREX item allocations,
qla24xx_alloc_purex_item() falls back to the locklessly shared default_item:

    if (atomic_inc_return(&vha->default_item.in_use) == 1) {
        item = &vha->default_item;

When this item is later freed in qla24xx_free_purex_item(), a non-atomic
memset is used to clear it:

drivers/scsi/qla2xxx/qla_os.c:qla24xx_free_purex_item() {
    if (item == &item->vha->default_item)
        memset(&item->vha->default_item, 0, sizeof(struct purex_item));
}

If memset clears the in_use atomic flag before finishing zeroing the rest
of the struct, could a concurrent hardware interrupt successfully claim the
item and begin writing its payload, only to have the ongoing memset
overwrite and corrupt the newly written data?

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