Re: [PATCH bpf v3 1/2] bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
bpf, veth: xdp: fix page_pool page leak on skb-backed XDP

This adds an XDP_FLAGS_FRAGS_PAGE_POOL xdp_buff flag that the two skb-cow
sites (generic XDP and veth) set, so bpf_xdp_shrink_data() frees a released
frag to the page_pool instead of using xdp->rxq->mem.type. The flag is
stripped on buff -> frame conversion and re-derived from frame->mem_type on
frame -> buff conversion.

Closes: https://syzkaller.appspot.com/bug?extid=237bbeed8dfe0699b7f5

> diff --git a/include/net/xdp.h b/include/net/xdp.h
> index aa742f413c358..8fd4139e9cbd3 100644
> --- a/include/net/xdp.h
> +++ b/include/net/xdp.h

[ ... ]

> @@ -394,6 +412,15 @@ void xdp_convert_frame_to_buff(const struct xdp_frame *frame,
>  	xdp->data_meta = frame->data - frame->metasize;
>  	xdp->frame_sz = frame->frame_sz;
>  	xdp->flags = frame->flags;
> +	/*
> +	 * frame->flags never carries XDP_FLAGS_FRAGS_PAGE_POOL (it is stripped
> +	 * in xdp_update_frame_from_buff()); re-derive it from the frame's own
> +	 * memory type. veth and devmap rebuild a buff here and run a program
> +	 * whose rxq says MEM_TYPE_PAGE_SHARED, so without this a shrink would
> +	 * free a page_pool frag through page_frag_free().
> +	 */
> +	if (frame->mem_type == MEM_TYPE_PAGE_POOL)
> +		xdp_buff_set_frag_pp(xdp);
>  }

[ ... ]

> diff --git a/net/core/filter.c b/net/core/filter.c
> index 532405988fd98..861c878d0a6d6 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -4377,6 +4377,13 @@ static bool bpf_xdp_shrink_data(struct xdp_buff *xdp, skb_frag_t *frag,
>  	if (mem_type == MEM_TYPE_XSK_BUFF_POOL) {
>  		netmem = 0;
>  		zc_frag = bpf_xdp_shrink_data_zc(xdp, shrink, tail, release);
> +	} else if (xdp_buff_is_frag_pp(xdp)) {
> +		/*
> +		 * Skb-backed XDP (generic XDP, veth) cow's the frags into a
> +		 * page_pool while the rxq stays MEM_TYPE_PAGE_SHARED, so free
> +		 * the frag to the pool, not via page_frag_free().
> +		 */
> +		mem_type = MEM_TYPE_PAGE_POOL;
>  	}
>  
>  	if (release) {

[Severity: High]
The new flag only overrides mem_type in one direction (towards
MEM_TYPE_PAGE_POOL), while mem_type is still initialized from the shared
xdp->rxq->mem.type:

net/core/filter.c:bpf_xdp_shrink_data() {
	enum xdp_mem_type mem_type = xdp->rxq->mem.type;
	...
}

The commit message says veth's rq->xdp_rxq.mem "is shared per-queue state"
that "can be clobbered under a packet still in flight". veth also writes a
frame's memory type into it and never restores it:

drivers/net/veth.c:veth_xdp_rcv_one() {
	case XDP_TX:
		orig_frame = *frame;
		xdp->rxq->mem.type = frame->mem_type;
	...
	case XDP_REDIRECT:
		orig_frame = *frame;
		xdp->rxq->mem.type = frame->mem_type;
}

Only veth_disable_xdp_range() puts rq->xdp_rxq.mem back to rq->xdp_mem.

So can the opposite direction still go wrong on a veth queue?

1) A multi-buff frame with frame->mem_type == MEM_TYPE_PAGE_POOL (e.g.
   redirected in from a page_pool NIC) takes XDP_TX or XDP_REDIRECT, and
   rq->xdp_rxq.mem.type stays MEM_TYPE_PAGE_POOL for every later packet on
   that queue.

2) A later multi-buff frame whose frame->mem_type is MEM_TYPE_PAGE_SHARED
   (the peer's veth_xdp_rcv_skb() XDP_TX/XDP_REDIRECT path stamps
   xdp->rxq->mem = rq->xdp_mem into the frame) or MEM_TYPE_PAGE_ORDER0
   arrives. xdp_convert_frame_to_buff() does not set
   XDP_FLAGS_FRAGS_PAGE_POOL for it, and it does not clear the flag either,
   so bpf_xdp_shrink_data() picks up the stale MEM_TYPE_PAGE_POOL from the
   rxq.

3) If the program then calls bpf_xdp_adjust_tail() with a negative offset
   that consumes a whole frag, the release path ends in:

net/core/xdp.c:__xdp_return() {
	case MEM_TYPE_PAGE_POOL:
		netmem = netmem_compound_head(netmem);
		...
		/* No need to check netmem_is_pp() as mem->type knows this a
		 * page_pool page
		 */
		page_pool_put_full_netmem(netmem_get_pp(netmem), netmem,
					  napi_direct);
}

There is no netmem_is_pp() validation here, so netmem_get_pp() on a plain
page returns whatever occupies the overlaid field, and NULL for a page that
veth already released from its pool. Can that dereference a garbage or NULL
struct page_pool *, or hand a foreign page to an unrelated pool's cache?

Would it be more robust for bpf_xdp_shrink_data() to derive the frag memory
type entirely from the buff (i.e. also express "not page_pool" instead of
falling back to rxq->mem.type), so the shared and clobberable veth rxq type
is not consulted at all?

Note this residual path is not addressed later in the series: the diff of
net/, drivers/net/veth.c and include/net/xdp.h between this commit and
6c9c432f is empty, the following patch only adds a selftest.