Re: [PATCH net v3 2/5] eth: fbnic: validate the Rx completion offset and length

[Mohsin Bashir <mohsin.bashr@gmail.com>]

On 10/11/26 9:15 PM, Yehyeong Lee wrote:
> fbnic_pkt_prepare() and fbnic_add_rx_frag() take the offset and length
> from the completion descriptor (FBNIC_RCD_AL_BUFF_OFF_MASK and _LEN_MASK)
> and use them to place data in the fragment page without checking them. A
> device that reports an offset below the reserved headroom, or a length
> that runs past the 4K fragment, produces a negative headroom and a frame
> that extends beyond the page, which is then read out past the page into
> adjacent memory.
> 
> Reject a header whose offset is below FBNIC_RX_HROOM or whose offset plus
> length exceeds the fragment, and a payload whose offset plus length
> exceeds the fragment, via the drop path. Clamp the derived frame end and
> truesize to the fragment so a FIN descriptor cannot push them past the
> page. A maximum-size header (offset 192, length 3584) stays inside the
> fragment and is still accepted.
> 
>    BUG: unable to handle page fault for address: ffffed1021a8ee10
>    #PF: supervisor read access in kernel mode
>    #PF: error_code(0x0000) - not-present page
>    Oops: Oops: 0000 [#1] SMP KASAN NOPTI
>    RIP: 0010:kasan_check_range+0x15e/0x1b0
>    Call Trace:
>     <TASK>
>     __asan_memset+0x23/0x50
>     __build_skb_around+0x222/0x320
>     napi_build_skb+0x50/0x1e0
>     xdp_build_skb_from_buff+0xda/0x8b0
>     fbnic_run_xdp+0x29a/0x1040 [fbnic]
> 
> An offset and length that keep the frame inside the page but past the
> valid data produce no splat: the read lands in adjacent page_pool memory,
> which carries no KASAN redzone, and is copied into the skb silently.
> 
> Fixes: a29b8eb6e533 ("eth: fbnic: Add basic Rx handling")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
>   drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 41 ++++++++++++++++++--
>   1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 305fe6d2f511f..7dda59bba14ee 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -1037,13 +1037,34 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>   		return;
>   	}
>   
> -	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
> +	/* The completion describes one frame inside the one
> +	 * FBNIC_BD_FRAG_SIZE fragment it names.  fbnic_hdr_pg_start() takes
> +	 * the headroom back out of the offset, so an offset below the
> +	 * headroom underflows it, and the frame data runs to
> +	 * hdr_pg_off + len, which has to stay inside the fragment.
> +	 */
> +	if (unlikely(hdr_pg_off < FBNIC_RX_HROOM ||
> +		     hdr_pg_off + len > FBNIC_BD_FRAG_SIZE)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
>   
>   	/* Short-cut the end calculation if we know page is fully consumed */
>   	hdr_pg_end = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
>   		     FBNIC_BD_FRAG_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);
>   	hdr_pg_start = fbnic_hdr_pg_start(hdr_pg_off);
>   
> +	/* fbnic_hdr_pg_end() rounds the frame up to where the next header
> +	 * would start, which can be past the end of the fragment when this
> +	 * frame is the last one in it.  napi_build_skb() puts the shared
> +	 * info at the end of the frame, so hold the frame inside the
> +	 * fragment, which is what the FBNIC_RCD_AL_PAGE_FIN case above
> +	 * already does.
> +	 */
> +	hdr_pg_end = min(hdr_pg_end, FBNIC_BD_FRAG_SIZE);
> +
> +	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
> +
>   	headroom = hdr_pg_off - hdr_pg_start + FBNIC_RX_PAD;
>   	frame_sz = hdr_pg_end - hdr_pg_start;
>   	xdp_init_buff(&pkt->buff, frame_sz, &qt->xdp_rxq);
> @@ -1091,11 +1112,25 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>   		return;
>   	}
>   
> -	netmem = fbnic_page_pool_get_data(qt, pg_idx);
> -
>   	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
>   		   FBNIC_BD_FRAG_SIZE - pg_off : ALIGN(len, 128);
>   
> +	/* The data has to stay inside the FBNIC_BD_FRAG_SIZE fragment the
> +	 * completion names.
> +	 */
> +	if (unlikely(pg_off + len > FBNIC_BD_FRAG_SIZE)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
> +	/* ALIGN() can round the charged size past the end of the fragment
> +	 * when this is the last fragment in it, so hold it inside, which is
> +	 * what the FBNIC_RCD_AL_PAGE_FIN case above already does.
> +	 */
> +	truesize = min(truesize, FBNIC_BD_FRAG_SIZE - pg_off);
> +
> +	netmem = fbnic_page_pool_get_data(qt, pg_idx);
> +
>   	pg_off += (FBNIC_RCD_AL_BUFF_FRAG_MASK & rcd) *
>   		  FBNIC_BD_FRAG_SIZE;
>   

The offset field is 12 bits, but that alone does not bound offset + 
length. The relevant guarantee comes from the device’s placement 
behavior. The driver configures RX_HROOM, RX_TROOM, and the max header 
length, and the receive path expects the device to honor those 
constraints. This patch assumes that the device violates those 
programmed placement constraints. I doubt this can happen on real 
healthy HW.