From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [RFC net-next PATCH 2/2] bpf: disallow XDP data_meta to overlap with xdp_frame area Date: Wed, 18 Apr 2018 22:28:39 +0200 Message-ID: References: <152405338404.30730.9846848505925123326.stgit@firesoul> <152405341684.30730.9208803786283211244.stgit@firesoul> <51af68d8-3c02-b828-12b5-f2dc73406a65@iogearbox.net> <20180418195322.381b041c@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: Daniel Borkmann , Alexei Starovoitov , netdev@vger.kernel.org To: Jesper Dangaard Brouer Return-path: Received: from www62.your-server.de ([213.133.104.62]:41248 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751972AbeDRU2m (ORCPT ); Wed, 18 Apr 2018 16:28:42 -0400 In-Reply-To: <20180418195322.381b041c@redhat.com> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: On 04/18/2018 07:53 PM, Jesper Dangaard Brouer wrote: > On Wed, 18 Apr 2018 18:21:21 +0200 > Daniel Borkmann wrote: > >> 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 >>> --- >>> 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))) > > Okay, so you say just disallow using xdp_frame area in general. It > would be simpler. Yeah, lets do that. > The advantage it that we don't run into strange situations, where the > user/bpf_prog increased headroom too much, such that convert_to_xdp_frame() > fails and thus XDP_REDIRECT action fails. (That will be confusing to > users to debug/troubleshoot). Agree, yet another argument for doing it.