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: fbnic_clean_rcq+0x1ba/0x1350 [fbnic] Fixes: a29b8eb6e533 ("eth: fbnic: Add basic Rx handling") Cc: stable@vger.kernel.org Signed-off-by: Yehyeong Lee --- 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 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: __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 --- 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 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 --- 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 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 --- 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 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: 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 --- 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