netdev - Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap with xdp

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <51af68d8-3c02-b828-12b5-f2dc73406a65@iogearbox.net>
Date:   Wed, 18 Apr 2018 18:21:21 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     netdev@...r.kernel.org
Subject: Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap
 with xdp_frame area

On 04/18/2018 02:10 PM, Jesper Dangaard Brouer wrote:
> If combining xdp_adjust_head and xdp_adjust_meta, then it is possible
> to make data_meta overlap with area used by xdp_frame.  And another
> invocation of xdp_adjust_head can then clear that area, due to
> clearing of xdp_frame area.
> 
> The easiest solution I found was to simply not allow
> xdp_buff->data_meta to overlap with area used by xdp_frame.

Thanks Jesper! Trying to answer both emails in one. :) More below.

> Fixes: 6dfb970d3dbd ("xdp: avoid leaking info stored in frame data on page reuse")
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  net/core/filter.c |   11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 15e9b5477360..e3623e741181 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2701,6 +2701,11 @@ BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
>  		     data > xdp->data_end - ETH_HLEN))
>  		return -EINVAL;
>  
> +	/* Disallow data_meta to use xdp_frame area */
> +	if (metalen > 0 &&
> +	    unlikely((data - metalen) < xdp_frame_end))
> +		return -EINVAL;
> +
>  	/* Avoid info leak, when reusing area prev used by xdp_frame */
>  	if (data < xdp_frame_end) {

Effectively, when metalen > 0, then data_meta < data pointer, so above test
on new data_meta might be better, but feels like a bit of a workaround to
handle moving data pointer but disallowing moving data_meta pointer whereas
both could be handled if we wanted to go that path.

>  		unsigned long clearlen = xdp_frame_end - data;
> @@ -2734,6 +2739,7 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {
>  
>  BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
>  {
> +	void *xdp_frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
>  	void *meta = xdp->data_meta + offset;
>  	unsigned long metalen = xdp->data - meta;
>  
> @@ -2742,6 +2748,11 @@ BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
>  	if (unlikely(meta < xdp->data_hard_start ||
>  		     meta > xdp->data))
>  		return -EINVAL;
> +
> +	/* Disallow data_meta to use xdp_frame area */
> +	if (unlikely(meta < xdp_frame_end))
> +		return -EINVAL;

(Ditto.)

>  	if (unlikely((metalen & (sizeof(__u32) - 1)) ||
>  		     (metalen > 32)))
>  		return -EACCES;

The other, perhaps less invasive/complex option would be to just disallow
moving anything into previous sizeof(struct xdp_frame) area. My original
concern was that not all drivers use 256 bytes of headroom, e.g. afaik the
i40e and ixgbe have around 192 bytes of headroom available, but that should
actually still be plenty of space for encap + meta data, and potentially
with meta data use I would expect that at best custom decap would be
happening when pushing the packet up the stack. So might as well disallow
going into that region and not worry about it. Thus, reverting e9e9545e10d3
("xdp: avoid leaking info stored in frame data on page reuse") and adding
something like the below (uncompiled), should just do it:

diff --git a/net/core/filter.c b/net/core/filter.c
index 3bb0cb9..ad98ddd 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2692,8 +2692,9 @@ static unsigned long xdp_get_metalen(const struct xdp_buff *xdp)

 BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset)
 {
+	void *frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
 	unsigned long metalen = xdp_get_metalen(xdp);
-	void *data_start = xdp->data_hard_start + metalen;
+	void *data_start = frame_end + metalen;
 	void *data = xdp->data + offset;

 	if (unlikely(data < data_start ||
@@ -2719,13 +2720,13 @@ static const struct bpf_func_proto bpf_xdp_adjust_head_proto = {

 BPF_CALL_2(bpf_xdp_adjust_meta, struct xdp_buff *, xdp, int, offset)
 {
+	void *frame_end = xdp->data_hard_start + sizeof(struct xdp_frame);
 	void *meta = xdp->data_meta + offset;
 	unsigned long metalen = xdp->data - meta;

 	if (xdp_data_meta_unsupported(xdp))
 		return -ENOTSUPP;
-	if (unlikely(meta < xdp->data_hard_start ||
-		     meta > xdp->data))
+	if (unlikely(meta < frame_end || meta > xdp->data))
 		return -EINVAL;
 	if (unlikely((metalen & (sizeof(__u32) - 1)) ||
 		     (metalen > 32)))

On top of that, we could even store a bool in struct xdp_rxq_info whether
the driver actually is able to participate in resp. has the XDP_REDIRECT
support and if not do something like:

void *frame_end = xdp->data_hard_start + xdp->rxq->has_redir ? sizeof(struct xdp_frame) : 0;

But the latter is merely a small optimization. Eventually we want all native XDP
drivers to support it. Thoughts?

Thanks,
Daniel