[PATCH net v2 0/2] eth: fbnic: validate the Rx completion path

[PATCH net v2 0/2] eth: fbnic: validate the Rx completion path

[Yehyeong Lee]

Patch 1 is v2 of "eth: fbnic: validate the Rx completion buffer index".
Besides bounding the completion buffer index against rx_buf[], it now
also rejects a buffer-descriptor-queue clean-up head that falls outside
the ring in fbnic_clean_bdq(), and sets add_frag_failed when
fbnic_add_rx_frag() bails on a buffer that was never set up.  Both were
raised in review of v1:
https://lore.kernel.org/netdev/20261009175358.1598001-1-yhlee@isslab.korea.ac.kr/

Patch 2 returns a page-pool reference that is leaked on the
fragment-overflow path of the same function, found while auditing
patch 1.  It carries a different Fixes tag, so it is a separate patch.

v2:
 - patch 1: also reject an out-of-range clean-up head in
   fbnic_clean_bdq(), and set add_frag_failed in fbnic_add_rx_frag()'s
   no-buffer bail.
 - patch 2: new.

Yehyeong Lee (2):
  eth: fbnic: validate the Rx completion buffer index
  eth: fbnic: return the page reference taken for a dropped Rx fragment

 drivers/net/ethernet/meta/fbnic/fbnic_txrx.c | 40 +++++++++++++++++++-
 1 file changed, 38 insertions(+), 2 deletions(-)

-- 
2.43.0

[PATCH net v2 1/2] eth: fbnic: validate the Rx completion buffer index

[Yehyeong Lee]

The buffer ID in an address/length Rx completion descriptor is a 16 bit
field, FBNIC_RCD_AL_BUFF_PAGE_MASK being DESC_GENMASK(15, 0) once the
fragment bits are removed, so the device can name any index from 0 to
65535.  fbnic_pkt_prepare() and fbnic_add_rx_frag() feed that value
straight into fbnic_page_pool_get_head() and fbnic_page_pool_get_data(),
which use it to subscript qt->sub0.rx_buf[] and qt->sub1.rx_buf[], and
fbnic_clean_rcq() keeps the same value in head0/head1 and hands it to
fbnic_clean_bdq() as the position the device has consumed up to.

Those arrays only cover the buffers the driver posted.  They are
allocated with size_mask + 1 entries, and size_mask comes from
hpq_size / FBNIC_BD_FRAG_COUNT, so at the default FBNIC_HPQ_SIZE_DEFAULT
of 256 each array holds 256 entries of 16 bytes, that is 4096 bytes.  A
buffer ID of 65535 reaches 1048560 bytes past the start of the
allocation.

The fill side already masks every index it computes against
bdq->size_mask, and so does all of the ring head and tail arithmetic.
The completion side does not.  A descriptor naming a buffer that was
never posted makes fbnic_page_pool_get_head() decrement pagecnt_bias
through a pointer outside the array and hand the out-of-bounds
netmem_ref back as a struct page; page_pool_get_dma_addr() and
page_address() then dereference it and the skb is built on whatever it
points at.

Injecting an out-of-range header completion reports under KASAN:

  BUG: KASAN: slab-out-of-bounds in fbnic_page_pool_get_head
  Read of size 8 at addr ffff88800c719008
  The buggy address is located 8 bytes to the right of
   allocated 4096-byte region [ffff88800c718000, ffff88800c719000)

Check the ID against size_mask before it is used and drop the packet if
it is out of range.  Masking it instead would keep the access in bounds
but would silently attribute the completion to an unrelated buffer that
the driver does own, which is worse than a drop.  Set add_frag_failed so
the frame takes the existing error path.

fbnic_clean_bdq() needs the same value bounded, for a different reason.
It walks from ring->head to the head the device reported, masking its
own running index against size_mask on every step:

	do {
		fbnic_page_pool_drain(ring, head, napi_budget);

		head++;
		head &= ring->size_mask;
	} while (head != hw_head);

A hw_head above size_mask is therefore never reached.  The loop drains
every buffer on the ring, wraps, and comes back to a slot whose netmem
fbnic_page_pool_drain() has already cleared, so page_pool_unref_netmem()
runs on a NULL netmem:

  BUG: KASAN: null-ptr-deref in fbnic_clean_bdq
  Read of size 8 at addr 0000000000000028

Return without touching the ring in that case.  head0 and head1 are
recomputed on every poll, so the first head the driver can act on lets
the clean-up catch up from ring->head and no buffer is leaked.  Masking
the head instead would release buffers the device never said it was done
with.

Finally, fbnic_add_rx_frag() has to mark the packet when it bails out on
a buffer that was never set up.  That bail is reached both when a header
completion was rejected and when the device sends a payload completion
with no header completion ahead of it, and in the second case
add_frag_failed is still clear, so the metadata descriptor treats the
packet as good and builds an skb on an empty buffer:

  BUG: KASAN: null-ptr-deref in __build_skb_around
  Write of size 32 at addr 0000000000000ec0

Set add_frag_failed there so the metadata case drops the frame instead.
On the normal path data_hard_start is always set, so the bail is not
taken and the buffer accounting is untouched.

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 | 38 ++++++++++++++++++--
 1 file changed, 36 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
index 10caacffee0f0..b4ca9f7e8bb2e 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -882,6 +882,13 @@ static void fbnic_clean_bdq(struct fbnic_ring *ring, unsigned int hw_head,
 {
 	unsigned int head = ring->head;
 
+	/* A head outside the ring cannot name a buffer we posted, 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(hw_head > ring->size_mask))
+		return;
+
 	if (head == hw_head)
 		return;
 
@@ -985,14 +992,24 @@ 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 the device never got from us cannot name a buffer we
+	 * own, so drop the packet instead of running off the end of rx_buf[].
+	 */
+	if (unlikely(hdr_pg_idx > qt->sub0.size_mask)) {
+		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 +1043,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(pg_idx > qt->sub1.size_mask)) {
+		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);
 
-- 
2.43.0

[PATCH net v2 2/2] 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 b4ca9f7e8bb2e..ba663fe7598f1 100644
--- a/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
+++ b/drivers/net/ethernet/meta/fbnic/fbnic_txrx.c
@@ -1076,6 +1076,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 v2 0/2] eth: fbnic: validate the Rx completion path

[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:

 - Whether the issue was actually triggered, or is only theoretical
   (e.g. found by code inspection). If it was triggered please include
   the symptoms, like the stack trace or error messages.

 - What hardware the change was tested on. For driver fixes please
   mention the device (and if relevant firmware version) used for
   testing, or say that the change was not tested on real hardware.

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.