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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Reordering fields in `struct qla_bsg_auth_els_request` and `struct qla_bsg_auth_els_reply` breaks binary ABI for user-space tools and causes memory data aliasing corruption with the core BSG transport layer.
--

--- Patch [1]: [PATCH v3] scsi: qla2xxx: Fix flex array member not at end ---
commit 9255e4eda69e9adc31b6a8430b8f725b80a4ca2a
Author: Jesse Taube <[email protected]>

scsi: qla2xxx: Fix flex array member not at end

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`, put `struct fc_bsg_*` inside a
union with the trailing members padded to the size of `struct fc_bsg_*`
to silence the warning. This is similar to TRAILING_OVERLAP, exept
that the position of the flexible array member is also in a union,
causing it to not be the last member of the structure.

> diff --git a/drivers/scsi/qla2xxx/qla_edif_bsg.h b/drivers/scsi/qla2xxx/qla_edif_bsg.h
> index 514c265ba86e2..bcc35d149c1f1 100644
> --- a/drivers/scsi/qla2xxx/qla_edif_bsg.h
> +++ b/drivers/scsi/qla2xxx/qla_edif_bsg.h
> @@ -36,16 +36,16 @@ struct extra_auth_els {
>  } __packed;
>  
>  struct qla_bsg_auth_els_request {
> -	struct fc_bsg_request r;
>  	struct extra_auth_els e;
> +	struct fc_bsg_request r;

[Severity: High]
Does moving `struct fc_bsg_request r` to the end break the ABI and BSG
transport integration?

The generic BSG transport expects the first word of the request buffer to be
`msgcode`, defined in `struct fc_bsg_request`. By moving `r` down, `e` is now
at offset 0, causing `msgcode` to alias with `e.sub_cmd`.

For example, in drivers/scsi/qla2xxx/qla_edif.c:qla_bsg_check():

	struct qla_bsg_auth_els_request *req =
	    (struct qla_bsg_auth_els_request *)bsg_job->request;
...
	p = &req->e;

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

Will this cause `qla_bsg_check()` to read the generic BSG command code instead
of the intended `sub_cmd`, failing to parse valid commands?

>  };
>  
>  struct qla_bsg_auth_els_reply {
> -	struct fc_bsg_reply r;
>  	uint32_t rx_xchg_address;
>  	uint8_t version;
>  	uint8_t pad[VND_CMD_PAD_SIZE];
>  	uint8_t reserved[VND_CMD_APP_RESERVED_SIZE];
> +	struct fc_bsg_reply r;

[Severity: High]
Does reordering these fields cause memory data aliasing corruption with the
core BSG transport layer?

The BSG core expects `result` at the beginning of the reply buffer.
In drivers/scsi/qla2xxx/qla_edif.c:qla_pur_get_pending():

	rpl->rx_xchg_address = purex->pur_info.pur_rx_xchg_address;

	SET_DID_STATUS(rpl->r.result, DID_OK);

Writing to `rpl->rx_xchg_address` overwrites the first 4 bytes of the reply
buffer. Will the BSG core subsequently read this as the standard `result` status
and return a corrupted status code to user-space?

>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1