netdev - Re: [PATCH v2] sfc: handle NULL returned by xdp_convert_buff_to

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <2671c27e-9a5e-4a6f-ae03-30ec1eb83013@kernel.org>
Date: Thu, 31 Jul 2025 11:23:24 +0200
From: Jesper Dangaard Brouer <hawk@...nel.org>
To: Chenyuan Yang <chenyuan0y@...il.com>, ecree.xilinx@...il.com,
 andrew+netdev@...n.ch, davem@...emloft.net, edumazet@...gle.com,
 kuba@...nel.org, pabeni@...hat.com, ast@...nel.org, daniel@...earbox.net,
 john.fastabend@...il.com, sdf@...ichev.me, lorenzo@...nel.org
Cc: netdev@...r.kernel.org, linux-net-drivers@....com, bpf@...r.kernel.org,
 zzjas98@...il.com, Kunwu Chan <kunwu.chan@...ux.dev>,
 Edward Cree <ecree@....com>
Subject: Re: [PATCH v2] sfc: handle NULL returned by
 xdp_convert_buff_to_frame()



On 26/07/2025 21.56, Chenyuan Yang wrote:
> The xdp_convert_buff_to_frame() function can return NULL when there is
> insufficient headroom in the buffer to store the xdp_frame structure
> or when the driver didn't reserve enough tailroom for skb_shared_info.
> 
> Currently, the sfc driver does not check for this NULL return value
> in the XDP_TX case within efx_do_xdp(). While the efx_xdp_tx_buffers()
> function has some defensive checks, passing a NULL xdpf can still lead
> to undefined behavior when the function tries to access xdpf->len and
> xdpf->data.
> 

The XDP_TX case is only for driver local frames/packets. And Edward says
the sfc driver reserves both enough headroom and tailroom.

> Fix by adding a proper NULL check in the XDP_TX case, following the
> suggestions of the developers.

In [V1] reply I question if this is possible
  - [v1] 
https://lore.kernel.org/all/a13646af-78f7-4ba7-9767-41d598222b1d@kernel.org/

Hmm... have you actually tested that XDP/BPF can adjust headroom so much
that xdp_convert_buff_to_frame() function fails?

I really doubt this possible for BPF-progs to violate this.

The XDP BPF-prog can only adjust the headroom via the helpers
bpf_xdp_adjust_head() and bpf_xdp_adjust_meta().  These helpers reserve
room for sizeof(struct xdp_frame).

The tailroom can be adjusted via helper bpf_xdp_adjust_tail() and it
also reserve room for sizeof(struct skb_shared_info) such that BPF-progs
cannot get access to this area. See define for xdp_data_hard_end.


> Fixes: 1b698fa5d8ef ("xdp: Rename convert_to_xdp_frame in xdp_convert_buff_to_frame")
> Signed-off-by: Chenyuan Yang <chenyuan0y@...il.com>
> Cc: Kunwu Chan <kunwu.chan@...ux.dev>
> Cc: Edward Cree <ecree@....com>
> ---
>   drivers/net/ethernet/sfc/rx.c | 12 +++++++++---
>   1 file changed, 9 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/net/ethernet/sfc/rx.c b/drivers/net/ethernet/sfc/rx.c
> index ffca82207e47..b56457c23f66 100644
> --- a/drivers/net/ethernet/sfc/rx.c
> +++ b/drivers/net/ethernet/sfc/rx.c
> @@ -308,14 +308,20 @@ static bool efx_do_xdp(struct efx_nic *efx, struct efx_channel *channel,
>   	case XDP_TX:
>   		/* Buffer ownership passes to tx on success. */
>   		xdpf = xdp_convert_buff_to_frame(&xdp);
> -		err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
> +		if (unlikely(!xdpf))
> +			err = -ENOBUFS;
> +		else
> +			err = efx_xdp_tx_buffers(efx, 1, &xdpf, true);
> +
>   		if (unlikely(err != 1)) {
>   			efx_free_rx_buffers(rx_queue, rx_buf, 1);
>   			if (net_ratelimit())
>   				netif_err(efx, rx_err, efx->net_dev,
> -					  "XDP TX failed (%d)\n", err);
> +					  "XDP TX failed (%d)%s\n", err,
> +					  err == -ENOBUFS ? " [frame conversion]" : "");
>   			channel->n_rx_xdp_bad_drops++;
> -			trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
> +			if (err != -ENOBUFS)
> +				trace_xdp_exception(efx->net_dev, xdp_prog, xdp_act);
>   		} else {
>   			channel->n_rx_xdp_tx++;
>   		}