Re: [PATCH v5 16/56] scsi: qla2xxx: Enable get_adapter_id mailbox for 29xx

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Missing QLA29XX support in BSG command handler prevents BBCR feature access.

Pre-existing issues:
- [High] Uninitialized kernel stack memory leak to userspace in `qla27xx_get_bbcr_data()`.
- [High] Race condition due to unprotected read-modify-write on shared `vha->hw->flags` bitfield.
--

--- Patch [16]: [PATCH v5 16/56] scsi: qla2xxx: Enable get_adapter_id mailbox for 29xx ---
commit 6a9facae245c0e3d5c4d496991f281dbefb90de2
Author: Nilesh Javali <njavali@marvell.com>

scsi: qla2xxx: Enable get_adapter_id mailbox for 29xx

This patch adds the IS_QLA29XX() check to qla2x00_get_adapter_id() alongside
the existing 27xx/28xx logic. This allows 29xx adapters to read additional
mailbox registers for buffer-to-buffer credits and SCM/EDC status.

> diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c
> index 52d70b61654c7..3fc08120fdf11 100644
> --- a/drivers/scsi/qla2xxx/qla_mbx.c
> +++ b/drivers/scsi/qla2xxx/qla_mbx.c
> @@ -1772,7 +1772,7 @@ qla2x00_get_adapter_id(scsi_qla_host_t *vha, uint16_t *id, uint8_t *al_pa,
>  		mcp->in_mb |= MBX_13|MBX_12|MBX_11|MBX_10;
>  	if (IS_FWI2_CAPABLE(vha->hw))
>  		mcp->in_mb |= MBX_19|MBX_18|MBX_17|MBX_16;
> -	if (IS_QLA27XX(vha->hw) || IS_QLA28XX(vha->hw))
> +	if (IS_QLA27XX(vha->hw) || IS_QLA28XX(vha->hw) || IS_QLA29XX(vha->hw))

[Severity: High]
This is a pre-existing issue, but does this function leak uninitialized kernel
stack memory to userspace?

In qla2x00_get_adapter_id(), the mailbox command structure is allocated on
the stack without initialization:

	mbx_cmd_t mc;
	mbx_cmd_t *mcp = &mc;

If qla2x00_mailbox_command() fails early (e.g., due to timeout or reset),
the mc.mb array isn't populated from the hardware registers. However, the
function still unconditionally assigns the uninitialized stack data to the
id pointer:

	/* Return data. */
	*id = mcp->mb[1];

When called from the BSG handler, this uninitialized data is saved to
bbcr.mbx1 in the error path and then copied to userspace:

drivers/scsi/qla2xxx/qla_bsg.c:qla27xx_get_bbcr_data() {
...
	rval = qla2x00_get_adapter_id(vha, &loop_id, &al_pa, &area,
...
	if (rval != QLA_SUCCESS) {
		bbcr.status = QLA_ST_WWN_MN_FAIL;
		bbcr.mbx1 = loop_id;
		goto done;
	}
...
done:
	sg_copy_from_buffer(bsg_job->reply_payload.sg_list,
		bsg_job->reply_payload.sg_cnt, &bbcr, sizeof(bbcr));
...
}

Should mc be zero-initialized to prevent leaking stack data?

>  		mcp->in_mb |= MBX_15|MBX_21|MBX_22|MBX_23;
>  
>  	mcp->tov = MBX_TOV_SECONDS;

[ ... ]

> @@ -1827,7 +1827,7 @@ qla2x00_get_adapter_id(scsi_qla_host_t *vha, uint16_t *id, uint8_t *al_pa,
>  			}
>  		}
>  
> -		if (IS_QLA27XX(vha->hw) || IS_QLA28XX(vha->hw)) {
> +		if (IS_QLA27XX(vha->hw) || IS_QLA28XX(vha->hw) || IS_QLA29XX(vha->hw)) {

[Severity: Medium]
Will this BBCR data actually be accessible for 29xx adapters from userspace?

While this patch adds support for reading the BBCR and SCM/EDC data for 29xx
adapters, the BSG command handler used to retrieve this data appears to lack
the corresponding IS_QLA29XX() check:

drivers/scsi/qla2xxx/qla_bsg.c:qla27xx_get_bbcr_data() {
...
	if (!IS_QLA27XX(ha) && !IS_QLA28XX(ha))
		return -EPERM;
...
}

Does qla27xx_get_bbcr_data() also need to be updated to include 29xx adapters
so the feature is fully reachable?

>  			vha->bbcr = mcp->mb[15];
>  			if (mcp->mb[7] & SCM_EDC_ACC_RECEIVED) {
>  				ql_log(ql_log_info, vha, 0x11a4,

[Severity: High]
This isn't a bug introduced by this patch, but does the following SCM logic
have a race condition?

Just below this log statement in the same block, there is an unprotected
read-modify-write on a shared bitfield:

			if (mcp->mb[7] & SCM_RDF_ACC_RECEIVED) {
				vha->hw->flags.scm_enabled = 1;

The flags member is a volatile struct containing bitfields like
scm_enabled:1, lip_ae:1, and n2n_ae:1. Because this modification occurs
in process context without holding a hardware spinlock, it performs a
non-atomic read-modify-write on the shared 32-bit word.

If an asynchronous hardware event fires during this operation, the ISR
might set a bit like lip_ae = 1. The process context would then overwrite
the change with stale data, silently dropping the ISR's modification.

Could this lead to dropped loop initialization events?

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