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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch enables reading Buffer-to-Buffer Credit (BBCR) data for 29xx adapters but fails to update the corresponding BSG handler `qla27xx_get_bbcr_data()`, preventing userspace retrieval.

Pre-existing issues:
- [High] Kernel stack memory leak to userspace due to uninitialized variable usage when `qla2x00_mailbox_command` fails.
--

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

scsi: qla2xxx: Enable get_adapter_id mailbox for 29xx

Add IS_QLA29XX() alongside the existing 27xx/28xx checks in
qla2x00_get_adapter_id() so that the additional mailbox
registers (buffer-to-buffer credit, SCM/EDC status) are read
on 29xx adapters.

> 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))
>  		mcp->in_mb |= MBX_15|MBX_21|MBX_22|MBX_23;

[Severity: Medium]
Will userspace actually be able to retrieve BBCR data for 29xx adapters?

Looking at qla27xx_get_bbcr_data() in drivers/scsi/qla2xxx/qla_bsg.c,
the BSG handler seems to explicitly reject anything other than 27xx and 28xx:

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

Could qla27xx_get_bbcr_data() also need an update to allow 29xx adapters so
userspace doesn't just receive an -EPERM error when requesting this data?

[ ... ]

> @@ -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)) {
>  			vha->bbcr = mcp->mb[15];
>  			if (mcp->mb[7] & SCM_EDC_ACC_RECEIVED) {

[Severity: High]
This is a pre-existing issue, but does qla2x00_get_adapter_id() leak
uninitialized kernel stack memory to userspace on error paths?

In qla2x00_get_adapter_id(), mc is allocated on the stack but not fully
initialized. If qla2x00_mailbox_command() fails, mcp->mb[1] remains
uninitialized, but it is unconditionally copied to the output pointer:

drivers/scsi/qla2xxx/qla_mbx.c:qla2x00_get_adapter_id() {
    mbx_cmd_t mc;
    ...
    rval = qla2x00_mailbox_command(vha, mcp);
    ...
    *id = mcp->mb[1];
    ...
}

When called from qla27xx_get_bbcr_data(), this uninitialized loop_id
(which comes from *id) is copied to userspace via bbcr.mbx1 when an
error is handled:

drivers/scsi/qla2xxx/qla_bsg.c:qla27xx_get_bbcr_data() {
    ...
    if (rval != QLA_SUCCESS) {
        bbcr.status = QLA_BBCR_STATUS_UNKNOWN;
        bbcr.state = QLA_BBCR_STATE_OFFLINE;
        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 initialized to zero, or should the output assignments be skipped
if qla2x00_mailbox_command() fails?

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