netdev - Re: [net-next V11 PATCH 17/17] xdp: avoid leaking info stored in frame data on page reuse

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]

Date:   Tue, 17 Apr 2018 17:04:07 +0200
From:   Daniel Borkmann <borkmann@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>, netdev@...r.kernel.org,
        BjörnTöpel <bjorn.topel@...el.com>,
        magnus.karlsson@...el.com
Cc:     eugenia@...lanox.com, Jason Wang <jasowang@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>, galp@...lanox.com,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [net-next V11 PATCH 17/17] xdp: avoid leaking info stored in
 frame data on page reuse

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 <brouer@...hat.com>
> ---
>  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);
>