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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The `vp_idx` is truncated to 8 bits, and the 24xx struct layout is incorrectly used to populate the 29xx response IOCB, corrupting vport indexing and SOF type.
- [High] `memcpy` from `sts_cont_entry_t->data` triggers a `FORTIFY_SOURCE` panic on 29xx hardware because it copies up to 124 bytes from an array with a statically defined size of 60 bytes.
- [High] Mismatched DMA free size for `rsp_els` on QLA29XX adapters.

Pre-existing issues:
- [High] `__qla_consume_iocb` skips marking the head slot as processed and prematurely marks unrelated future slots, leading to dropped hardware responses.
- [High] `qla24xx_alloc_purex_item` has a data race on `default_item.in_use` with `qla24xx_free_purex_item`.
--

--- Patch [28]: [PATCH v5 28/56] scsi: qla2xxx: Enhance purex_entry handling for 29xx series ---
commit 778e5ac5d8bcf791b8a97a7b9c1906d28083e550
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enhance purex_entry handling for 29xx series

This commit updates function signatures and internal logic across qla_edif.c,
qla_isr.c, and qla_os.c to differentiate between standard and extended purex
structures based on IS_QLA29XX(). It ensures proper initialization and
processing of command and response data for 64-byte and 128-byte PUREX IOCBs
across ELS paths.

> diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
> index b05f8e0b705ee..ade1d81785736 100644
> --- a/drivers/scsi/qla2xxx/qla_edif.c
> +++ b/drivers/scsi/qla2xxx/qla_edif.c
[ ... ]
> @@ -2544,26 +2544,55 @@ void qla24xx_auth_els(scsi_qla_host_t *vha, void **pkt, struct rsp_que **rsp)
>  	struct fc_port *fcport;
>  	struct qla_els_pt_arg a;
>  	be_id_t beid;
> +	__le16 nport_handle;
> +	__le32 rx_xchg_addr;
> +	__le16 ox_id;
> +	__le16 frame_size, status_flags, trunc_frame_size;
> +	uint8_t s_id[3], d_id[3];
> +	uint8_t vp_idx;
> +	struct purex_entry_24xx *p = *pkt;
>  
>  	memset(&a, 0, sizeof(a));
>  
> +	/*
> +	 * purex_entry_24xx_ext (29xx) overlays purex_entry_24xx for every
> +	 * field touched here -- nport_handle, rx_xchg_addr, ox_id, frame_size,
> +	 * status_flags, trunc_frame_size, s_id[3], d_id[3] -- with only
> +	 * vp_idx differing in width (u8 at offset 6 vs __le16 at offsets 6-7,
> +	 * with reserved2 at offset 7 in the 24xx layout). So all reads but
> +	 * vp_idx go through a single struct purex_entry_24xx * view.
> +	 */
> +	nport_handle = p->nport_handle;
> +	rx_xchg_addr = p->rx_xchg_addr;
> +	ox_id = p->ox_id;
> +	frame_size = p->frame_size;
> +	status_flags = p->status_flags;
> +	trunc_frame_size = p->trunc_frame_size;
> +	memcpy(s_id, p->s_id, sizeof(s_id));
> +	memcpy(d_id, p->d_id, sizeof(d_id));
> +	if (IS_QLA29XX(ha))
> +		vp_idx = le16_to_cpu(((struct purex_entry_24xx_ext *)
> +				      *pkt)->vp_idx);
> +	else
> +		vp_idx = p->vp_idx;

[Severity: High]
Does this code truncate the 9-bit virtual port index?

