Re: [PATCH] scsi: qla2xxx: Rework BUILD_BUG_ON() assertion

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: qla2xxx: FORTIFY_SOURCE field-spanning write warning in qlt_queue_unknown_atio
- [High] scsi: qla2xxx: UBSAN/FORTIFY overflow in add_cdb array accesses
--

commit 4d96d5f0f41af34ce19bfbd80ce5e059ab28cd98
Author: Finn Thain <fthain@linux-m68k.org>

scsi: qla2xxx: Rework BUILD_BUG_ON() assertion

The LKP bot reported a build failure with CONFIG_COLDFIRE=y together with
CONFIG_SCSI_QLA_FC=y. The failure is caused by an incorrect BUILD_BUG_ON()
assertion in qlt_queue_unknown_atio(). To resolve this, a flex array is
placed at the end of struct qla_tgt_sess_op.

Closes: https://lore.kernel.org/oe-kbuild-all/202603030747.VX0v4otS-lkp@intel.com/
Closes: https://lore.kernel.org/oe-kbuild-all/202609301230.CauIUXCF-lkp@intel.com/

> diff --git a/drivers/scsi/qla2xxx/qla_target.h b/drivers/scsi/qla2xxx/qla_target.h
> index 71bc1affb1bc9..b27bd3aecbf8e 100644
> --- a/drivers/scsi/qla2xxx/qla_target.h
> +++ b/drivers/scsi/qla2xxx/qla_target.h
> @@ -309,7 +309,8 @@ struct atio7_fcp_cmnd {
>  	/*
>  	 * add_cdb is optional and can absent from struct atio7_fcp_cmnd. Size 4
>  	 * only to make sizeof(struct atio7_fcp_cmnd) be as expected by
> -	 * BUILD_BUG_ON in qlt_init().
> +	 * BUILD_BUG_ON in tcm_qla2xxx_init(). See also, BUILD_BUG_ON in
> +	 * qlt_queue_unknown_atio().
>  	 */
>  	uint8_t  add_cdb[4];

[Severity: High]
Since add_cdb is explicitly declared as a fixed-size 4-byte array here, does
the current code cause UBSAN array bounds violations and FORTIFY_SOURCE read
overflow panics when accessing elements beyond these bounds?

For instance, in memcpy_atio() when len > 0, it accesses
&src->u.isp24.fcp_cmnd.add_cdb[len * 4] which is out-of-bounds, and reading
4 bytes triggers the overflow panic:

drivers/scsi/qla2xxx/qla_target.c:memcpy_atio() {
...
    memcpy(&dst->u.isp24.fcp_cmnd.add_cdb[0],
           &src->u.isp24.fcp_cmnd.add_cdb[len * 4],
           4);
...
}

Similarly, get_datalen_for_atio() accesses the array out-of-bounds when
len > 0:

drivers/scsi/qla2xxx/qla_target.h:get_datalen_for_atio() {
...
    return get_unaligned_be32(&atio->u.isp24.fcp_cmnd.add_cdb[len * 4]);
...
}

[ ... ]

> @@ -845,7 +846,11 @@ struct qla_tgt_sess_op {
>  	struct rsp_que *rsp;
>  
>  	struct atio_from_isp atio;
> -	/* DO NOT ADD ANYTHING ELSE HERE - atio must be last member */
> +	/*
> +	 * DO NOT ADD ANYTHING ELSE HERE.
> +	 * atio.u.isp24.fcp_cmnd.add_cdb may extend past end of atio.
> +	 */
> +	uint8_t atio_u_isp24_fcp_cmnd_add_cdb[];
>  };

[Severity: High]
Since this patch introduces the flexible array atio_u_isp24_fcp_cmnd_add_cdb[]
to legally hold the trailing data, shouldn't the memcpy() in
qlt_queue_unknown_atio() be updated to target it?

drivers/scsi/qla2xxx/qla_target.c:qlt_queue_unknown_atio() {
...
    u = kzalloc(sizeof(*u) + add_cdb_len, GFP_ATOMIC);
    ...
    memcpy(&u->atio, atio, sizeof(*atio) + add_cdb_len);
...
}

Because the destination is typed strictly as &u->atio, copying more than
64 bytes (sizeof(*atio)) when add_cdb_len > 0 causes a FORTIFY_SOURCE
intra-object write overflow panic.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/4808a5dc1a71528ffa3360868561009744277450.1791502886.git.fthain@linux-m68k.org?part=1