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

[Mohsin Bashir <mohsin.bashr@gmail.com>]

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.