Re: [PATCH v4 2/3] scsi: bfa: drop unsolicited frames shorter than their VFT header

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] scsi: bfa: Incomplete bounds checking in bfa_fcs_uf_recv leads to out-of-bounds reads
--

commit 5c4d11be4a4f404e42897f255d18569a1acd5b93
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: bfa: drop unsolicited frames shorter than their VFT header

This patch fixes an integer underflow by checking that the received frame
length is at least the size of the virtual fabric tagging (VFT) header
before attempting to step over it in bfa_fcs_uf_recv().

> diff --git a/drivers/scsi/bfa/bfa_fcs.c b/drivers/scsi/bfa/bfa_fcs.c
> index 9b57312f43f50..711d2ed83a7aa 100644
> --- a/drivers/scsi/bfa/bfa_fcs.c
> +++ b/drivers/scsi/bfa/bfa_fcs.c
> @@ -1552,6 +1552,11 @@ bfa_fcs_uf_recv(void *cbarg, struct bfa_uf_s *uf)
>  		/*
>  		 * skip vft header
>  		 */
> +		if (len < sizeof(struct fc_vft_s)) {
> +			bfa_uf_free(uf);
> +			return;
> +		}
> +

[Severity: High]
Does this check occur too late to prevent out-of-bounds reads?

Before reaching this new bounds check, the function already accesses fields
from the frame buffer to check for the VFT header:

	if (fchs->routing == FC_RTG_EXT_HDR &&
	    fchs->cat_info == FC_CAT_VFT_HDR) {
		bfa_stats(fcs, uf.tagged);
		vft = bfa_uf_get_frmbuf(uf);
		if (fcs->port_vfid == vft->vf_id)

If the frame is shorter than 8 bytes, could these accesses read uninitialized
or stale DMA memory?

>  		fchs = (struct fchs_s *) (vft + 1);
>  		len -= sizeof(struct fc_vft_s);
>  

Does this code need to verify that the remaining frame length can hold the
mandatory 24-byte inner fchs_s header?

Without checking that the remaining len is at least sizeof(struct fchs_s),
truncated frames are passed to downstream handlers. For example, in
bfa_fcs_fabric_uf_recv(), the pointer is unconditionally cast and accessed:

	u32 pid = fchs->d_id;
	struct fc_els_cmd_s *els_cmd = (struct fc_els_cmd_s *) (fchs + 1);

Could this process stale ghost data as valid protocol commands and confuse the
state machine?

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