Re: [PATCH v5 2/3] scsi: bfa: drop unsolicited frames shorter than the headers they must hold

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: bfa: OOB read of ELS command code due to missing payload bounds check
--

commit 31f50cb105260d95a224555b9e6f894b5a59692b
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: bfa: drop unsolicited frames shorter than the headers they must hold

bfa_fcs_uf_recv() keeps the received frame length in a u16 and hands it,
with a pointer to the frame, to every unsolicited frame consumer. Two
lengths are never checked.

> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
> index 9b57312f43f50..8bb92f935a40f 100644
> --- a/drivers/scsi/bfa/bfa_fcs.c
> +++ b/drivers/scsi/bfa/bfa_fcs.c
> @@ -1527,6 +1527,14 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
>  	struct fc_vft_s *vft;
>  	struct bfa_fcs_fabric_s *fabric;
>  
> +	/*
> +	 * every consumer reads the FC header, so the frame has to hold one
> +	 */
> +	if (len < sizeof(struct fchs_s)) {
> +		bfa_uf_free(uf);
> +		return;
> +	}
> +

[Severity: High]
Does this check need to account for the payload length when processing ELS
commands?

If a remote device sends an unsolicited FC frame of exactly 24 bytes with
fchs->type == FC_TYPE_ELS, can this cause a regression where the driver reads
beyond the verified length?

For example, in drivers/scsi/bfa/bfa_fcs.c:bfa_fcs_fabric_uf_recv():

    struct fc_els_cmd_s *els_cmd = (struct fc_els_cmd_s *) (fchs + 1);
    ...
    if ((pid == bfa_ntoh3b(FC_FABRIC_PORT)) &&
        (els_cmd->els_code == FC_ELS_FLOGI) && ...

And similarly in drivers/scsi/bfa/bfa_fcs_lport.c:bfa_fcs_lport_uf_recv():

    struct fc_els_cmd_s *els_cmd = (struct fc_els_cmd_s *) (fchs + 1);
    ...
    if ((fchs->type == FC_TYPE_ELS) &&
        (els_cmd->els_code == FC_ELS_PLOGI)) {

Because the driver recycles DMA buffers without zeroing them, the hardware DMA
writes only 24 bytes, leaving the old payload intact in the buffer. Could the
driver read the stale els_code and process the stale payload as a new command?

Should there be an additional length verification before accessing
els_cmd->els_code?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016155441.1607196-1-yhlee@isslab.korea.ac.kr?part=2