netdev - Re: [PATCH net-next 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]

Message-ID: <940b8c06-b71f-f6b1-4832-4abc58027589@iogearbox.net>
Date:   Mon, 27 Apr 2020 21:01:14 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>, sameehj@...zon.com
Cc:     netdev@...r.kernel.org, bpf@...r.kernel.org, zorik@...zon.com,
        akiyano@...zon.com, gtzalik@...zon.com,
        Toke Høiland-Jørgensen <toke@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        John Fastabend <john.fastabend@...il.com>,
        Alexander Duyck <alexander.duyck@...il.com>,
        Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        David Ahern <dsahern@...il.com>,
        Willem de Bruijn <willemdebruijn.kernel@...il.com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        Lorenzo Bianconi <lorenzo@...nel.org>,
        Saeed Mahameed <saeedm@...lanox.com>,
        steffen.klassert@...unet.com
Subject: Re: [PATCH net-next 29/33] xdp: allow bpf_xdp_adjust_tail() to grow
 packet size

On 4/22/20 6:09 PM, 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.
> 
> 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        |   15 +++++++++++++--
>   2 files changed, 15 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 7d6ceaa54d21..5e9c387f74eb 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -3422,12 +3422,23 @@ 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;
>   
> +	/* ALL drivers MUST init xdp->frame_sz, some chicken checks below */
> +	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;
> +	}

I don't think we can add the WARN()s here. If there is a bug in the driver in this
area and someone deploys an XDP-based application (otherwise known to work well
elsewhere) on top of this, then an attacker can basically remote DoS the machine
with malicious packets that end up triggering these WARN()s over and over.

If you are worried that not all your driver changes are correct, maybe only add
those that you were able to actually test yourself or that have been acked, and
otherwise pre-init the frame_sz to a known invalid value so this helper would only
allow shrinking for them in here (as today)?

Thanks,
Daniel

>   	if (unlikely(data_end < xdp->data + ETH_HLEN))
>   		return -EINVAL;
>   
> 
>