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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Accessing hardware structures via C bitfields defined on `__le16` causes silent data corruption on big-endian architectures.

Pre-existing issues:
- [High] Use-after-free of `vp` pointer due to unlocked access without reference pinning.
--

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

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

This commit adds reserved_end padding to the extended VP report ID entry
structure to match the hardware IOCB stride. It also updates
qla24xx_report_id_acquisition() to handle extracting vp_idx and vp_status
from the larger IOCB format on 29xx series adapters.

> 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

[ ... ]

> @@ -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 accessing hardware structures via C bitfields defined on __le16
cause silent data corruption on big-endian architectures?

If the hardware writes this DMA data in little-endian format, reading it
natively without le16_to_cpu() could result in byte-swapped data being
loaded into registers on big-endian systems. The compiler would then extract
bits from this mangled word, which might lead to corrupted values for vport
indexing and state transitions.

Should we use le16_to_cpu() here before extracting the bitfields?

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

[ ... ]

> @@ -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;
>  				}
>  			}
>  			spin_unlock_irqrestore(&ha->vport_slock, flags);

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free
of the vp pointer after we release the lock?

The code iterates over ha->vp_list while holding ha->vport_slock to find a
matching vp. Once found, it breaks the loop, releases the spinlock, and
subsequently accesses the vp pointer (e.g., calling qla_update_host_map()
and setting flags further down in qla24xx_report_id_acquisition()).

Since no reference count is taken while the lock is held, could a
concurrent vport deletion remove the vp from the list and free it while
this function is still using it?

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