Re: [PATCH] scsi: bnx2fc: Use kmalloc_array() for array allocations

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Unchecked CQE frame length leads to integer underflow, small skb allocation, and out-of-bounds memset.
- [High] If `kmalloc_array()` fails in `bnx2fc_process_unsol_compl`, the function breaks out without updating the hardware receive queue (RQ) consumer index or returning the consumed RQEs, permanently desynchronizing the RQ from the completion queue (CQ).
- [High] Calling `bnx2fc_return_rqe()` with `num_rq > 1` corrupts the producer index due to a strict equality wrap check.
--

commit d860000c312968cdacef28865412092a3af9d66c
Author: Can Peng <pengcan@kylinos.cn>

scsi: bnx2fc: Use kmalloc_array() for array allocations

Documentation/process/deprecated.rst discourages open-coded arithmetic
in allocator arguments and recommends using the 2-factor allocator forms.

The unsolicited completion path allocates num_rq buffers of
BNX2FC_RQ_BUF_SZ bytes, and task_ctx_dma is an array of task_ctx_arr_sz
dma_addr_t entries. Use kmalloc_array() for both allocations.

Signed-off-by: Can Peng <pengcan@kylinos.cn>

> diff --git a/drivers/scsi/bnx2fc/bnx2fc_hwi.c b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> --- a/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> +++ b/drivers/scsi/bnx2fc/bnx2fc_hwi.c
> @@ -655,8 +655,8 @@ static void bnx2fc_process_unsol_compl(struct bnx2fc_rport *tgt, u16 wqe)
>  		if (rq_data) {
>  			buf = rq_data;
>  		} else {
> -			buf1 = buf = kmalloc((num_rq * BNX2FC_RQ_BUF_SZ),
> -					      GFP_ATOMIC);
> +			buf1 = buf = kmalloc_array(num_rq, BNX2FC_RQ_BUF_SZ,
> +						   GFP_ATOMIC);
>  
>  			if (!buf1) {
>  				BNX2FC_TGT_DBG(tgt, "Memory alloc failure\n");
>  				break;
>  			}

[Severity: High]
This isn't a bug introduced by this patch, but I noticed that if 
kmalloc_array fails here, the function breaks out of the loop without 
updating the hardware receive queue consumer index or returning the 
consumed RQEs.

Does this failure path permanently desynchronize the receive queue and the
completion queue? If the ring buffer wraps around and bnx2fc_get_next_rqe
returns NULL without advancing rq_cons_idx, breaking here seems to abandon
the completion queue entry while the outer loop advances cq_cons_idx.

[ ... ]

> 			for (i = 0; i < num_rq; i++) {
> 				spin_lock_bh(&tgt->tgt_lock);
> 				rq_data = (unsigned char *)
> 					   bnx2fc_get_next_rqe(tgt, 1);
> 				spin_unlock_bh(&tgt->tgt_lock);
> 				len = BNX2FC_RQ_BUF_SZ;
> 				memcpy(buf1, rq_data, len);
> 				buf1 += len;
> 			}
> 		}
> 		bnx2fc_process_l2_frame_compl(tgt, buf, frame_len,
> 					      FC_XID_UNKNOWN);

[Severity: Critical]
This is a pre-existing issue, but I am concerned about an integer underflow
occurring when bnx2fc_process_l2_frame_compl is called with frame_len.

In bnx2fc_process_l2_frame_compl():
    payload_len = frame_len - sizeof(struct fc_frame_header);
    fp = fc_frame_alloc(lport, payload_len);

If a remote attacker sends a malformed frame with a length less than 24
bytes, won't payload_len underflow?

Since payload_len is eventually passed to fc_frame_alloc_fill, it appears
alloc_skb_fclone truncates it to a 32-bit size, allocating a small buffer.
Could the subsequent memset then write past the small allocation using the
huge underflowed length, causing remote out-of-bounds memory corruption?

> 		if (buf != rq_data)
> 			kfree(buf);
> 		spin_lock_bh(&tgt->tgt_lock);
> 		bnx2fc_return_rqe(tgt, num_rq);

[Severity: High]
This is a pre-existing issue, but calling bnx2fc_return_rqe here with a
num_rq greater than 1 looks like it could corrupt the producer index.

In bnx2fc_return_rqe():
    if ((next_prod_idx & 0x7fff) == BNX2FC_RQ_WQES_MAX) {
        /* Wrap around RQ */
        next_prod_idx += 0x8000 - BNX2FC_RQ_WQES_MAX;
    }

If a frame spans multiple buffers, could incrementing by num_rq cause the
index to skip exactly over BNX2FC_RQ_WQES_MAX? If it does, the strict
equality check would fail to wrap the index, pushing it out of bounds and
writing an invalid index to the hardware doorbell.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623112428.98097-1-pengcan@kylinos.cn?part=1