netdev - Re: [PATCH RFC v2 29/33] xdp: allow bpf_xdp_adjust

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: Windows password security audit tool. GUI, reports in PDF.

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

Date:   Thu, 9 Apr 2020 03:31:14 +0000
From:   Saeed Mahameed <saeedm@...lanox.com>
To:     "brouer@...hat.com" <brouer@...hat.com>
CC:     "toke@...hat.com" <toke@...hat.com>,
        "gtzalik@...zon.com" <gtzalik@...zon.com>,
        "ilias.apalodimas@...aro.org" <ilias.apalodimas@...aro.org>,
        "borkmann@...earbox.net" <borkmann@...earbox.net>,
        "alexander.duyck@...il.com" <alexander.duyck@...il.com>,
        "john.fastabend@...il.com" <john.fastabend@...il.com>,
        "akiyano@...zon.com" <akiyano@...zon.com>,
        "zorik@...zon.com" <zorik@...zon.com>,
        "alexei.starovoitov@...il.com" <alexei.starovoitov@...il.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "jeffrey.t.kirsher@...el.com" <jeffrey.t.kirsher@...el.com>,
        "bpf@...r.kernel.org" <bpf@...r.kernel.org>,
        "dsahern@...il.com" <dsahern@...il.com>,
        "lorenzo@...nel.org" <lorenzo@...nel.org>,
        "willemdebruijn.kernel@...il.com" <willemdebruijn.kernel@...il.com>
Subject: Re: [PATCH RFC v2 29/33] xdp: allow bpf_xdp_adjust_tail() to grow
 packet size

On Wed, 2020-04-08 at 13:53 +0200, Jesper Dangaard Brouer wrote:
> Finally, after all drivers have a frame size, allow BPF-helper
> bpf_xdp_adjust_tail() to grow or extend packet size at frame tail.
> 

can you provide a list of usecases for why tail extension is necessary
?

and what do you have in mind as immediate use of bpf_xdp_adjust_tail()
? 

both cover letter and commit messages didn't list any actual use case..

> Remember that helper/macro xdp_data_hard_end have reserved some
> tailroom.  Thus, this helper makes sure that the BPF-prog don't have
> access to this tailroom area.
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  include/uapi/linux/bpf.h |    4 ++--
>  net/core/filter.c        |   18 ++++++++++++++++--
>  2 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h
> index 2e29a671d67e..0e5abe991ca3 100644
> --- a/include/uapi/linux/bpf.h
> +++ b/include/uapi/linux/bpf.h
> @@ -1969,8 +1969,8 @@ union bpf_attr {
>   * int bpf_xdp_adjust_tail(struct xdp_buff *xdp_md, int delta)
>   * 	Description
>   * 		Adjust (move) *xdp_md*\ **->data_end** by *delta*
> bytes. It is
> - * 		only possible to shrink the packet as of this writing,
> - * 		therefore *delta* must be a negative integer.
> + * 		possible to both shrink and grow the packet tail.
> + * 		Shrink done via *delta* being a negative integer.
>   *
>   * 		A call to this helper is susceptible to change the
> underlying
>   * 		packet buffer. Therefore, at load time, all checks on
> pointers
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 7628b947dbc3..4d58a147eed0 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3422,12 +3422,26 @@ static const struct bpf_func_proto
> bpf_xdp_adjust_head_proto = {
>  
>  BPF_CALL_2(bpf_xdp_adjust_tail, struct xdp_buff *, xdp, int, offset)
>  {
> +	void *data_hard_end = xdp_data_hard_end(xdp);
>  	void *data_end = xdp->data_end + offset;
>  
> -	/* only shrinking is allowed for now. */
> -	if (unlikely(offset >= 0))
> +	/* Notice that xdp_data_hard_end have reserved some tailroom */
> +	if (unlikely(data_end > data_hard_end))
>  		return -EINVAL;
>  

i don't know if i like this approach for couple of reasons.

1. drivers will provide arbitrary frames_sz, which is normally larger
than mtu, and could be a full page size, for XDP_TX action this can be
problematic if xdp progs will allow oversized packets to get caught at
the driver level.. 

2. xdp_data_hard_end(xdp) has a hardcoded assumption of the skb shinfo
and it introduces a reverse dependency between xdp buff and skbuff 

both of the above can be solved if the drivers provided the max allowed
frame size, already accounting for mtu and shinfo when setting
xdp_buff.frame_sz at the driver level.


> +	/* DANGER: ALL drivers MUST be converted to init xdp->frame_sz
> +	 * - Adding some chicken checks below
> +	 * - Will (likely) not be for upstream
> +	 */
> +	if (unlikely(xdp->frame_sz < (xdp->data_end - xdp-
> >data_hard_start))) {
> +		WARN(1, "Too small xdp->frame_sz = %d\n", xdp-
> >frame_sz);
> +		return -EINVAL;
> +	}
> +	if (unlikely(xdp->frame_sz > PAGE_SIZE)) {
> +		WARN(1, "Too BIG xdp->frame_sz = %d\n", xdp->frame_sz);
> +		return -EINVAL;
> +	}
> +
>  	if (unlikely(data_end < xdp->data + ETH_HLEN))
>  		return -EINVAL;
>  
> 
>