Since vp_idx is defined as a uint8_t, assigning the 16-bit vp_idx from
the 29xx extended structure drops the MSB, which might corrupt the
vport index routing.

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 5aa45b8f49290..75ce09933afc1 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
[ ... ]
> @@ -202,6 +202,11 @@ void __qla_consume_iocb(struct scsi_qla_host *vha,
>  	struct rsp_que *rsp_q = *rsp;
>  	response_t *new_pkt;
>  	uint16_t entry_count_remaining;
> +	/*
> +	 * entry_count is u8 at offset 1 in both purex_entry_24xx and
> +	 * purex_entry_24xx_ext, so the 24xx view is layout-compatible with
> +	 * either stride.
> +	 */
>  	struct purex_entry_24xx *purex = *pkt;
>  
>  	entry_count_remaining = purex->entry_count;

[Severity: High]
This is a pre-existing issue, but does the loop in __qla_consume_iocb()
corrupt the ring tracking by skipping the head slot?

If the caller, such as qla24xx_process_response_queue(), advances the ring
pointer before calling this function, the loop will grab the next slot:

void __qla_consume_iocb(...)
{
    ...
    while (entry_count_remaining > 0) {
        new_pkt = rsp_q->ring_ptr;
        *pkt = new_pkt;
        qla_rsp_ring_advance(rsp_q);
        new_pkt->signature = RESPONSE_PROCESSED;
        ...
    }
}

This marks the newly advanced pointer as processed and leaks the original
head slot, which can lead to hardware hangs or dropped responses.

[ ... ]
> @@ -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;

[Severity: High]
Can the subsequent continuation loop in __qla_copy_purex_to_buffer()
overread the sts_cont_entry_t buffer on 29xx hardware?

Further down in this function (and similarly in qla27xx_copy_multiple_pkt
and qla2x00_status_cont_entry), the code copies data from the continuation
entry:

    data = ((sts_cont_entry_t *)*pkt)->data;
    data_sz = qla_sts_cont_data_size(ha);
    ...
    memcpy((buf + buffer_copy_offset), data, no_bytes);

On 29xx hardware, qla_sts_cont_data_size(ha) returns 124. However, the data
array in sts_cont_entry_t is statically defined as 60 bytes. Copying up to
124 bytes from a 60-byte array triggers a FORTIFY_SOURCE panic.

[ ... ]
> @@ -1126,14 +1161,20 @@ qla24xx_queue_purex_item(scsi_qla_host_t *vha, struct purex_item *pkt,
>  static struct purex_item
>  *qla24xx_copy_std_pkt(struct scsi_qla_host *vha, void *pkt)
>  {
> +	struct qla_hw_data *ha = vha->hw;
>  	struct purex_item *item;
> +	u16 copy_sz;
>  
> -	item = qla24xx_alloc_purex_item(vha,
> -					QLA_DEFAULT_PAYLOAD_SIZE);
> +	if (IS_QLA29XX(ha))
> +		copy_sz = sizeof(struct purex_entry_24xx_ext);
> +	else
> +		copy_sz = QLA_DEFAULT_PAYLOAD_SIZE;
> +
> +	item = qla24xx_alloc_purex_item(vha, copy_sz);

[Severity: High]
This is a pre-existing issue, but does allocating the fallback purex item
race with freeing it?

In qla24xx_alloc_purex_item(), the fallback item is claimed via:

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

However, qla24xx_free_purex_item() resets it using a non-atomic memset:

    if (item == &item->vha->default_item)
        memset(&item->vha->default_item, 0, sizeof(struct purex_item));

If another CPU calls atomic_inc_return() concurrently with the memset, the
atomic increment could be lost, allowing multiple CPUs to claim the
default item at the same time and corrupting memory.

> diff --git a/drivers/scsi/qla2xxx/qla_os.c b/drivers/scsi/qla2xxx/qla_os.c
> index 0b7c4ee73ebfd..c327d053f8eee 100644
> --- a/drivers/scsi/qla2xxx/qla_os.c
> +++ b/drivers/scsi/qla2xxx/qla_os.c
[ ... ]
> @@ -6209,15 +6211,26 @@ void qla24xx_process_purex_rdp(struct scsi_qla_host *vha,
>  	uint8_t *sfp = NULL;
>  	uint16_t sfp_flags = 0;
>  	uint rsp_payload_length = sizeof(*rsp_payload);
> +	uint8_t vp_idx;
> +	size_t purex_sz;
>  	int rval;
>  
>  	ql_dbg(ql_dbg_init + ql_dbg_verbose, vha, 0x0180,
>  	    "%s: Enter\n", __func__);
>  
> +	if (IS_QLA29XX(ha)) {
> +		vp_idx = le16_to_cpu(
> +		    ((struct purex_entry_24xx_ext *)purex)->vp_idx);
> +		purex_sz = sizeof(struct purex_entry_24xx_ext);
> +	} else {

[Severity: High]
Does this code truncate the 9-bit virtual port index, similar to the logic
in qla24xx_auth_els()?

The vp_idx is defined as a uint8_t, which causes the 16-bit vp_idx from
the extended structure to lose its MSB.

> @@ -6257,7 +6270,7 @@ void qla24xx_process_purex_rdp(struct scsi_qla_host *vha,
>  	rsp_els->handle = 0;
>  	rsp_els->nport_handle = purex->nport_handle;
>  	rsp_els->tx_dsd_count = cpu_to_le16(1);
> -	rsp_els->vp_index = purex->vp_idx;
> +	rsp_els->vp_index = vp_idx;
>  	rsp_els->sof_type = EST_SOFI3;

[Severity: High]
Does freeing rsp_els result in a size mismatch on 29xx hardware, and does
this corrupt the bitfield on 29xx?

This function dynamically allocates rsp_els_pkt using rsp_els_sz, which
is 128 bytes on 29xx hardware, and then casts it to the 64-byte
struct els_entry_24xx:

    if (IS_QLA29XX(ha))
        rsp_els_sz = sizeof(struct els_entry_24xx_ext);
    ...
    rsp_els_pkt = dma_alloc_coherent(..., rsp_els_sz, ...);
    rsp_els = rsp_els_pkt;

In the cleanup paths, the memory is freed using sizeof(*rsp_els), which
evaluates statically to 64 bytes:

    dma_free_coherent(&ha->pdev->dev, sizeof(*rsp_els), rsp_els, rsp_els_dma);

This passes a 64-byte size to free a 128-byte DMA allocation.

Additionally, writing the 8-bit EST_SOFI3 to the legacy sof_type offset on
the 29xx extended layout misaligns with the __le16 bitfield on
little-endian systems, incorrectly setting sof_type to 1 instead of 2.

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