Re: [PATCH v2 31/60] scsi: qla2xxx: Enhance purex_entry handling for 29xx series

[sashiko-bot@kernel.org]

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

New issues:
- [High] The 9-bit `vp_idx` field extracted from QLA29XX extended IOCBs is silently truncated to 8 bits when assigned to a `uint8_t` local variable and other 8-bit struct fields.

Pre-existing issues:
- [Critical] The response queue polling logic overlaps the `signature` field with untrusted ELS payload data, allowing a remote attacker to wedge the queue or cause a kernel hard lockup.
- [High] Failure to correctly mark the head IOCB signature and over-consumption of IOCBs in `__qla_consume_iocb` leads to ring slot leaks and queue corruption.
--

commit 01f1d63d6b8d4fcc9af6e64914963d0a1cf5920b
Author: Nilesh Javali <njavali@marvell.com>
Subject: scsi: qla2xxx: Enhance purex_entry handling for 29xx series

This commit updates the QLogic qla2xxx driver to handle both standard and
extended 29xx series purex_entry structures. It uses a generic pointer to
process command and response data efficiently, branching only where field
encodings like vp_idx and els_frame_payload lengths differ.

> diff --git a/drivers/scsi/qla2xxx/qla_edif.c b/drivers/scsi/qla2xxx/qla_edif.c
> index b05f8e0b705ee..f8bc248e5d18e 100644
> --- a/drivers/scsi/qla2xxx/qla_edif.c
> +++ b/drivers/scsi/qla2xxx/qla_edif.c
> @@ -2544,26 +2544,58 @@ 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;
>  
>  	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.
> +	 */
> +	{
> +		struct purex_entry_24xx *p = *pkt;
> +
> +		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);

[Severity: High]
Does this code inadvertently truncate the 16-bit vp_idx from QLA29XX extended
IOCBs to 8 bits?

The local variable vp_idx is declared as uint8_t, and it is later assigned
to a.vp_idx (which is a u8 in struct qla_els_pt_arg) and
purex->pur_info.vp_idx. For NPIV ports with an index of 256 or higher, could
this wrap around and direct ELS responses to the wrong virtual port?

> +		else
> +			vp_idx = p->vp_idx;
> +	}
> +

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index f81cf70a0542c..76c66f060b7b4 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 __qla_consume_iocb() consume too many
IOCBs from the ring?

By the time this function is called, the main polling loop in
qla24xx_process_response_queue() has already advanced rsp->ring_ptr past the
head IOCB. Since this loop runs purex->entry_count times, doesn't it
start consuming from the second IOCB and steal one IOCB belonging to the
next hardware packet?

Additionally, does this mean the head IOCB's signature is never marked as
RESPONSE_PROCESSED, potentially causing queue corruption when the ring wraps
around?

[ ... ]

> @@ -914,6 +945,8 @@ qla27xx_copy_multiple_pkt(struct scsi_qla_host *vha, void **pkt,
>  
>  	if (is_purls)
>  		memcpy(iocb_pkt, &purls->payload[0], no_bytes);
> +	else if (IS_QLA29XX(ha))
> +		memcpy(iocb_pkt, &purex_ext->els_frame_payload[0], no_bytes);
>  	else
>  		memcpy(iocb_pkt, &purex->els_frame_payload[0], no_bytes);
>  	buffer_copy_offset += no_bytes;
> @@ -922,6 +955,8 @@ qla27xx_copy_multiple_pkt(struct scsi_qla_host *vha, void **pkt,
>  
>  	if (is_purls)
>  		((response_t *)purls)->signature = RESPONSE_PROCESSED;
> +	else if (IS_QLA29XX(ha))
> +		((struct response_ext *)purex_ext)->signature = RESPONSE_PROCESSED;
>  	else
>  		((response_t *)purex)->signature = RESPONSE_PROCESSED;

[Severity: Critical]
This is a pre-existing issue, but does the hardware polling logic here overlap
the signature field with untrusted ELS payload data?

In the purex_entry_24xx and purex_entry_24xx_ext structures, the payload
array physically overlaps the signature field at offset 60. Could a remote
FC port craft an unsolicited packet with the RESPONSE_PROCESSED byte sequence
(e.g., 0xDEADDEAD) at this offset, causing the response queue to be perceived
as already processed?

If so, would this lead to an infinite cpu_relax() soft-lockup loop in
qla27xx_copy_multiple_pkt() or otherwise wedge the queue?

>  	wmb();

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