Re: [PATCH bpf-next 11/15] ice: set skb RX metadata from the skb core extension area of xdp_buff

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [Critical] ice: invalid page dereference and page leak in
  ice_clean_rx_irq() [1]
--

commit ba151a63223661db4511ce1634b36c26afe91c8f
Author: Tianyi Gao <tianyi@cloudflare.com>

ice: set skb RX metadata from the skb core extension area of xdp_buff

This commit populates skb RX metadata from the xdp_buff skb core extension
area on XDP_PASS in the ice driver and exports xdp_skb_ext_to_skb().

> diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> index a2c7c4962882d..06a1a5fdc43b7 100644
> --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
[ ... ]
> @@ -1041,6 +1042,7 @@ static int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
>  		continue;
>  
>  construct_skb:
> +		ext = xdp_buff_skb_ext(&xdp->base);
>  		skb = xdp_build_skb_from_buff(&xdp->base);
>  		xdp->data = NULL;

[Severity: Critical]
This problem wasn't introduced by this patch, but clearing xdp->data before
checking whether xdp_build_skb_from_buff() succeeded causes an invalid page
dereference and leaks the head buffer [1] when skb allocation fails:

drivers/net/ethernet/intel/ice/ice_txrx.c:ice_clean_rx_irq() {
	...
construct_skb:
		ext = xdp_buff_skb_ext(&xdp->base);
		skb = xdp_build_skb_from_buff(&xdp->base);
		xdp->data = NULL;

		/* exit if we failed to retrieve a buffer */
		if (!skb) {
			libeth_xdp_return_buff_slow(xdp);
	...
}

When napi_build_skb() fails inside xdp_build_skb_from_buff(), it returns
NULL without freeing the page_pool buffers attached to xdp. Because
xdp->data is already set to NULL, libeth_xdp_return_buff_slow() passes NULL
to libeth_xdp_return_va(), which translates NULL via virt_to_netmem() and
dereferences an invalid page pointer while leaking the head page.

Should xdp->data = NULL be moved after the !skb check (since
__libeth_xdp_return_buff() already clears xdp->data on the failure path)?

[1] https://sashiko.dev/#/bug/linux-d6eb94b5-df62-4b78-9f73-358fdd002a0c

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1794281983.git.tianyi@cloudflare.com?part=11