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

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

[netdev-bot+sashiko]

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

Critical: 0 · High: 1 · Medium: 0 (1) · Low: 1

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [High] fbnic_pkt_prepare()
  (drivers/net/ethernet/meta/fbnic/fbnic_txrx.c) still lets a device-…
- [Low] The commit message says the derived frame end and truesize are
  clamped "so a FIN descriptor cannot push them past the page".

Pre-existing issues:
- [Medium] In fbnic_add_rx_frag(), fbnic_page_pool_get_data() takes a
  fragment reference by decrementing rx_buf->pagecnt_bias (line 782).

--

eth: fbnic: validate the Rx completion offset and length

This commit adds checks on the header offset and length in
fbnic_pkt_prepare() and on the payload offset and length in
fbnic_add_rx_frag(). Completions that fall outside the 4K fragment are
dropped. It also clamps the derived header frame end and the payload
truesize to the fragment.

> 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.

[Severity: Low]

Is this description accurate? The FIN branches already produce values
inside the fragment:

	hdr_pg_end = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
		     FBNIC_BD_FRAG_SIZE : fbnic_hdr_pg_end(hdr_pg_off, len);

	truesize = FIELD_GET(FBNIC_RCD_AL_PAGE_FIN, rcd) ?
		   FBNIC_BD_FRAG_SIZE - pg_off : ALIGN(len, 128);

So both new min() calls change only non-FIN completions. The new in-code
comments describe this correctly, but the commit message says the
opposite.

The clamp also changes in-spec traffic. Take the 192/3584 example without
FIN. fbnic_hdr_pg_end() returns ALIGN(4288, 128) - 192 = 4160. Before this
patch, the shinfo sat at 3840..4160, past the fragment. After it,
frame_sz is 4096.

Could the commit message say that the clamp fixes an overrun on valid
non-FIN descriptors? It could also note that the clamp reduces frame_sz
and the charged truesize for them, since this is headed to stable.

> 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);

[ ... ]

> +	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);
>  

[ ... ]

> +	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);

[Severity: High]

Does the new upper bound in fbnic_pkt_prepare() leave room for the
skb_shared_info that napi_build_skb() puts at the end of the frame?

The check only rejects hdr_pg_off + len > FBNIC_BD_FRAG_SIZE. The frame
also has to hold FBNIC_RX_TROOM, which is
SKB_DATA_ALIGN(sizeof(struct skb_shared_info)), or 320 bytes on x86_64.

Take a non-FIN completion with hdr_pg_off = 4000 and len = 64. It passes
the check (4064 <= 4096), and then:

  fbnic_hdr_pg_end(4000, 64) = ALIGN(4576, 128) - 192 = 4416
  hdr_pg_end = min(4416, FBNIC_BD_FRAG_SIZE)        = 4096
  hdr_pg_start = 4000 - 192                         = 3808
  frame_sz = 4096 - 3808                            = 288

That frame_sz then reaches the core skb builder:

  fbnic_clean_rcq()
    fbnic_run_xdp()                (no program, or XDP_PASS)
      fbnic_build_skb()
        xdp_build_skb_from_buff()
          napi_build_skb(hard_start, 288)
            __finalize_skb_around()
              size -= SKB_DATA_ALIGN(sizeof(struct skb_shared_info));

Can this subtraction wrap to 0xFFFFFFE0? If so, skb_shinfo() points about
4 GiB past head, and the memset() of the shinfo writes there. That looks
like the same __build_skb_around() memset crash quoted in the commit
message.

Any hdr_pg_off above 3968 gives frame_sz below 320, with or without FIN.
FIN completions already had this before the patch. A non-FIN frame,
though, always had frame_sz >= len + 512. For these completions, the new
min() clamp turns a short overrun past the fragment into a write about
4 GiB away.

A second case is when frame_sz is large enough but the shinfo overlaps
packet data. With hdr_pg_off = 192 and len = 3900, the frame is clamped
to frame_sz = 4096, and the shinfo starts at fragment offset 3776, on top
of received bytes. napi_build_skb() zeroes it. The unchecked __skb_put()
in xdp_build_skb_from_buff() then leaves skb->tail (4092) past skb->end
(3776).

Wouldn't a later skb_put() on that skb hit skb_over_panic()?

With an XDP program attached, xdp_buff_add_frag() also writes nr_frags
and frags[] into those overlapping packet bytes. The program can read and
modify them. fbnic_put_pkt_buff() later trusts them for
page_pool_put_full_netmem().

The driver programs the device with MIN_TROOM = FBNIC_RX_TROOM in
fbnic_config_drop_mode_rcq(). FBNIC_HDS_THRESH_MAX is also defined as:

  (4096 - FBNIC_RX_HROOM - FBNIC_RX_TROOM - FBNIC_RX_PAD)

The commit message's maximum example (192 + 3584 = 3776) ends exactly at
4096 - FBNIC_RX_TROOM.

Should the check be something like this instead?

	hdr_pg_off + len + FBNIC_RX_TROOM > FBNIC_BD_FRAG_SIZE

That still accepts the 192/3584 case. It also keeps frame_sz >= len + 512
with no overlap. The last patch in the series leaves this check
unchanged.

> @@ -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);
>  

[ ... ]

> +	truesize = min(truesize, FBNIC_BD_FRAG_SIZE - pg_off);
> +
> +	netmem = fbnic_page_pool_get_data(qt, pg_idx);
> +

[Severity: Medium]

This isn't a bug introduced by this patch, but does fbnic_add_rx_frag()
leak the sub1 page reference taken here when xdp_buff_add_frag() fails?

fbnic_page_pool_get_data() decrements rx_buf->pagecnt_bias. Further down,
the failure path only marks the packet:

	added = xdp_buff_add_frag(&pkt->buff, netmem, pg_off, len, truesize);
	if (unlikely(!added)) {
		pkt->add_frag_failed = true;
		netdev_err_once(nv->napi.dev,
				"Failed to add fragment to xdp_buff\n");
	}

When the buffer already holds MAX_SKB_FRAGS fragments, the netmem never
reaches shinfo->frags[], so fbnic_put_pkt_buff() never puts it.
fbnic_page_pool_drain() then unrefs with the reduced bias, and the page
never goes back to the pool.

This patch only moves the fbnic_page_pool_get_data() call below the new
checks. A later patch in this series, "eth: fbnic: return the page
reference taken for a dropped Rx fragment", adds
qt->sub1.rx_buf[pg_idx].pagecnt_bias++ on this path, so the leak is gone
by the end of the series.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/cover.1791775409.git.yhlee%40isslab.korea.ac.kr

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

[Mohsin Bashir]

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.

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

[Yehyeong Lee]

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;
 
-- 
2.43.0