[PATCH net v3 0/5] eth: fbnic: harden Rx and Tx completion handling

[PATCH net v3 0/5] eth: fbnic: harden Rx and Tx completion handling

[Yehyeong Lee]

The Rx and Tx completion descriptors carry buffer ids, offsets and
lengths that the device writes and the driver consumes without
validating. v2 bounded the Rx buffer index against the array only, which
the review found insufficient: an in-range id naming an unposted slot
still reaches a NULL page, a bare META completion still builds an skb from
a NULL buffer, and the offset, length and the Tx completion head are not
checked at all.

This series validates each of these against what was actually posted and
takes the existing drop path, and bounds the XDP Tx completion loop so a
head outside the ring cannot spin it.

Patch 1 validates the Rx buffer id at every use and drops a bare META
completion. Patch 2 validates the offset and length. Patch 3 returns a
pending packet before a new header reuses its slot. Patch 4 is unchanged
from v2 and returns the page reference for a dropped fragment. Patch 5
bounds the Tx completion head.

v2: https://lore.kernel.org/netdev/cover.1791659860.git.yhlee@isslab.korea.ac.kr/

Yehyeong Lee (5):
  eth: fbnic: only act on Rx completions naming a posted buffer
  eth: fbnic: validate the Rx completion offset and length
  eth: fbnic: return a pending Rx packet before reusing its buffer
  eth: fbnic: return the page reference taken for a dropped Rx fragment
  eth: fbnic: bound the XDP Tx completion head to the ring

 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 192 +++++++++++++++----
 1 file changed, 158 insertions(+), 34 deletions(-)

-- 
2.43.0

[PATCH net v3 1/5] eth: fbnic: only act on Rx completions naming a posted buffer

[Yehyeong Lee]

An Rx completion descriptor carries a 16-bit buffer id that the driver
uses to index the rx_buf[] array (FBNIC_RCD_AL_BUFF_PAGE_MASK). The fill
path masks the id to the ring size, but the completion path does not, so
the device can hand back an id that is past the array or that names a
slot which was never posted. An unposted in-range slot holds netmem 0,
and netmem_to_page(0) returns NULL, which fbnic_pkt_prepare() and
fbnic_add_rx_frag() then dereference.

Masking the id would not fix this: it aliases an out-of-range id onto a
posted buffer and corrupts that buffer's page_pool reference count.
Reject a completion whose id is outside the array or outside the posted
window and take the existing drop path.

