Re: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support

On 08/06 21:36, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [Critical] Use-After-Free in the AF_XDP Zero-Copy receive path due to stagnant index variable on error.
> --
> 
> --- Patch [10]: [PATCH net-next 10/15] i40e: prepare for XDP metadata ops support ---
> commit cf380b8d06da010c94723db1eb6af6df8fb0fdc8
> Author: Kohei Enju <[email protected]>
> 
> i40e: prepare for XDP metadata ops support
> 
> This commit introduces struct i40e_xdp_buff to wrap an xdp_buff alongside
> a pointer to an i40e_rx_desc. It passes the RX descriptor to XDP kfuncs
> and adds bounds checking in the AF_XDP Zero-Copy path using
> XSK_CHECK_PRIV_TYPE.
> 
> > diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > index 9f47388eaba53..51a05ce4c7ce8 100644
> > --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> > +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> 
> [ ... ]
> 
> > @@ -472,6 +482,8 @@ int i40e_clean_rx_irq_zc(struct i40e_ring *rx_ring, int budget)
> >  		if (i40e_is_non_eop(rx_ring, rx_desc))
> >  			continue;
> >  
> > +		xsk_buff_to_i40e_ctx(first)->desc = rx_desc;
> > +
> 
> [Severity: Critical]
> This is a pre-existing issue, but does this function have a use-after-free
> in its error handling path?
> 
> If xsk_buff_add_frag() fails earlier in the loop, it calls

IIUC, I40E_MAX_CHAINED_RX_BUFFERS (5) limits a packet to one head and at
most four frags. Since xsk_buff_add_frag() fails only when nr_frags ==
MAX_SKB_FRAGS, which is at least 17, this path is unreachable for a
valid i40e RX chain.

However, the UAF scenario looks valid if it does fail, so the
pre-existing error may still be worth fixing.

> xsk_buff_free(first) and breaks out of the loop without advancing
> next_to_clean:
> 
>     if (!first)
>         first = bi;
>     else if (!xsk_buff_add_frag(first, bi)) {
>         xsk_buff_free(first);
>         break;
>     }
> 
> On the subsequent NAPI poll, because next_to_process != next_to_clean,
> the code loads the freed buffer pointer from the stale next_to_clean
> index:
> 
>     if (next_to_process != next_to_clean)
>         first = *i40e_rx_bi(rx_ring, next_to_clean);
> 
> Could this cause the next packet to be built using a freed buffer?
> 
> >  		xdp_res = i40e_run_xdp_zc(rx_ring, first, xdp_prog);
> >  		i40e_handle_xdp_result_zc(rx_ring, first, rx_desc, &rx_packets,
> >  					  &rx_bytes, xdp_res, &failure);
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=10