Re: [PATCH v6 37/56] scsi: qla2xxx: Add size check for extended VP report ID entry

[sashiko-bot@kernel.org]

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

New issues:
- [High] Extracting `vp_idx` and `vp_status` using C bitfields over a little-endian `__le16` type causes undefined behavior and data corruption on Big-Endian architectures.

Pre-existing issues:
- [High] The vport pointer (`vp`) is accessed after `ha->vport_slock` is released without holding a reference, leading to a potential Use-After-Free.
- [High] The `fcport` list is traversed locklessly for Format 2 RIDA IOCBs, and the found `fcport` is accessed after dropping `tgt.sess_lock` without holding a reference, leading to data races and Use-After-Free.
- [High] Missing `dma_rmb()` read barrier after polling the hardware DMA response queue signature.
--

--- Patch [37]: [PATCH v6 37/56] scsi: qla2xxx: Add size check for extended VP report ID entry ---
commit b119ba076c242111b3050f1eff45472d3fdf947f
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Add size check for extended VP report ID entry

Add reserved_end[64] padding to bring the struct to 128 bytes, matching
the hardware IOCB stride. Change qla24xx_report_id_acquisition() to
accept a void pointer and extract vp_idx and vp_status from the extended
structure on 29xx series adapters.

> diff --git a/drivers/scsi/qla2xxx/qla_fw29.h b/drivers/scsi/qla2xxx/qla_fw29.h
> index 088a220472a50..a4aa5bacb171a 100644
> --- a/drivers/scsi/qla2xxx/qla_fw29.h
> +++ b/drivers/scsi/qla2xxx/qla_fw29.h

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
> index 128138b4d16a9..085961e04ec2d 100644
> --- a/drivers/scsi/qla2xxx/qla_isr.c
> +++ b/drivers/scsi/qla2xxx/qla_isr.c
> @@ -4175,8 +4175,7 @@ void qla24xx_process_response_queue(struct scsi_qla_host *vha,

[Severity: High]
This isn't a bug introduced by this patch, but looking at
qla24xx_process_response_queue(), is a dma_rmb() read barrier needed after
polling the hardware DMA response queue signature?

The driver checks if the signature is valid:

	while (rsp->ring_index != rsp_in &&
		       rsp->ring_ptr->signature != RESPONSE_PROCESSED) {
		pkt = (void *)rsp->ring_ptr;
		cur_ring_index = rsp->ring_index;

		qla_rsp_ring_advance(rsp);

		if (((response_t *)pkt)->entry_status != 0) {

Without a dma_rmb() barrier after observing the valid signature, can the CPU
speculatively reorder reads and fetch the queue entry payload before the
hardware's DMA write is fully visible?

>  			    qla_sts_cont_data_size(rsp->hw));
>  			break;
>  		case VP_RPT_ID_IOCB_TYPE:
> -			qla24xx_report_id_acquisition(vha,
> -			    (struct vp_rpt_id_entry_24xx *)pkt);
> +			qla24xx_report_id_acquisition(vha, pkt);
>  			break;
>  		case LOGINOUT_PORT_IOCB_TYPE:
>  			qla24xx_logio_entry(vha, rsp->req, pkt);

[ ... ]

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 3ebda35dd584f..cec308811d9eb 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -4086,15 +4086,18 @@ qla2x00_set_idma_speed(scsi_qla_host_t *vha, uint16_t loop_id,
>  }
>  
>  void
> -qla24xx_report_id_acquisition(scsi_qla_host_t *vha,
> -	struct vp_rpt_id_entry_24xx *rptid_entry)
> +qla24xx_report_id_acquisition(scsi_qla_host_t *vha, void *pkt)
>  {
>  	struct qla_hw_data *ha = vha->hw;
> +	struct vp_rpt_id_entry_24xx *rptid_entry = pkt;
> +	struct vp_rpt_id_entry_24xx_ext *rptid_entry_ext = pkt;
>  	scsi_qla_host_t *vp = NULL;
>  	unsigned long   flags;
>  	int found;
>  	port_id_t id;
>  	struct fc_port *fcport;
> +	u16 vp_idx;
> +	u8 vp_status;
>  
>  	ql_dbg(ql_dbg_mbx + ql_dbg_verbose, vha, 0x10b6,
>  	    "Entered %s.\n", __func__);
> @@ -4102,6 +4105,14 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha,
>  	if (rptid_entry->entry_status != 0)
>  		return;
>  
> +	if (IS_QLA29XX(ha)) {
> +		vp_idx = rptid_entry_ext->vp_idx;
> +		vp_status = rptid_entry_ext->vp_status;

[Severity: High]
Does this extract the wrong bits on big-endian architectures?

struct vp_rpt_id_entry_24xx_ext defines vp_idx and vp_status as bitfields
over an __le16 type:

	__le16	vp_idx : 9;		/* VP Index 9bits */
	__le16	vp_status : 7;		/* VP Status 7bits */

Since C bitfield layout is implementation- and endian-dependent, the
compiler on big-endian CPUs typically allocates the first bitfield to the
most significant bits.

Since the hardware writes the DMA data in little-endian format, should this
be defined as a single __le16 field and parsed via le16_to_cpu() combined
with bitwise masks to ensure it is endian-safe?

> +	} else {
> +		vp_idx = rptid_entry->vp_idx;
> +		vp_status = rptid_entry->vp_status;
> +	}
> +
>  	id.b.domain = rptid_entry->port_id[2];
>  	id.b.area   = rptid_entry->port_id[1];

[ ... ]

> @@ -4241,18 +4251,18 @@ qla24xx_report_id_acquisition(scsi_qla_host_t *vha,
>  			set_bit(REGISTER_FC4_NEEDED, &vha->dpc_flags);
>  			set_bit(REGISTER_FDMI_NEEDED, &vha->dpc_flags);
>  		} else {
> -			if (rptid_entry->vp_status != VP_STAT_COMPL &&
> -				rptid_entry->vp_status != VP_STAT_ID_CHG) {
> +			if (vp_status != VP_STAT_COMPL &&
> +				vp_status != VP_STAT_ID_CHG) {
>  				ql_dbg(ql_dbg_mbx, vha, 0x10ba,
>  				    "Could not acquire ID for VP[%d].\n",
> -				    rptid_entry->vp_idx);
> +				    vp_idx);
>  				return;
>  			}
>  
>  			found = 0;
>  			spin_lock_irqsave(&ha->vport_slock, flags);
>  			list_for_each_entry(vp, &ha->vp_list, list) {
> -				if (rptid_entry->vp_idx == vp->vp_idx) {
> +				if (vp_idx == vp->vp_idx) {
>  					found = 1;
>  					break;
>  				}
>  			}

[Severity: High]
This is a pre-existing issue, but can this lead to a use-after-free of the
vp pointer?

In qla24xx_report_id_acquisition(), the code iterates over ha->vp_list while
holding ha->vport_slock to find a matching vp. Once found, the lock is
released without taking a reference count (e.g., atomic_inc(&vp->vref_count)).

Can vp be concurrently removed from the list and freed by qla24xx_vport_delete(),
resulting in a use-after-free when the unreferenced vp is subsequently passed
to qla_update_host_map(vp, id)?

[ ... ]

[Severity: High]
This is also a pre-existing issue, but I see fcport accessed outside of its
protective lock in qla24xx_report_id_acquisition().

In the Format 1 path, sess_lock is held during lookup but dropped immediately:

	fcport = qla2x00_find_fcport_by_wwpn(vha,
	    rptid_entry->u.f1.port_name, 1);
	spin_unlock_irqrestore(&vha->hw->tgt.sess_lock, flags);

	if (fcport) {
		fcport->plogi_nack_done_deadline = jiffies + HZ;

Could a concurrent session deletion free this memory, causing a use-after-free
when fcport is modified?

Also, in the Format 2 path, the list is traversed locklessly:

	list_for_each_entry(fcport, &vha->vp_fcports, list) {
		fcport->scan_state = QLA_FCPORT_SCAN;
		fcport->n2n_flag = 0;
	}

	fcport = qla2x00_find_fcport_by_wwpn(vha,
	    rptid_entry->u.f2.port_name, 1);

Can this cause list corruption or a crash on stale pointers if other contexts
modify this list under sess_lock?

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