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

[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 5/5] eth: fbnic: bound the XDP Tx completion head to the ring

[netdev-bot+sashiko]

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

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

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] The patch fixes only TWQ1.

Pre-existing issues:
- [High] fbnic_clean_twq1() can release the same XDP_TX pages to the
  page_pool twice, by a path the new head-window check does not cover.

--

eth: fbnic: bound the XDP Tx completion head to the ring

This patch adds a check to fbnic_clean_twq1() that returns early when the
TWQ1 completion head reported by the device is outside the ring or outside
the posted window. The goal is to stop the cleanup loop from running
forever, and to stop XDP Tx pages being released to the page pool twice,
when the device reports a bogus head.

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

[Severity: High]
The new comment names fbnic_clean_twq0() as the model for this check. The
commit message says:

  fbnic_clean_twq0() masks the head difference to the ring and is finite;
  twq1 did not, which this brings in line.

Does fbnic_clean_twq0() actually do either of these checks? As far as I
can see, it only masks the head:

fbnic_clean_twq0() {
    ...
	clean_desc = (hw_head - head) & ring->size_mask;

	while (clean_desc) {
		struct sk_buff *skb = ring->tx_buf[head];
		unsigned int desc_cnt;

		desc_cnt = FBNIC_XMIT_CB(skb)->desc_count;
		if (desc_cnt > clean_desc)
			break;
    ...
}

fbnic_clean_tcq() reads head0 from the same TCD that head1 comes from:

	head0 = FIELD_GET(FBNIC_TCD_TYPE0_HEAD0_MASK, tcd);

head0 then reaches fbnic_clean_twq0() through fbnic_clean_twq() with no
ring or window check.

Suppose head0 is past the posted tail, or wraps behind ring->head. The loop
cleans every posted packet and head reaches ring->tail, but clean_desc is
still non-zero.

Wouldn't skb = ring->tx_buf[tail] be NULL at that point? The slot was
either never written or was cleared by ring->tx_buf[head] = NULL. If so,
FBNIC_XMIT_CB(skb)->desc_count dereferences NULL before the
desc_cnt > clean_desc break can run.

The timestamp path has the same exposure. It stores the unchecked value in
FBNIC_XMIT_CB(skb)->hw_head, and fbnic_clean_tsq() later passes it back as
head0.

fbnic_bdq_head_valid() already implements this exact check:

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

Could this helper be used for both TWQ0 and TWQ1? If so, the comment and
commit message could be updated so they no longer describe TWQ0 as already
bounded.

> +
>  	while (hw_head != head) {
>  		struct page *page;
>  		u64 twd;

[Severity: High]
This is a pre-existing issue, not one introduced by this patch. Can
fbnic_clean_twq1() still release the same XDP_TX pages twice, by a path
the new window check does not cover?

Further down in fbnic_clean_twq1(), the page is released for every AL
descriptor the loop walks. ring->head only advances when total_bytes is
non-zero:

		page_pool_put_page(pp_page_to_nmdesc(page)->pp, page, -1,
				   pp_allow_direct);
next_desc:
		head++;
		head &= ring->size_mask;
	}

	if (!total_bytes)
		return;

	ring->head = head;

The tx_buf[] entries are not cleared either. If every descriptor walked in
one call has length 0, the pages go back to the pool but ring->head does
not move.

The next TWQ1 completion would then walk from the stale head and put the
same pages again. So would the flush in fbnic_nv_flush():

	fbnic_clean_twq1(nv, false, &qt->sub1, true,
			 qt->sub1.tail);

A zero-length AL descriptor appears to be postable. FBNIC_RX_PAD is 0, and
fbnic_pkt_prepare() accepts an RCD with len == 0 and builds a zero-length
XDP frame from it.

An XDP program that returns XDP_TX without adjusting the head or tail sends
that frame to fbnic_pkt_tx(). fbnic_pkt_tx() stores the page in tx_buf[]
and writes this length into the descriptor:

	size = pkt->buff.data_end - pkt->buff.data;

Zero-size frags from fbnic_add_rx_frag() can also be posted as len-0
descriptors when a non-zero frag follows them.

A device head1 just past such descriptors would pass the new check. Should
ring->head be advanced before the total_bytes early return?

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