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 <[email protected]>
> ---
> 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);
>
