Re: [PATCH v2] scsi: qla2xxx: Fix flex array member not at end

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The commit message incorrectly claims that structs were moved to the end of the parent structures, while the code actually uses a union trick to preserve the existing layout.

Pre-existing issues:
- [High] Heap out-of-bounds read due to missing request_len validation in BSG handlers.
--

--- Patch [1]: [PATCH v2] scsi: qla2xxx: Fix flex array member not at end ---
commit 71cbbca6ae587defc94dd45238daf79e9aba8d13
Author: Jesse Taube <jtaubepe@redhat.com>

scsi: qla2xxx: Fix flex array member not at end

This commit addresses the flex array member warning for struct fc_bsg_reply
and struct fc_bsg_request in qla_edif_bsg.h. It updates the parent structs
qla_bsg_auth_els_request and qla_bsg_auth_els_reply to wrap the flexible
array structures within unions.

> In qla_edif_bsg.h: struct fc_bsg_reply and struct fc_bsg_request
> have flexible array members, thus they must be the last member of
> the parent structure. Contininging in the effort to add
> -Wflex-array-member-not-at-end, move the structs to the end of
> the parent structures, struct qla_bsg_auth_els_reply and
> struct qla_bsg_auth_els_request .

[Severity: Low]
This isn't a bug, but the commit message states the structs are moved to
the end of the parent structures, while the code actually uses a union
trick with an anonymous struct padding to preserve the existing binary
layout. Could the message be updated to more accurately reflect the use of
the union rather than a structural move?

> diff --git a/drivers/scsi/qla2xxx/qla_edif_bsg.h b/drivers/scsi/qla2xxx/qla_edif_bsg.h
> index 514c265ba86e2..2b32a31856292 100644
> --- a/drivers/scsi/qla2xxx/qla_edif_bsg.h
> +++ b/drivers/scsi/qla2xxx/qla_edif_bsg.h
> @@ -36,16 +36,26 @@ struct extra_auth_els {
>  } __packed;
>  
>  struct qla_bsg_auth_els_request {
> -	struct fc_bsg_request r;
> -	struct extra_auth_els e;
> +	union {
> +		struct fc_bsg_request r;
> +		struct {
> +			unsigned char __fc_bsg_request_sz[sizeof(struct fc_bsg_request)];
> +			struct extra_auth_els e;
> +		};
> +	};
>  };

[Severity: High]
This is a pre-existing issue, but while looking at these structures, is
there a missing bounds check on the request length when processing BSG
messages?

If a user issues an SG_IO ioctl to a BSG device with a minimal request_len
of 4 bytes but specifies a command requiring more data, bsg-lib allocates
exactly request_len bytes.

The generic FC transport increments cmdlen but reads vendor_id without
verifying the bounds:

drivers/scsi/scsi_transport_fc.c:fc_bsg_host_dispatch() {
    ...
    case FC_BSG_HST_VENDOR:
        cmdlen += sizeof(struct fc_bsg_host_vendor);
        if ((shost->hostt->vendor_id == 0L) ||
            (bsg_request->rqst_data.h_vendor.vendor_id !=
             shost->hostt->vendor_id)) {
    ...
}

The qla2xxx driver also unconditionally reads req->e.sub_cmd from this
request structure at offset 20:

drivers/scsi/qla2xxx/qla_edif.c:qla_bsg_check() {
    ...
    p = &req->e;

    /* Get response */
    if (p->sub_cmd == PULL_ELS) {
    ...
}

And unconditionally reads vendor_cmd[0] at offset 12:

drivers/scsi/qla2xxx/qla_bsg.c:qla2x00_process_vendor_specific() {
    ...
    ql_dbg(ql_dbg_edif, vha, 0x911b, "%s FC_BSG_HST_VENDOR cmd[0]=0x%x\n",
        __func__, bsg_request->rqst_data.h_vendor.vendor_cmd[0]);
    ...
}

Could this lead to a heap out-of-bounds read if request_len is too small?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714185621.610105-1-jtaubepe@redhat.com?part=1