From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [net-next V11 PATCH 17/17] xdp: avoid leaking info stored in frame data on page reuse Date: Tue, 17 Apr 2018 17:04:07 +0200 Message-ID: <8c47dbc6-b52d-e341-9fad-595638ec5688@iogearbox.net> References: <152397622657.20272.10121948713784224943.stgit@firesoul> <152397640301.20272.9781402055431898663.stgit@firesoul> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: eugenia@mellanox.com, Jason Wang , John Fastabend , Eran Ben Elisha , Saeed Mahameed , galp@mellanox.com, Alexei Starovoitov , Tariq Toukan To: Jesper Dangaard Brouer , netdev@vger.kernel.org, =?UTF-8?B?QmrDtnJuVMO2cGVs?= , magnus.karlsson@intel.com Return-path: Received: from www62.your-server.de ([213.133.104.62]:59929 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752703AbeDQPEL (ORCPT ); Tue, 17 Apr 2018 11:04:11 -0400 In-Reply-To: <152397640301.20272.9781402055431898663.stgit@firesoul> Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: Hi Jesper, On 04/17/2018 04:46 PM, Jesper Dangaard Brouer wrote: > The bpf infrastructure and verifier goes to great length to avoid > bpf progs leaking kernel (pointer) info. > > For queueing an xdp_buff via XDP_REDIRECT, xdp_frame info stores > kernel info (incl pointers) in top part of frame data (xdp->data_hard_start). > Checks are in place to assure enough headroom is available for this. > > This info is not cleared, and if the frame is reused, then a > malicious user could use bpf_xdp_adjust_head helper to move > xdp->data into this area. Thus, making this area readable. > > This is not super critical as XDP progs requires root or > CAP_SYS_ADMIN, which are privileged enough for such info. An > effort (is underway) towards moving networking bpf hooks to the > lesser privileged mode CAP_NET_ADMIN, where leaking such info > should be avoided. Thus, this patch to clear the info when > needed. > > Signed-off-by: Jesper Dangaard Brouer > --- > net/core/filter.c | 8 ++++++++ > 1 file changed, 8 insertions(+) > > diff --git a/net/core/filter.c b/net/core/filter.c > index 3bb0cb98a9be..a374b8560bc4 100644 > --- a/net/core/filter.c > +++ b/net/core/filter.c > @@ -2692,6 +2692,7 @@ 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 *xdp_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 = xdp->data + offset; > @@ -2700,6 +2701,13 @@ BPF_CALL_2(bpf_xdp_adjust_head, struct xdp_buff *, xdp, int, offset) > data > xdp->data_end - ETH_HLEN)) > return -EINVAL; > > + /* Avoid info leak, when reusing area prev used by xdp_frame */ > + if (data < xdp_frame_end) { > + unsigned long clearlen = xdp_frame_end - data; > + > + memset(data, 0, clearlen); > + } > + Sorry, but this patch is actually incomplete: bpf_xdp_adjust_meta() needs the same treatment, otherwise there is no point in clearing here, but not when using meta data. But also I think this is a bit suboptimal resolved in general (sorry, haven't had a chance to review this particular patch earlier as I was swamped). What happens when you adjust head for the data, where you first cleared this frame data, but then later on in the program you would adjust data_meta pointer, which also falls under the 'data{,_meta} < xdp_frame_end' condition. Then, you are going to clear again valid data that the user wrote to the packet before. > if (metalen) > memmove(xdp->data_meta + offset, > xdp->data_meta, metalen); >