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

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@redhat.com>
---
 net/core/filter.c |    8 ++++++++
 1 file changed, 8 insertions(+)

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@redhat.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);
>