Re: [PATCH v4 1/3] scsi: bfa: clamp the received unsolicited frame length to the buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] scsi: bfa: integer underflow causes massive OOB DMA read in bfa_fcs_lport_echo
- [High] scsi: bfa: missing bounds check on firmware-supplied buf_tag in uf_recv()
--

commit 4545309c07b7ce26f9ea539ffa38c0ea8612717f
Author: Yehyeong Lee <yhlee@isslab.korea.ac.kr>

scsi: bfa: clamp the received unsolicited frame length to the buffer

The commit addresses an issue where uf_recv() blindly accepts the
unsolicited frame length from the firmware. By capping uf->data_len to
the size of the posted buffer, it prevents out-of-bounds writes in
downstream consumers.

> diff --git a/drivers/scsi/bfa/bfa_svc.c b/drivers/scsi/bfa/bfa_svc.c
> index df33afaaa6735..1ccf2e994a671 100644
> --- a/drivers/scsi/bfa/bfa_svc.c
> +++ b/drivers/scsi/bfa/bfa_svc.c
> @@ -5453,7 +5453,7 @@ uf_recv(struct bfa_s *bfa, struct bfi_uf_frm_rcvd_s *m)

[Severity: High]
Since this patch addresses untrusted firmware input by clamping m->xfr_len,
is it safe to use m->buf_tag as a direct array index?

Earlier in this same function:

drivers/scsi/bfa/bfa_svc.c:uf_recv() {
    ...
	u16 uf_tag = m->buf_tag;
	struct bfa_uf_s *uf = &ufm->uf_list[uf_tag];
    ...
	uf_buf = (struct bfa_uf_buf_s *)
			bfa_mem_get_dmabuf_kva(ufm, uf_tag, uf->pb_len);
    ...
}

If a buggy or malicious firmware provides an out-of-bounds buf_tag, won't this
lead to out-of-bounds array accesses to ufm->uf_list and out-of-bounds DMA
segment calculations?

>  	list_del(&uf->qe);	/* dequeue from posted queue */
>  
>  	uf->data_ptr = buf;
> -	uf->data_len = m->xfr_len;
> +	uf->data_len = min_t(u16, m->xfr_len, sizeof(struct bfa_uf_buf_s));
>  
>  	WARN_ON(uf->data_len < sizeof(struct fchs_s));
>  

[Severity: High]
If the received frame is truncated and m->xfr_len is smaller than
sizeof(struct fchs_s) (24 bytes), this warns but still processes the frame.

Will this lead to an integer underflow later? In bfa_fcs_lport_echo(),
pyld_len is calculated as:

drivers/scsi/bfa/bfa_fcs_lport.c:bfa_fcs_lport_echo() {
    ...
    pyld_len = rx_len - sizeof(struct fchs_s);
    ...
}

If rx_len is less than 24, pyld_len underflows. Since pyld_len is an int,
this evaluates to a negative number, bypassing the subsequent check:

    if (pyld_len > len)

The negative pyld_len is then passed to bfa_fcxp_send(), which takes a u32
reqlen. Does this implicit cast to a massive length (~4.29GB) result in a huge
out-of-bounds DMA read when submitted to the firmware?

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