The same id is used as the BDQ head in fbnic_clean_bdq(), where an
out-of-range value spins the cleanup loop; bound it there with the same
test. Validate head0/head1 in fbnic_clean_rcq() before they are assigned,
and drop a META completion that arrives with no header buffer rather than
building an skb around a NULL pointer.

  Oops: general protection fault, probably for non-canonical address
  0xdffffc0000000004: 0000 [#1] SMP KASAN NOPTI
  KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
  RIP: 0010:fbnic_pkt_prepare+0x1a2/0x5a0 [fbnic]
  Call Trace:
   <TASK>
   fbnic_clean_rcq+0x1ba/0x1350 [fbnic]

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 | 87 ++++++++++++++++++--
 1 file changed, 82 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 10caacffee0f0..305fe6d2f511f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -797,6 +797,34 @@ static void fbnic_page_pool_drain(struct fbnic_ring *ring, unsigned int idx,
 	rx_buf->netmem = 0;
 }
 
+/* The device names an Rx buffer by its index in the buffer descriptor
+ * ring.  A completion may only name a buffer we posted, that is one in
+ * [head, tail), so test the index against the array and against the
+ * posted window.  The window test is not enough on its own: it masks the
+ * index, so an index past the end of the array can fold back into the
+ * window and run off rx_buf[].
+ */
+static bool fbnic_rx_buf_posted(struct fbnic_ring *ring, unsigned int idx)
+{
+	if (idx > ring->size_mask)
+		return false;
+
+	return ((idx - ring->head) & ring->size_mask) < fbnic_desc_used(ring);
+}
+
+/* fbnic_clean_bdq() is given a position to consume up to rather than a
+ * buffer to use, so unlike fbnic_rx_buf_posted() it also accepts tail,
+ * which is how the teardown path asks for the whole ring to be drained.
+ */
+static bool fbnic_bdq_head_valid(struct fbnic_ring *ring, unsigned int hw_head)
+{
+	if (hw_head > ring->size_mask)
+		return false;
+
+	return ((hw_head - ring->head) & ring->size_mask) <=
+	       fbnic_desc_used(ring);
+}
+
 static void fbnic_clean_twq(struct fbnic_napi_vector *nv, int napi_budget,
 			    struct fbnic_q_triad *qt, s32 ts_head, s32 head0,
 			    s32 head1)
@@ -882,6 +910,13 @@ static void fbnic_clean_bdq(struct fbnic_ring *ring, unsigned int hw_head,
 {
 	unsigned int head = ring->head;
 
+	/* A head we never posted cannot name a buffer we own, and the loop
+	 * below masks head to the ring so it would never reach it.  Leave
+	 * the ring alone and wait for a head we can act on.
+	 */
+	if (unlikely(!fbnic_bdq_head_valid(ring, hw_head)))
+		return;
+
 	if (head == hw_head)
 		return;
 
@@ -985,14 +1020,25 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 {
 	unsigned int hdr_pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
 	unsigned int hdr_pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
-	struct page *page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
 	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
 	unsigned int frame_sz, hdr_pg_start, hdr_pg_end, headroom;
 	unsigned char *hdr_start;
+	struct page *page;
 
 	/* data_hard_start should always be NULL when this is called */
 	WARN_ON_ONCE(pkt->buff.data_hard_start);
 
+	/* A buffer ID we never posted cannot name a buffer we own, so drop
+	 * the packet instead of running off the end of rx_buf[] or picking
+	 * up a slot we have already drained.
+	 */
+	if (unlikely(!fbnic_rx_buf_posted(&qt->sub0, hdr_pg_idx))) {
+		pkt->add_frag_failed = true;
+		return;
+	}
+
+	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
+
 	/* 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);
@@ -1026,10 +1072,27 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 	unsigned int pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
 	unsigned int pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
 	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
-	netmem_ref netmem = fbnic_page_pool_get_data(qt, pg_idx);
 	unsigned int truesize;
+	netmem_ref netmem;
 	bool added;
 
+	/* No header completion has set buff up, either because one was
+	 * rejected or because the device sent none, so there is nothing to
+	 * add a fragment to.  Mark the packet so the metadata descriptor
+	 * drops it instead of building an skb on an empty buffer.
+	 */
+	if (unlikely(!pkt->buff.data_hard_start)) {
+		pkt->add_frag_failed = true;
+		return;
+	}
+
+	if (unlikely(!fbnic_rx_buf_posted(&qt->sub1, pg_idx))) {
+		pkt->add_frag_failed = true;
+		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);
 
@@ -1270,6 +1333,7 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
 	while (likely(packets < budget)) {
 		struct sk_buff *skb = ERR_PTR(-EINVAL);
 		u32 pkt_bytes;
+		u32 pg_idx;
 		u64 rcd;
 
 		if ((*raw_rcd & cpu_to_le64(FBNIC_RCD_DONE)) == done)
@@ -1281,12 +1345,21 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
 
 		switch (FIELD_GET(FBNIC_RCD_TYPE_MASK, rcd)) {
 		case FBNIC_RCD_TYPE_HDR_AL:
-			head0 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
+			pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
+			/* Only take the head from a completion we are going
+			 * to act on, so that one rejected ID does not discard
+			 * the progress an earlier completion in this poll has
+			 * already reported.
+			 */
+			if (likely(fbnic_rx_buf_posted(&qt->sub0, pg_idx)))
+				head0 = pg_idx;
 			fbnic_pkt_prepare(nv, rcd, pkt, qt);
 
 			break;
 		case FBNIC_RCD_TYPE_PAY_AL:
-			head1 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
+			pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
+			if (likely(fbnic_rx_buf_posted(&qt->sub1, pg_idx)))
+				head1 = pg_idx;
 			fbnic_add_rx_frag(nv, rcd, pkt, qt);
 
 			break;
@@ -1300,7 +1373,11 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
 			/* We currently ignore the action table index */
 			break;
 		case FBNIC_RCD_TYPE_META:
-			if (likely(!fbnic_rcd_metadata_err(rcd) &&
+			/* A metadata completion with no header completion
+			 * ahead of it leaves nothing to build a frame from.
+			 */
+			if (likely(pkt->buff.data_hard_start &&
+				   !fbnic_rcd_metadata_err(rcd) &&
 				   !pkt->add_frag_failed)) {
 				pkt_bytes = xdp_get_buff_len(&pkt->buff);
 				skb = fbnic_run_xdp(nv, pkt);
-- 
2.43.0

Re: [PATCH net v3 1/5] eth: fbnic: only act on Rx completions naming a posted buffer

[Mohsin Bashir]

On 10/11/26 9:15 PM, Yehyeong Lee wrote:
> An Rx completion descriptor carries a 16-bit buffer id that the driver
> uses to index the rx_buf[] array (FBNIC_RCD_AL_BUFF_PAGE_MASK). The fill
> path masks the id to the ring size, but the completion path does not, so
> the device can hand back an id that is past the array or that names a
> slot which was never posted. An unposted in-range slot holds netmem 0,
> and netmem_to_page(0) returns NULL, which fbnic_pkt_prepare() and
> fbnic_add_rx_frag() then dereference.
> 
> Masking the id would not fix this: it aliases an out-of-range id onto a
> posted buffer and corrupts that buffer's page_pool reference count.
> Reject a completion whose id is outside the array or outside the posted
> window and take the existing drop path.
> 
> The same id is used as the BDQ head in fbnic_clean_bdq(), where an
> out-of-range value spins the cleanup loop; bound it there with the same
> test. Validate head0/head1 in fbnic_clean_rcq() before they are assigned,
> and drop a META completion that arrives with no header buffer rather than
> building an skb around a NULL pointer.
> 
>    Oops: general protection fault, probably for non-canonical address
>    0xdffffc0000000004: 0000 [#1] SMP KASAN NOPTI
>    KASAN: null-ptr-deref in range [0x0000000000000020-0x0000000000000027]
>    RIP: 0010:fbnic_pkt_prepare+0x1a2/0x5a0 [fbnic]
>    Call Trace:
>     <TASK>
>     fbnic_clean_rcq+0x1ba/0x1350 [fbnic]
> 
> 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 | 87 ++++++++++++++++++--
>   1 file changed, 82 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 10caacffee0f0..305fe6d2f511f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -797,6 +797,34 @@ static void fbnic_page_pool_drain(struct fbnic_ring *ring, unsigned int idx,
>   	rx_buf->netmem = 0;
>   }
>   
> +/* The device names an Rx buffer by its index in the buffer descriptor
> + * ring.  A completion may only name a buffer we posted, that is one in
> + * [head, tail), so test the index against the array and against the
> + * posted window.  The window test is not enough on its own: it masks the
> + * index, so an index past the end of the array can fold back into the
> + * window and run off rx_buf[].
> + */
> +static bool fbnic_rx_buf_posted(struct fbnic_ring *ring, unsigned int idx)
> +{
> +	if (idx > ring->size_mask)
> +		return false;
> +
> +	return ((idx - ring->head) & ring->size_mask) < fbnic_desc_used(ring);
> +}
> +
> +/* fbnic_clean_bdq() is given a position to consume up to rather than a
> + * buffer to use, so unlike fbnic_rx_buf_posted() it also accepts tail,
> + * which is how the teardown path asks for the whole ring to be drained.
> + */
> +static bool fbnic_bdq_head_valid(struct fbnic_ring *ring, unsigned int hw_head)
> +{
> +	if (hw_head > ring->size_mask)
> +		return false;
> +
> +	return ((hw_head - ring->head) & ring->size_mask) <=
> +	       fbnic_desc_used(ring);
> +}
> +
>   static void fbnic_clean_twq(struct fbnic_napi_vector *nv, int napi_budget,
>   			    struct fbnic_q_triad *qt, s32 ts_head, s32 head0,
>   			    s32 head1)
> @@ -882,6 +910,13 @@ static void fbnic_clean_bdq(struct fbnic_ring *ring, unsigned int hw_head,
>   {
>   	unsigned int head = ring->head;
>   
> +	/* A head we never posted cannot name a buffer we own, and the loop
> +	 * below masks head to the ring so it would never reach it.  Leave
> +	 * the ring alone and wait for a head we can act on.
> +	 */
> +	if (unlikely(!fbnic_bdq_head_valid(ring, hw_head)))
> +		return;
> +
>   	if (head == hw_head)
>   		return;
>   
> @@ -985,14 +1020,25 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>   {
>   	unsigned int hdr_pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
>   	unsigned int hdr_pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
> -	struct page *page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
>   	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
>   	unsigned int frame_sz, hdr_pg_start, hdr_pg_end, headroom;
>   	unsigned char *hdr_start;
> +	struct page *page;
>   
>   	/* data_hard_start should always be NULL when this is called */
>   	WARN_ON_ONCE(pkt->buff.data_hard_start);
>   
> +	/* A buffer ID we never posted cannot name a buffer we own, so drop
> +	 * the packet instead of running off the end of rx_buf[] or picking
> +	 * up a slot we have already drained.
> +	 */
> +	if (unlikely(!fbnic_rx_buf_posted(&qt->sub0, hdr_pg_idx))) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
> +	page = fbnic_page_pool_get_head(qt, hdr_pg_idx);
> +
>   	/* 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);
> @@ -1026,10 +1072,27 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>   	unsigned int pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
>   	unsigned int pg_off = FIELD_GET(FBNIC_RCD_AL_BUFF_OFF_MASK, rcd);
>   	unsigned int len = FIELD_GET(FBNIC_RCD_AL_BUFF_LEN_MASK, rcd);
> -	netmem_ref netmem = fbnic_page_pool_get_data(qt, pg_idx);
>   	unsigned int truesize;
> +	netmem_ref netmem;
>   	bool added;
>   
> +	/* No header completion has set buff up, either because one was
> +	 * rejected or because the device sent none, so there is nothing to
> +	 * add a fragment to.  Mark the packet so the metadata descriptor
> +	 * drops it instead of building an skb on an empty buffer.
> +	 */
> +	if (unlikely(!pkt->buff.data_hard_start)) {
> +		pkt->add_frag_failed = true;
> +		return;
> +	}
> +
> +	if (unlikely(!fbnic_rx_buf_posted(&qt->sub1, pg_idx))) {
> +		pkt->add_frag_failed = true;
> +		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);
>   
> @@ -1270,6 +1333,7 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
>   	while (likely(packets < budget)) {
>   		struct sk_buff *skb = ERR_PTR(-EINVAL);
>   		u32 pkt_bytes;
> +		u32 pg_idx;
>   		u64 rcd;
>   
>   		if ((*raw_rcd & cpu_to_le64(FBNIC_RCD_DONE)) == done)
> @@ -1281,12 +1345,21 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
>   
>   		switch (FIELD_GET(FBNIC_RCD_TYPE_MASK, rcd)) {
>   		case FBNIC_RCD_TYPE_HDR_AL:
> -			head0 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
> +			pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
> +			/* Only take the head from a completion we are going
> +			 * to act on, so that one rejected ID does not discard
> +			 * the progress an earlier completion in this poll has
> +			 * already reported.
> +			 */
> +			if (likely(fbnic_rx_buf_posted(&qt->sub0, pg_idx)))
> +				head0 = pg_idx;
>   			fbnic_pkt_prepare(nv, rcd, pkt, qt);
>   
>   			break;
>   		case FBNIC_RCD_TYPE_PAY_AL:
> -			head1 = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
> +			pg_idx = FIELD_GET(FBNIC_RCD_AL_BUFF_PAGE_MASK, rcd);
> +			if (likely(fbnic_rx_buf_posted(&qt->sub1, pg_idx)))
> +				head1 = pg_idx;
>   			fbnic_add_rx_frag(nv, rcd, pkt, qt);
>   
>   			break;
> @@ -1300,7 +1373,11 @@ static int fbnic_clean_rcq(struct fbnic_napi_vector *nv,
>   			/* We currently ignore the action table index */
>   			break;
>   		case FBNIC_RCD_TYPE_META:
> -			if (likely(!fbnic_rcd_metadata_err(rcd) &&
> +			/* A metadata completion with no header completion
> +			 * ahead of it leaves nothing to build a frame from.
> +			 */
> +			if (likely(pkt->buff.data_hard_start &&
> +				   !fbnic_rcd_metadata_err(rcd) &&
>   				   !pkt->add_frag_failed)) {
>   				pkt_bytes = xdp_get_buff_len(&pkt->buff);
>   				skb = fbnic_run_xdp(nv, pkt);

As Daniel pointed-out on V1, the device does not make up the buffer 
ID's. Rather, it only echoes them back as provided by the SW. A 
completion with ID's that SW did not post would indicate a bad device or 
an inconsistency between SW and HW state. Either of the two would 
indicate that the issue is somewhere else. These checks would only mask 
the issue.

Re: [PATCH net v3 1/5] eth: fbnic: only act on Rx completions naming a posted buffer

[Yehyeong Lee]

Thanks for looking at these, and for the detailed reasoning.

You're right that the buffer IDs, offsets and lengths are values the
driver itself programs and the device echoes back, and that a healthy
device honours the headroom and placement the driver configures.  The
series treated the device as untrusted, which is not the model fbnic
uses; a completion that disagrees with what software posted points at a
software/hardware state bug these checks would hide rather than fix.
I'll drop patches 1-3.

For patch 4 I missed that Daniel already fixed the same reference leak in
1c85ea5df86b ("eth: fbnic: put back netmem reference when
xdp_buff_add_frag() fails"); please drop it as well.

Sorry for the noise.

[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

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 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

[Yehyeong Lee]

fbnic_pkt_prepare() starts a new packet on each header completion and so
far only warned when pkt->buff was still populated from a previous header
that no META completion had finished. The stale buffer, and any fragments
attached to it, were then overwritten and leaked.

Return the pending buffer to the page pool before starting over and clear
data_hard_start, so the META drop path does not release it a second time.
This leaks memory rather than crashing, so there is no oops to show.

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 | 62 +++++++++++---------
 1 file changed, 33 insertions(+), 29 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 7dda59bba14ee..47ded7a9e0a81 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1014,6 +1014,33 @@ static unsigned int fbnic_hdr_pg_end(unsigned int pg_off, unsigned int len)
 	return ALIGN(pg_off, 128) - FBNIC_RX_HROOM;
 }
 
+static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
+			       struct fbnic_pkt_buff *pkt, int budget)
+{
+	struct page *page;
+
+	if (!pkt->buff.data_hard_start)
+		return;
+
+	if (xdp_buff_has_frags(&pkt->buff)) {
+		struct skb_shared_info *shinfo;
+		netmem_ref netmem;
+		int nr_frags;
+
+		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
+		nr_frags = shinfo->nr_frags;
+
+		while (nr_frags--) {
+			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
+			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
+						  !!budget);
+		}
+	}
+
+	page = virt_to_page(pkt->buff.data_hard_start);
+	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
+}
+
 static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 			      struct fbnic_pkt_buff *pkt,
 			      struct fbnic_q_triad *qt)
@@ -1025,8 +1052,12 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
 	unsigned char *hdr_start;
 	struct page *page;
 
-	/* data_hard_start should always be NULL when this is called */
-	WARN_ON_ONCE(pkt->buff.data_hard_start);
+	/* a new header while one is pending: release the old one */
+	if (unlikely(pkt->buff.data_hard_start)) {
+		WARN_ON_ONCE(1);
+		fbnic_put_pkt_buff(qt, pkt, 1);
+		pkt->buff.data_hard_start = NULL;
+	}
 
 	/* A buffer ID we never posted cannot name a buffer we own, so drop
 	 * the packet instead of running off the end of rx_buf[] or picking
@@ -1146,33 +1177,6 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 	}
 }
 
-static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
-			       struct fbnic_pkt_buff *pkt, int budget)
-{
-	struct page *page;
-
-	if (!pkt->buff.data_hard_start)
-		return;
-
-	if (xdp_buff_has_frags(&pkt->buff)) {
-		struct skb_shared_info *shinfo;
-		netmem_ref netmem;
-		int nr_frags;
-
-		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
-		nr_frags = shinfo->nr_frags;
-
-		while (nr_frags--) {
-			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
-			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
-						  !!budget);
-		}
-	}
-
-	page = virt_to_page(pkt->buff.data_hard_start);
-	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
-}
-
 static struct sk_buff *fbnic_build_skb(struct fbnic_napi_vector *nv,
 				       struct fbnic_pkt_buff *pkt)
 {
-- 
2.43.0

Re: [PATCH net v3 3/5] eth: fbnic: return a pending Rx packet before reusing its buffer

[Mohsin Bashir]

On 10/11/26 9:15 PM, Yehyeong Lee wrote:
> fbnic_pkt_prepare() starts a new packet on each header completion and so
> far only warned when pkt->buff was still populated from a previous header
> that no META completion had finished. The stale buffer, and any fragments
> attached to it, were then overwritten and leaked.
> 
> Return the pending buffer to the page pool before starting over and clear
> data_hard_start, so the META drop path does not release it a second time.
> This leaks memory rather than crashing, so there is no oops to show.
> 
> 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 | 62 +++++++++++---------
>   1 file changed, 33 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 7dda59bba14ee..47ded7a9e0a81 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -1014,6 +1014,33 @@ static unsigned int fbnic_hdr_pg_end(unsigned int pg_off, unsigned int len)
>   	return ALIGN(pg_off, 128) - FBNIC_RX_HROOM;
>   }
>   
> +static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
> +			       struct fbnic_pkt_buff *pkt, int budget)
> +{
> +	struct page *page;
> +
> +	if (!pkt->buff.data_hard_start)
> +		return;
> +
> +	if (xdp_buff_has_frags(&pkt->buff)) {
> +		struct skb_shared_info *shinfo;
> +		netmem_ref netmem;
> +		int nr_frags;
> +
> +		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
> +		nr_frags = shinfo->nr_frags;
> +
> +		while (nr_frags--) {
> +			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
> +			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
> +						  !!budget);
> +		}
> +	}
> +
> +	page = virt_to_page(pkt->buff.data_hard_start);
> +	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
> +}
> +
>   static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>   			      struct fbnic_pkt_buff *pkt,
>   			      struct fbnic_q_triad *qt)
> @@ -1025,8 +1052,12 @@ static void fbnic_pkt_prepare(struct fbnic_napi_vector *nv, u64 rcd,
>   	unsigned char *hdr_start;
>   	struct page *page;
>   
> -	/* data_hard_start should always be NULL when this is called */
> -	WARN_ON_ONCE(pkt->buff.data_hard_start);
> +	/* a new header while one is pending: release the old one */
> +	if (unlikely(pkt->buff.data_hard_start)) {
> +		WARN_ON_ONCE(1);
> +		fbnic_put_pkt_buff(qt, pkt, 1);
> +		pkt->buff.data_hard_start = NULL;
> +	}
>   
>   	/* A buffer ID we never posted cannot name a buffer we own, so drop
>   	 * the packet instead of running off the end of rx_buf[] or picking
> @@ -1146,33 +1177,6 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>   	}
>   }
>   
> -static void fbnic_put_pkt_buff(struct fbnic_q_triad *qt,
> -			       struct fbnic_pkt_buff *pkt, int budget)
> -{
> -	struct page *page;
> -
> -	if (!pkt->buff.data_hard_start)
> -		return;
> -
> -	if (xdp_buff_has_frags(&pkt->buff)) {
> -		struct skb_shared_info *shinfo;
> -		netmem_ref netmem;
> -		int nr_frags;
> -
> -		shinfo = xdp_get_shared_info_from_buff(&pkt->buff);
> -		nr_frags = shinfo->nr_frags;
> -
> -		while (nr_frags--) {
> -			netmem = skb_frag_netmem(&shinfo->frags[nr_frags]);
> -			page_pool_put_full_netmem(qt->sub1.page_pool, netmem,
> -						  !!budget);
> -		}
> -	}
> -
> -	page = virt_to_page(pkt->buff.data_hard_start);
> -	page_pool_put_full_page(qt->sub0.page_pool, page, !!budget);
> -}
> -
>   static struct sk_buff *fbnic_build_skb(struct fbnic_napi_vector *nv,
>   				       struct fbnic_pkt_buff *pkt)
>   {

Under normal operation, the completion sequence is HDR_AL, followed by 
any PAY_AL completions, optional META, and primary META. This patch 
assumes that a new HDR_AL arrives before the previous packet’s primary 
META, violating that sequence.

[PATCH net v3 4/5] eth: fbnic: return the page reference taken for a dropped Rx fragment

[Yehyeong Lee]

fbnic_add_rx_frag() takes a page reference through
fbnic_page_pool_get_data(), which decrements the buffer's pagecnt_bias,
before handing the page to xdp_buff_add_frag().  xdp_buff_add_frag()
fails when the buffer already holds MAX_SKB_FRAGS fragments, and on that
path the reference is lost: add_frag_failed is set and the frame is
dropped, but the bias is not restored and fbnic_put_pkt_buff() only
returns the fragments that were actually attached.

The page is then never returned to the page pool.  A device that keeps
sending payload completions for one packet past MAX_SKB_FRAGS leaks a
page for every completion beyond the limit.

Restore pagecnt_bias on the failure path, the exact inverse of the get,
so fbnic_page_pool_drain() reclaims the page.

Fixes: 61f9a066c309 ("eth: fbnic: Use shinfo to track frags state on Rx")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 47ded7a9e0a81..7a9bd91bcc41c 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1171,6 +1171,8 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
 
 	added = xdp_buff_add_frag(&pkt->buff, netmem, pg_off, len, truesize);
 	if (unlikely(!added)) {
+		/* Give back the reference fbnic_page_pool_get_data() took */
+		qt->sub1.rx_buf[pg_idx].pagecnt_bias++;
 		pkt->add_frag_failed = true;
 		netdev_err_once(nv->napi.dev,
 				"Failed to add fragment to xdp_buff\n");
-- 
2.43.0

Re: [PATCH net v3 4/5] eth: fbnic: return the page reference taken for a dropped Rx fragment

[Mohsin Bashir]

On 10/11/26 9:15 PM, Yehyeong Lee wrote:
> fbnic_add_rx_frag() takes a page reference through
> fbnic_page_pool_get_data(), which decrements the buffer's pagecnt_bias,
> before handing the page to xdp_buff_add_frag().  xdp_buff_add_frag()
> fails when the buffer already holds MAX_SKB_FRAGS fragments, and on that
> path the reference is lost: add_frag_failed is set and the frame is
> dropped, but the bias is not restored and fbnic_put_pkt_buff() only
> returns the fragments that were actually attached.
> 
> The page is then never returned to the page pool.  A device that keeps
> sending payload completions for one packet past MAX_SKB_FRAGS leaks a
> page for every completion beyond the limit.
> 
> Restore pagecnt_bias on the failure path, the exact inverse of the get,
> so fbnic_page_pool_drain() reclaims the page.
> 
> Fixes: 61f9a066c309 ("eth: fbnic: Use shinfo to track frags state on Rx")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
>   drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 47ded7a9e0a81..7a9bd91bcc41c 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -1171,6 +1171,8 @@ static void fbnic_add_rx_frag(struct fbnic_napi_vector *nv, u64 rcd,
>   
>   	added = xdp_buff_add_frag(&pkt->buff, netmem, pg_off, len, truesize);
>   	if (unlikely(!added)) {
> +		/* Give back the reference fbnic_page_pool_get_data() took */
> +		qt->sub1.rx_buf[pg_idx].pagecnt_bias++;
>   		pkt->add_frag_failed = true;
>   		netdev_err_once(nv->napi.dev,
>   				"Failed to add fragment to xdp_buff\n");

We can expect to receive at most four payload frags per packet, so 
exceeding MAX_SKB_FRAGS is not expected during normal operation. 
Independently, Daniel already fixed reference handling in 1c85ea5df86b

[PATCH net v3 5/5] eth: fbnic: bound the XDP Tx completion head to the ring

[Yehyeong Lee]

fbnic_clean_twq1() walks tx_buf[] from ring->head to the head the device
reports in the Tx completion (FBNIC_TCD_TYPE0_HEAD1_MASK, a 16-bit
field). head is masked to the ring, the device head is not, so a head
outside the 1024-entry ring is never reached and the loop does not
terminate; it keeps releasing the same buffers to the page pool. A head
that is inside the ring but past the posted tail walks over descriptors
from an earlier wrap and releases their pages a second time.

Return without cleaning if the device head is outside the ring or outside
the posted window, the way fbnic_clean_bdq() already guards its head.
fbnic_clean_twq0() masks the head difference to the ring and is finite;
twq1 did not, which this brings in line. A head equal to the tail is
accepted, as fbnic_nv_flush() passes the tail to drain the whole ring.

  Oops: general protection fault, probably for non-canonical address
  0xfbd59c000000002b: 0000 [#1] SMP KASAN NOPTI
  KASAN: maybe wild-memory-access in range
  [0xdead000000000158-0xdead00000000015f]
  RIP: 0010:page_pool_return_netmem+0x2a/0x3f0
  Call Trace:
   <TASK>
   fbnic_clean_twq1.isra.0+0x20c/0x410 [fbnic]

The loop was exercised by passing a crafted completion head to
fbnic_clean_twq1() under KASAN; no hardware is involved. On a kernel
without soft-lockup detection the repeated release trips a page_pool
use-after-free before the loop is otherwise noticed.

Fixes: cf4facfb132a ("eth: fbnic: Add support for XDP queues")
Cc: stable@vger.kernel.org
Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
---
 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 7a9bd91bcc41c..3a148eb4e560f 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -661,6 +661,12 @@ static void fbnic_clean_twq1(struct fbnic_napi_vector *nv, bool pp_allow_direct,
 	u64 total_bytes = 0, total_packets = 0;
 	unsigned int head = ring->head;
 
+	/* bound the device head to the ring; see fbnic_clean_twq0() */
+	if (unlikely(hw_head > ring->size_mask ||
+		     ((hw_head - head) & ring->size_mask) >
+		     fbnic_desc_used(ring)))
+		return;
+
 	while (hw_head != head) {
 		struct page *page;
 		u64 twd;
-- 
2.43.0

Re: [PATCH net v3 5/5] eth: fbnic: bound the XDP Tx completion head to the ring

[Mohsin Bashir]

On 10/11/26 9:16 PM, Yehyeong Lee wrote:
> fbnic_clean_twq1() walks tx_buf[] from ring->head to the head the device
> reports in the Tx completion (FBNIC_TCD_TYPE0_HEAD1_MASK, a 16-bit
> field). head is masked to the ring, the device head is not, so a head
> outside the 1024-entry ring is never reached and the loop does not
> terminate; it keeps releasing the same buffers to the page pool. A head
> that is inside the ring but past the posted tail walks over descriptors
> from an earlier wrap and releases their pages a second time.
> 
> Return without cleaning if the device head is outside the ring or outside
> the posted window, the way fbnic_clean_bdq() already guards its head.
> fbnic_clean_twq0() masks the head difference to the ring and is finite;
> twq1 did not, which this brings in line. A head equal to the tail is
> accepted, as fbnic_nv_flush() passes the tail to drain the whole ring.
> 
>    Oops: general protection fault, probably for non-canonical address
>    0xfbd59c000000002b: 0000 [#1] SMP KASAN NOPTI
>    KASAN: maybe wild-memory-access in range
>    [0xdead000000000158-0xdead00000000015f]
>    RIP: 0010:page_pool_return_netmem+0x2a/0x3f0
>    Call Trace:
>     <TASK>
>     fbnic_clean_twq1.isra.0+0x20c/0x410 [fbnic]
> 
> The loop was exercised by passing a crafted completion head to
> fbnic_clean_twq1() under KASAN; no hardware is involved. On a kernel
> without soft-lockup detection the repeated release trips a page_pool
> use-after-free before the loop is otherwise noticed.
> 
> Fixes: cf4facfb132a ("eth: fbnic: Add support for XDP queues")
> Cc: stable@vger.kernel.org
> Signed-off-by: Yehyeong Lee <yhlee@isslab.korea.ac.kr>
> ---
>   drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> index 7a9bd91bcc41c..3a148eb4e560f 100644
> --- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> +++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
> @@ -661,6 +661,12 @@ static void fbnic_clean_twq1(struct fbnic_napi_vector *nv, bool pp_allow_direct,
>   	u64 total_bytes = 0, total_packets = 0;
>   	unsigned int head = ring->head;
>   
> +	/* bound the device head to the ring; see fbnic_clean_twq0() */
> +	if (unlikely(hw_head > ring->size_mask ||
> +		     ((hw_head - head) & ring->size_mask) >
> +		     fbnic_desc_used(ring)))
> +		return;
> +
>   	while (hw_head != head) {
>   		struct page *page;
>   		u64 twd;

The device can only consume descriptors up to the tail published by the 
driver. A completion head outside the ring violates that contract which 
can be hardening but not a bug fix. IMHO such a defensive hardening may 
not be a good idea.

Re: [PATCH net v3 0/5] eth: fbnic: harden Rx and Tx completion handling

[netdev-bot+sinfo]

Hi!

This is an automated message. This series looks like a fix, but its
commit messages seem to be missing some information:

 - How the issue was discovered, e.g. hit in production, hit during
   development, syzbot report, manual code inspection, LLM or static
   analysis tool scan.

Please do not repost the series just to address the above. Instead,
reply to this email with the missing information, so that reviewers
can take it into account. If the series needs another revision for
other reasons, please include the information in the commit messages
then.

The evaluation is done by an LLM so it may be wrong, if you think
that is the case please reply and explain.

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