[V2 PATCH net-next 0/2] Fixes for XDP_REDIRECT map

[V2 PATCH net-next 0/2] Fixes for XDP_REDIRECT map

[Jesper Dangaard Brouer]

This my V2 of catching XDP_REDIRECT and bpf_redirect_map() API usage
that can potentially crash the kernel.  Addressed Daniels feedback in
patch01, and added patch02 which catch and cleanup dangling map
pointers.

I know John and Daniel are working on a more long-term solution, of
recording the bpf_prog pointer together with the map pointer.  I just
wanted to propose these fixes as a stop-gap to the potential crashes.

---

Jesper Dangaard Brouer (2):
      xdp: implement xdp_redirect_map for generic XDP
      xdp: catch invalid XDP_REDIRECT API usage


 include/linux/filter.h     |    1 +
 include/trace/events/xdp.h |    4 ++--
 net/core/dev.c             |    3 +++
 net/core/filter.c          |   39 ++++++++++++++++++++++++++++++++++++---
 4 files changed, 42 insertions(+), 5 deletions(-)

[V2 PATCH net-next 1/2] xdp: implement xdp_redirect_map for generic XDP

[Jesper Dangaard Brouer]

Using bpf_redirect_map is allowed for generic XDP programs, but the
appropriate map lookup was never performed in xdp_do_generic_redirect().

Instead the map-index is directly used as the ifindex.  For the
xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
sending on ifindex 0 which isn't valid, resulting in getting SKB
packets dropped.  Thus, the reported performance numbers are wrong in
commit 24251c264798 ("samples/bpf: add option for native and skb mode
for redirect apps") for the 'xdp_redirect_map -S' case.

It might seem innocent this was lacking, but it can actually crash the
kernel.  The potential crash is caused by not consuming redirect_info->map.
The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
pointer, which will survive even after unloading the xdp bpf_prog and
deallocating the devmap data-structure.  This leaves a dead map
pointer around.  The kernel will crash when loading the xdp_redirect
sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
and returns XDP_REDIRECT, which will cause it to dereference the map
pointer.

Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/trace/events/xdp.h |    4 ++--
 net/core/filter.c          |   14 +++++++++++---
 2 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 862575ac8da9..4e16c43fba10 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -138,11 +138,11 @@ DEFINE_EVENT_PRINT(xdp_redirect_template, xdp_redirect_map_err,
 
 #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx)		\
 	 trace_xdp_redirect_map(dev, xdp, fwd ? fwd->ifindex : 0,	\
-				0, map, idx);
+				0, map, idx)
 
 #define _trace_xdp_redirect_map_err(dev, xdp, fwd, map, idx, err)	\
 	 trace_xdp_redirect_map_err(dev, xdp, fwd ? fwd->ifindex : 0,	\
-				    err, map, idx);
+				    err, map, idx)
 
 #endif /* _TRACE_XDP_H */
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 5912c738a7b2..3767470cab6c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2566,13 +2566,19 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 			    struct bpf_prog *xdp_prog)
 {
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
+	struct bpf_map *map = ri->map;
 	u32 index = ri->ifindex;
 	struct net_device *fwd;
 	unsigned int len;
 	int err = 0;
 
-	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
+	ri->map = NULL;
+
+	if (map)
+		fwd = __dev_map_lookup_elem(map, index);
+	else
+		fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	if (unlikely(!fwd)) {
 		err = -EINVAL;
 		goto err;
@@ -2590,10 +2596,12 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 	}
 
 	skb->dev = fwd;
-	_trace_xdp_redirect(dev, xdp_prog, index);
+	map ? _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index)
+		: _trace_xdp_redirect(dev, xdp_prog, index);
 	return 0;
 err:
-	_trace_xdp_redirect_err(dev, xdp_prog, index, err);
+	map ? _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err)
+		: _trace_xdp_redirect_err(dev, xdp_prog, index, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);

[V2 PATCH net-next 2/2] xdp: catch invalid XDP_REDIRECT API usage

[Jesper Dangaard Brouer]

Catch different invalid XDP_REDIRECT and bpf_redirect_map API usage.

It is fairly easy to create a dangling redirect_info->map pointer,
which (until John or Daniel fix this) can crash the kernel.

The intended usage of the BPF helper bpf_redirect_map(), is to return
XDP_REDIRECT action after invoking it, but there is nothing stopping
the bpf_prog to return anything else.  When XDP_REDIRECT isn't
returned, then a dangling ->map pointer is left behind, as
xdp_do_redirect() isn't called.

This also happens for drivers not implementing XDP_REDIRECT, as they
are not aware of this new XDP_REDIRECT return code, they leave the map
pointer dangling.

The simply solution to check for a dangling ->map pointer after each
driver napi->poll() invocation, see xdp_do_map_check().

This patch also add a check for a dangling ->map_to_flush pointer.
This should be considered a driver bug, as the driver contract is that
a pair of xdp_do_redirect and xdp_do_flush_map MUST be called in the
same cpu context.

Note, we need to check after each drivers napi->poll call, as:
 1. DevA poll call bpf_redirect_map() but not xdp_do_redirect()
 2. DevB bpf_prog uses bpf_redirect() and call xdp_do_redirect()
    which now use map from DevA

Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/linux/filter.h |    1 +
 net/core/dev.c         |    3 +++
 net/core/filter.c      |   25 +++++++++++++++++++++++++
 3 files changed, 29 insertions(+)

diff --git a/include/linux/filter.h b/include/linux/filter.h
index d29e58fde364..0c48941e0022 100644
--- a/include/linux/filter.h
+++ b/include/linux/filter.h
@@ -724,6 +724,7 @@ int xdp_do_redirect(struct net_device *dev,
 		    struct xdp_buff *xdp,
 		    struct bpf_prog *prog);
 void xdp_do_flush_map(void);
+void xdp_do_map_check(struct napi_struct *napi);
 
 void bpf_warn_invalid_xdp_action(u32 act);
 void bpf_warn_invalid_xdp_redirect(u32 ifindex);
diff --git a/net/core/dev.c b/net/core/dev.c
index 6f845e4fec17..7eac642b469f 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -5320,6 +5320,7 @@ static void busy_poll_stop(struct napi_struct *napi, void *have_poll_lock)
 	 */
 	rc = napi->poll(napi, BUSY_POLL_BUDGET);
 	trace_napi_poll(napi, rc, BUSY_POLL_BUDGET);
+	xdp_do_map_check(napi);
 	netpoll_poll_unlock(have_poll_lock);
 	if (rc == BUSY_POLL_BUDGET)
 		__napi_schedule(napi);
@@ -5367,6 +5368,7 @@ void napi_busy_loop(unsigned int napi_id,
 		}
 		work = napi_poll(napi, BUSY_POLL_BUDGET);
 		trace_napi_poll(napi, work, BUSY_POLL_BUDGET);
+		xdp_do_map_check(napi);
 count:
 		if (work > 0)
 			__NET_ADD_STATS(dev_net(napi->dev),
@@ -5529,6 +5531,7 @@ static int napi_poll(struct napi_struct *n, struct list_head *repoll)
 	if (test_bit(NAPI_STATE_SCHED, &n->state)) {
 		work = n->poll(n, weight);
 		trace_napi_poll(n, work, weight);
+		xdp_do_map_check(n);
 	}
 
 	WARN_ON_ONCE(work > weight);
diff --git a/net/core/filter.c b/net/core/filter.c
index 3767470cab6c..f0e1135eeb9d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2500,6 +2500,31 @@ void xdp_do_flush_map(void)
 }
 EXPORT_SYMBOL_GPL(xdp_do_flush_map);
 
+void xdp_do_map_check(struct napi_struct *napi)
+{
+	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
+
+	/* XDP drivers (and XDP-generic) must invoke xdp_do_redirect()
+	 * when bpf_prog use helper bpf_redirect_map(), else the map
+	 * pointer can be left dangling.  Catch this invalid API
+	 * usage, instead of potentially crashing.
+	 */
+	if (ri->map) {
+		ri->map = NULL;
+		net_err_ratelimited("%s: caught invalid XDP bpf_redirect_map\n",
+				    napi->dev->name);
+		trace_xdp_exception(napi->dev, NULL, XDP_REDIRECT);
+	}
+	if (ri->map_to_flush) { /* Driver bug */
+		net_err_ratelimited("%s: XDP driver miss xdp_do_flush_map\n",
+				    napi->dev->name);
+		trace_xdp_exception(napi->dev, NULL, XDP_REDIRECT);
+		/* Flush map, else pkts can be stuck on XDP TXq */
+		xdp_do_flush_map();
+	}
+}
+EXPORT_SYMBOL_GPL(xdp_do_map_check);
+
 static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
 			       struct bpf_prog *xdp_prog)
 {

Re: [V2 PATCH net-next 1/2] xdp: implement xdp_redirect_map for generic XDP

[Daniel Borkmann]

On 09/07/2017 02:33 PM, Jesper Dangaard Brouer wrote:
> Using bpf_redirect_map is allowed for generic XDP programs, but the
> appropriate map lookup was never performed in xdp_do_generic_redirect().
>
> Instead the map-index is directly used as the ifindex.  For the
> xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> sending on ifindex 0 which isn't valid, resulting in getting SKB
> packets dropped.  Thus, the reported performance numbers are wrong in
> commit 24251c264798 ("samples/bpf: add option for native and skb mode
> for redirect apps") for the 'xdp_redirect_map -S' case.
>
> It might seem innocent this was lacking, but it can actually crash the
> kernel.  The potential crash is caused by not consuming redirect_info->map.
> The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
> pointer, which will survive even after unloading the xdp bpf_prog and
> deallocating the devmap data-structure.  This leaves a dead map
> pointer around.  The kernel will crash when loading the xdp_redirect
> sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
> and returns XDP_REDIRECT, which will cause it to dereference the map
> pointer.
>
> Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> ---
>   include/trace/events/xdp.h |    4 ++--
>   net/core/filter.c          |   14 +++++++++++---
>   2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> index 862575ac8da9..4e16c43fba10 100644
> --- a/include/trace/events/xdp.h
> +++ b/include/trace/events/xdp.h
> @@ -138,11 +138,11 @@ DEFINE_EVENT_PRINT(xdp_redirect_template, xdp_redirect_map_err,
>
>   #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx)		\
>   	 trace_xdp_redirect_map(dev, xdp, fwd ? fwd->ifindex : 0,	\
> -				0, map, idx);
> +				0, map, idx)
>
>   #define _trace_xdp_redirect_map_err(dev, xdp, fwd, map, idx, err)	\
>   	 trace_xdp_redirect_map_err(dev, xdp, fwd ? fwd->ifindex : 0,	\
> -				    err, map, idx);
> +				    err, map, idx)
>
>   #endif /* _TRACE_XDP_H */
>
> diff --git a/net/core/filter.c b/net/core/filter.c
> index 5912c738a7b2..3767470cab6c 100644
> --- a/net/core/filter.c
> +++ b/net/core/filter.c
> @@ -2566,13 +2566,19 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>   			    struct bpf_prog *xdp_prog)
>   {
>   	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> +	struct bpf_map *map = ri->map;
>   	u32 index = ri->ifindex;
>   	struct net_device *fwd;
>   	unsigned int len;
>   	int err = 0;
>
> -	fwd = dev_get_by_index_rcu(dev_net(dev), index);
>   	ri->ifindex = 0;
> +	ri->map = NULL;
> +
> +	if (map)
> +		fwd = __dev_map_lookup_elem(map, index);
> +	else
> +		fwd = dev_get_by_index_rcu(dev_net(dev), index);
>   	if (unlikely(!fwd)) {
>   		err = -EINVAL;
>   		goto err;
> @@ -2590,10 +2596,12 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>   	}
>
>   	skb->dev = fwd;

Looks much better above, thanks!

> -	_trace_xdp_redirect(dev, xdp_prog, index);
> +	map ? _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index)
> +		: _trace_xdp_redirect(dev, xdp_prog, index);

Could we rather make this in a way such that when the two
tracepoints are disabled and thus patched out, that we can
also omit the extra conditional which has no purpose then?
Perhaps just a consolidated _trace_xdp_generic_redirect_map()
would be better to avoid this altogether given we have twice
the same anyway, here and in err path.

Thanks,
Daniel

>   	return 0;
>   err:
> -	_trace_xdp_redirect_err(dev, xdp_prog, index, err);
> +	map ? _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err)
> +		: _trace_xdp_redirect_err(dev, xdp_prog, index, err);
>   	return err;
>   }
>   EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);
>

Re: [V2 PATCH net-next 2/2] xdp: catch invalid XDP_REDIRECT API usage

[Daniel Borkmann]

Hey Jesper,

On 09/07/2017 02:33 PM, Jesper Dangaard Brouer wrote:
> Catch different invalid XDP_REDIRECT and bpf_redirect_map API usage.
>
> It is fairly easy to create a dangling redirect_info->map pointer,
> which (until John or Daniel fix this) can crash the kernel.
[...]

Here's what I wrote up yesterday, test looked good, feel
free to give it a spin as well if you want. Was planning
to submit it later today as official patch. Definitely less
intrusive than adding something to napi hot path impacting
everyone.

Cheers,
Daniel

 From 56ad381c87dcc2b32156c5970c75bf98818ea7f2 Mon Sep 17 00:00:00 2001
Message-Id: <56ad381c87dcc2b32156c5970c75bf98818ea7f2.1504790124.git.daniel@iogearbox.net>
From: Daniel Borkmann <daniel@iogearbox.net>
Date: Wed, 6 Sep 2017 21:21:27 +0200
Subject: [PATCH net] bpf: don't select potentially stale ri->map from buggy xdp progs

We can potentially run into a couple of issues with the XDP
bpf_redirect_map() helper. The ri->map in the per CPU storage
can become stale in several ways, mostly due to misuse, where
we can then trigger a use after free on the map:

i) prog A is calling bpf_redirect_map(), returning XDP_REDIRECT
and running on a driver not supporting XDP_REDIRECT yet. The
ri->map on that CPU becomes stale when the XDP program is unloaded
on the driver, and a prog B loaded on a different driver which
supports XDP_REDIRECT return code. prog B would have to omit
calling to bpf_redirect_map() and just return XDP_REDIRECT, which
would then access the freed map in xdp_do_redirect() since not
cleared for that CPU.

ii) prog A is calling bpf_redirect_map(), returning a code other
than XDP_REDIRECT. prog A is then detached, which triggers release
of the map. prog B is attached which, similarly as in i), would
just return XDP_REDIRECT without having called bpf_redirect_map()
and thus be accessing the freed map in xdp_do_redirect() since
not cleared for that CPU.

iii) prog A is attached to generic XDP, calling the bpf_redirect_map()
helper and returning XDP_REDIRECT. xdp_do_generic_redirect() is
currently not handling ri->map (will be fixed by Jesper), so it's
not being reset. Later loading a e.g. native prog B which would,
say, call bpf_xdp_redirect() and then returns XDP_REDIRECT would
find in xdp_do_redirect() that a map was set and uses that causing
use after free on map access.

Fix thus needs to avoid accessing stale ri->map pointers, naive
way would be to call a BPF function from drivers that just resets
it to NULL for all XDP return codes but XDP_REDIRECT and including
XDP_REDIRECT for drivers not supporting it yet (and let ri->map
being handled in xdp_do_generic_redirect()). There is a less
intrusive way w/o letting drivers call a reset for each BPF run.

The verifier knows we're calling into bpf_xdp_redirect_map()
helper, so it can do a small insn rewrite transparent to the prog
itself in the sense that it fills R4 with a pointer to the own
bpf_prog. We have that pointer at verification time anyway and
R4 is allowed to be used as per calling convention we scratch
R0 to R5 anyway, so they become inaccessible and program cannot
read them prior to a write. Then, the helper would store the prog
pointer in the current CPUs struct redirect_info. Later in
xdp_do_*_redirect() we check whether the redirect_info's prog
pointer is the same as passed xdp_prog pointer, and if that's
the case then all good, since the prog holds a ref on the map
anyway, so it is always valid at that point in time and must
have a reference count of at least 1. If in the unlikely case
they are not equal, it means we got a stale pointer, so we clear
and bail out right there. Also do reset map and the owning prog
in bpf_xdp_redirect(), so that bpf_xdp_redirect_map() and
bpf_xdp_redirect() won't get mixed up, only the last call should
take precedence. A tc bpf_redirect() doesn't use map anywhere
yet, so no need to clear it there since never accessed in that
layer.

Note that in case the prog is released, and thus the map as
well we're still under RCU read critical section at that time
and have preemption disabled as well. Once we commit with the
__dev_map_insert_ctx() from xdp_do_redirect_map() and set the
map to ri->map_to_flush, we still wait for a xdp_do_flush_map()
to finish in devmap dismantle time once flush_needed bit is set,
so that is fine.

Fixes: 97f91a7cf04f ("bpf: add bpf_redirect_map helper routine")
Reported-by: Jesper Dangaard Brouer <brouer@redhat.com>
Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
Signed-off-by: John Fastabend <john.fastabend@gmail.com>
---
  kernel/bpf/verifier.c | 16 ++++++++++++++++
  net/core/filter.c     | 21 +++++++++++++++++++--
  2 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index d690c7d..af13987 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -4203,6 +4203,22 @@ static int fixup_bpf_calls(struct bpf_verifier_env *env)
  			continue;
  		}

+		if (insn->imm == BPF_FUNC_redirect_map) {
+			uint64_t addr = (unsigned long)prog;
+			struct bpf_insn r4_ld[] = {
+				BPF_LD_IMM64(BPF_REG_4, addr),
+				*insn,
+			};
+			cnt = ARRAY_SIZE(r4_ld);
+
+			new_prog = bpf_patch_insn_data(env, i + delta, r4_ld, cnt);
+			if (!new_prog)
+				return -ENOMEM;
+
+			delta    += cnt - 1;
+			env->prog = prog = new_prog;
+			insn      = new_prog->insnsi + i + delta;
+		}
  patch_call_imm:
  		fn = prog->aux->ops->get_func_proto(insn->imm);
  		/* all functions that have prototype and verifier allowed
diff --git a/net/core/filter.c b/net/core/filter.c
index 5912c73..0848df2 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1794,6 +1794,7 @@ struct redirect_info {
  	u32 flags;
  	struct bpf_map *map;
  	struct bpf_map *map_to_flush;
+	const struct bpf_prog *map_owner;
  };

  static DEFINE_PER_CPU(struct redirect_info, redirect_info);
@@ -1807,7 +1808,6 @@ struct redirect_info {

  	ri->ifindex = ifindex;
  	ri->flags = flags;
-	ri->map = NULL;

  	return TC_ACT_REDIRECT;
  }
@@ -2504,6 +2504,7 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
  			       struct bpf_prog *xdp_prog)
  {
  	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
+	const struct bpf_prog *map_owner = ri->map_owner;
  	struct bpf_map *map = ri->map;
  	u32 index = ri->ifindex;
  	struct net_device *fwd;
@@ -2511,6 +2512,15 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,

  	ri->ifindex = 0;
  	ri->map = NULL;
+	ri->map_owner = NULL;
+
+	/* This is really only caused by a deliberately crappy
+	 * BPF program, normally we would never hit that case,
+	 * so no need to inform someone via tracepoints either,
+	 * just bail out.
+	 */
+	if (unlikely(map_owner != xdp_prog))
+		return -EINVAL;

  	fwd = __dev_map_lookup_elem(map, index);
  	if (!fwd) {
@@ -2607,6 +2617,8 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,

  	ri->ifindex = ifindex;
  	ri->flags = flags;
+	ri->map = NULL;
+	ri->map_owner = NULL;

  	return XDP_REDIRECT;
  }
@@ -2619,7 +2631,8 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
  	.arg2_type      = ARG_ANYTHING,
  };

-BPF_CALL_3(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex, u64, flags)
+BPF_CALL_4(bpf_xdp_redirect_map, struct bpf_map *, map, u32, ifindex, u64, flags,
+	   const struct bpf_prog *, map_owner)
  {
  	struct redirect_info *ri = this_cpu_ptr(&redirect_info);

@@ -2629,10 +2642,14 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
  	ri->ifindex = ifindex;
  	ri->flags = flags;
  	ri->map = map;
+	ri->map_owner = map_owner;

  	return XDP_REDIRECT;
  }

+/* Note, arg4 is hidden from users and populated by the verifier
+ * with the right pointer.
+ */
  static const struct bpf_func_proto bpf_xdp_redirect_map_proto = {
  	.func           = bpf_xdp_redirect_map,
  	.gpl_only       = false,
-- 
1.9.3

Re: [V2 PATCH net-next 2/2] xdp: catch invalid XDP_REDIRECT API usage

[Daniel Borkmann]

On 09/07/2017 04:13 PM, Daniel Borkmann wrote:
[...]
> +            uint64_t addr = (unsigned long)prog;

And of course: s/uint64_t/u64/. Lack of context switch. ;-)

Re: [V2 PATCH net-next 1/2] xdp: implement xdp_redirect_map for generic XDP

[Jesper Dangaard Brouer]

On Thu, 07 Sep 2017 16:09:56 +0200
Daniel Borkmann <daniel@iogearbox.net> wrote:

> On 09/07/2017 02:33 PM, Jesper Dangaard Brouer wrote:
> > Using bpf_redirect_map is allowed for generic XDP programs, but the
> > appropriate map lookup was never performed in xdp_do_generic_redirect().
> >
> > Instead the map-index is directly used as the ifindex.  For the
> > xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> > sending on ifindex 0 which isn't valid, resulting in getting SKB
> > packets dropped.  Thus, the reported performance numbers are wrong in
> > commit 24251c264798 ("samples/bpf: add option for native and skb mode
> > for redirect apps") for the 'xdp_redirect_map -S' case.
> >
> > It might seem innocent this was lacking, but it can actually crash the
> > kernel.  The potential crash is caused by not consuming redirect_info->map.
> > The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
> > pointer, which will survive even after unloading the xdp bpf_prog and
> > deallocating the devmap data-structure.  This leaves a dead map
> > pointer around.  The kernel will crash when loading the xdp_redirect
> > sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
> > and returns XDP_REDIRECT, which will cause it to dereference the map
> > pointer.
> >
> > Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> > Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> >   include/trace/events/xdp.h |    4 ++--
> >   net/core/filter.c          |   14 +++++++++++---
> >   2 files changed, 13 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
> > index 862575ac8da9..4e16c43fba10 100644
> > --- a/include/trace/events/xdp.h
> > +++ b/include/trace/events/xdp.h
> > @@ -138,11 +138,11 @@ DEFINE_EVENT_PRINT(xdp_redirect_template, xdp_redirect_map_err,
> >
> >   #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx)		\
> >   	 trace_xdp_redirect_map(dev, xdp, fwd ? fwd->ifindex : 0,	\
> > -				0, map, idx);
> > +				0, map, idx)
> >
> >   #define _trace_xdp_redirect_map_err(dev, xdp, fwd, map, idx, err)	\
> >   	 trace_xdp_redirect_map_err(dev, xdp, fwd ? fwd->ifindex : 0,	\
> > -				    err, map, idx);
> > +				    err, map, idx)
> >
> >   #endif /* _TRACE_XDP_H */
> >
> > diff --git a/net/core/filter.c b/net/core/filter.c
> > index 5912c738a7b2..3767470cab6c 100644
> > --- a/net/core/filter.c
> > +++ b/net/core/filter.c
> > @@ -2566,13 +2566,19 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> >   			    struct bpf_prog *xdp_prog)
> >   {
> >   	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
> > +	struct bpf_map *map = ri->map;
> >   	u32 index = ri->ifindex;
> >   	struct net_device *fwd;
> >   	unsigned int len;
> >   	int err = 0;
> >
> > -	fwd = dev_get_by_index_rcu(dev_net(dev), index);
> >   	ri->ifindex = 0;
> > +	ri->map = NULL;
> > +
> > +	if (map)
> > +		fwd = __dev_map_lookup_elem(map, index);
> > +	else
> > +		fwd = dev_get_by_index_rcu(dev_net(dev), index);
> >   	if (unlikely(!fwd)) {
> >   		err = -EINVAL;
> >   		goto err;
> > @@ -2590,10 +2596,12 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
> >   	}
> >
> >   	skb->dev = fwd;  
> 
> Looks much better above, thanks!
> 
> > -	_trace_xdp_redirect(dev, xdp_prog, index);
> > +	map ? _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index)
> > +		: _trace_xdp_redirect(dev, xdp_prog, index);  
> 
> Could we rather make this in a way such that when the two
> tracepoints are disabled and thus patched out, that we can
> also omit the extra conditional which has no purpose then?

First of all I don't think it make much of a difference, I measured the
impact of the full patch to "cost" 1.62 nanosec (which is arguably
below the accuracy level of the system under test)

Secondly, I plan to optimize the map case for generic XDP later, where
I would naturally split this into two functions (as V1, and as
native-XDP), thus this extra conditional would go away.  As I've shown
offlist (to you, John and Andy) I demonstrated a 24% speedup via a
xmit_more hack for generic XDP.


> Perhaps just a consolidated _trace_xdp_generic_redirect_map()
> would be better to avoid this altogether given we have twice
> the same anyway, here and in err path.

I do want separate tracepoints for xdp_redirect and xdp_redirect_map,
as it makes it more clear for users of the tracepoint (and attached
bpf_prog's can be faster, knowing the context).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer

Re: [V2 PATCH net-next 1/2] xdp: implement xdp_redirect_map for generic XDP

[Daniel Borkmann]

On 09/08/2017 10:36 AM, Jesper Dangaard Brouer wrote:
> On Thu, 07 Sep 2017 16:09:56 +0200
> Daniel Borkmann <daniel@iogearbox.net> wrote:
>> On 09/07/2017 02:33 PM, Jesper Dangaard Brouer wrote:
>>> Using bpf_redirect_map is allowed for generic XDP programs, but the
>>> appropriate map lookup was never performed in xdp_do_generic_redirect().
>>>
>>> Instead the map-index is directly used as the ifindex.  For the
>>> xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
>>> sending on ifindex 0 which isn't valid, resulting in getting SKB
>>> packets dropped.  Thus, the reported performance numbers are wrong in
>>> commit 24251c264798 ("samples/bpf: add option for native and skb mode
>>> for redirect apps") for the 'xdp_redirect_map -S' case.
>>>
>>> It might seem innocent this was lacking, but it can actually crash the
>>> kernel.  The potential crash is caused by not consuming redirect_info->map.
>>> The bpf_redirect_map helper will set this_cpu_ptr(&redirect_info)->map
>>> pointer, which will survive even after unloading the xdp bpf_prog and
>>> deallocating the devmap data-structure.  This leaves a dead map
>>> pointer around.  The kernel will crash when loading the xdp_redirect
>>> sample (in native XDP mode) as it doesn't reset map (via bpf_redirect)
>>> and returns XDP_REDIRECT, which will cause it to dereference the map
>>> pointer.
>>>
>>> Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
>>> Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
>>> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
>>> ---
>>>    include/trace/events/xdp.h |    4 ++--
>>>    net/core/filter.c          |   14 +++++++++++---
>>>    2 files changed, 13 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
>>> index 862575ac8da9..4e16c43fba10 100644
>>> --- a/include/trace/events/xdp.h
>>> +++ b/include/trace/events/xdp.h
>>> @@ -138,11 +138,11 @@ DEFINE_EVENT_PRINT(xdp_redirect_template, xdp_redirect_map_err,
>>>
>>>    #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx)		\
>>>    	 trace_xdp_redirect_map(dev, xdp, fwd ? fwd->ifindex : 0,	\
>>> -				0, map, idx);
>>> +				0, map, idx)
>>>
>>>    #define _trace_xdp_redirect_map_err(dev, xdp, fwd, map, idx, err)	\
>>>    	 trace_xdp_redirect_map_err(dev, xdp, fwd ? fwd->ifindex : 0,	\
>>> -				    err, map, idx);
>>> +				    err, map, idx)
>>>
>>>    #endif /* _TRACE_XDP_H */
>>>
>>> diff --git a/net/core/filter.c b/net/core/filter.c
>>> index 5912c738a7b2..3767470cab6c 100644
>>> --- a/net/core/filter.c
>>> +++ b/net/core/filter.c
>>> @@ -2566,13 +2566,19 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>    			    struct bpf_prog *xdp_prog)
>>>    {
>>>    	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
>>> +	struct bpf_map *map = ri->map;
>>>    	u32 index = ri->ifindex;
>>>    	struct net_device *fwd;
>>>    	unsigned int len;
>>>    	int err = 0;
>>>
>>> -	fwd = dev_get_by_index_rcu(dev_net(dev), index);
>>>    	ri->ifindex = 0;
>>> +	ri->map = NULL;
>>> +
>>> +	if (map)
>>> +		fwd = __dev_map_lookup_elem(map, index);
>>> +	else
>>> +		fwd = dev_get_by_index_rcu(dev_net(dev), index);
>>>    	if (unlikely(!fwd)) {
>>>    		err = -EINVAL;
>>>    		goto err;
>>> @@ -2590,10 +2596,12 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
>>>    	}
>>>
>>>    	skb->dev = fwd;
>>
>> Looks much better above, thanks!
>>
>>> -	_trace_xdp_redirect(dev, xdp_prog, index);
>>> +	map ? _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index)
>>> +		: _trace_xdp_redirect(dev, xdp_prog, index);
>>
>> Could we rather make this in a way such that when the two
>> tracepoints are disabled and thus patched out, that we can
>> also omit the extra conditional which has no purpose then?
>
> First of all I don't think it make much of a difference, I measured the
> impact of the full patch to "cost" 1.62 nanosec (which is arguably
> below the accuracy level of the system under test)
>
> Secondly, I plan to optimize the map case for generic XDP later, where
> I would naturally split this into two functions (as V1, and as
> native-XDP), thus this extra conditional would go away.  As I've shown
> offlist (to you, John and Andy) I demonstrated a 24% speedup via a
> xmit_more hack for generic XDP.

Okay, that would be nice indeed to have xmit_more support for
generic XDP as well. If this is going to be split off anyway
later on as in xdp_do_redirect() case, then:

Acked-by: Daniel Borkmann <daniel@iogearbox.net>

Re: [V2 PATCH net-next 0/2] Fixes for XDP_REDIRECT map

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Thu, 07 Sep 2017 14:33:08 +0200

> This my V2 of catching XDP_REDIRECT and bpf_redirect_map() API usage
> that can potentially crash the kernel.  Addressed Daniels feedback in
> patch01, and added patch02 which catch and cleanup dangling map
> pointers.
> 
> I know John and Daniel are working on a more long-term solution, of
> recording the bpf_prog pointer together with the map pointer.  I just
> wanted to propose these fixes as a stop-gap to the potential crashes.

Jesper if these are still relevant, please resubmit against the 'net'
tree, thanks!

[V3 PATCH net] xdp: implement xdp_redirect_map for generic XDP

[Jesper Dangaard Brouer]

Using bpf_redirect_map is allowed for generic XDP programs, but the
appropriate map lookup was never performed in xdp_do_generic_redirect().

Instead the map-index is directly used as the ifindex.  For the
xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
sending on ifindex 0 which isn't valid, resulting in getting SKB
packets dropped.  Thus, the reported performance numbers are wrong in
commit 24251c264798 ("samples/bpf: add option for native and skb mode
for redirect apps") for the 'xdp_redirect_map -S' case.

Before commit 109980b894e9 ("bpf: don't select potentially stale
ri->map from buggy xdp progs") it could crash the kernel.  Like this
commit also check that the map_owner owner is correct before
dereferencing the map pointer.  But make sure that this API misusage
can be caught by a tracepoint. Thus, allowing userspace via
tracepoints to detect misbehaving bpf_progs.

Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
---
 include/trace/events/xdp.h |    4 ++--
 net/core/filter.c          |   38 ++++++++++++++++++++++++++------------
 2 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/include/trace/events/xdp.h b/include/trace/events/xdp.h
index 862575ac8da9..4e16c43fba10 100644
--- a/include/trace/events/xdp.h
+++ b/include/trace/events/xdp.h
@@ -138,11 +138,11 @@ DEFINE_EVENT_PRINT(xdp_redirect_template, xdp_redirect_map_err,
 
 #define _trace_xdp_redirect_map(dev, xdp, fwd, map, idx)		\
 	 trace_xdp_redirect_map(dev, xdp, fwd ? fwd->ifindex : 0,	\
-				0, map, idx);
+				0, map, idx)
 
 #define _trace_xdp_redirect_map_err(dev, xdp, fwd, map, idx, err)	\
 	 trace_xdp_redirect_map_err(dev, xdp, fwd ? fwd->ifindex : 0,	\
-				    err, map, idx);
+				    err, map, idx)
 
 #endif /* _TRACE_XDP_H */
 
diff --git a/net/core/filter.c b/net/core/filter.c
index 3a50a9b021e2..24dd33dd9f04 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -2506,21 +2506,19 @@ static int xdp_do_redirect_map(struct net_device *dev, struct xdp_buff *xdp,
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
 	const struct bpf_prog *map_owner = ri->map_owner;
 	struct bpf_map *map = ri->map;
+	struct net_device *fwd = NULL;
 	u32 index = ri->ifindex;
-	struct net_device *fwd;
 	int err;
 
 	ri->ifindex = 0;
 	ri->map = NULL;
 	ri->map_owner = NULL;
 
-	/* This is really only caused by a deliberately crappy
-	 * BPF program, normally we would never hit that case,
-	 * so no need to inform someone via tracepoints either,
-	 * just bail out.
-	 */
-	if (unlikely(map_owner != xdp_prog))
-		return -EINVAL;
+	if (unlikely(map_owner != xdp_prog)) {
+		err = -EFAULT;
+		map = NULL;
+		goto err;
+	}
 
 	fwd = __dev_map_lookup_elem(map, index);
 	if (!fwd) {
@@ -2576,13 +2574,27 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 			    struct bpf_prog *xdp_prog)
 {
 	struct redirect_info *ri = this_cpu_ptr(&redirect_info);
+	const struct bpf_prog *map_owner = ri->map_owner;
+	struct bpf_map *map = ri->map;
+	struct net_device *fwd = NULL;
 	u32 index = ri->ifindex;
-	struct net_device *fwd;
 	unsigned int len;
 	int err = 0;
 
-	fwd = dev_get_by_index_rcu(dev_net(dev), index);
 	ri->ifindex = 0;
+	ri->map = NULL;
+	ri->map_owner = NULL;
+
+	if (map) {
+		if (unlikely(map_owner != xdp_prog)) {
+			err = -EFAULT;
+			map = NULL;
+			goto err;
+		}
+		fwd = __dev_map_lookup_elem(map, index);
+	} else {
+		fwd = dev_get_by_index_rcu(dev_net(dev), index);
+	}
 	if (unlikely(!fwd)) {
 		err = -EINVAL;
 		goto err;
@@ -2600,10 +2612,12 @@ int xdp_do_generic_redirect(struct net_device *dev, struct sk_buff *skb,
 	}
 
 	skb->dev = fwd;
-	_trace_xdp_redirect(dev, xdp_prog, index);
+	map ? _trace_xdp_redirect_map(dev, xdp_prog, fwd, map, index)
+		: _trace_xdp_redirect(dev, xdp_prog, index);
 	return 0;
 err:
-	_trace_xdp_redirect_err(dev, xdp_prog, index, err);
+	map ? _trace_xdp_redirect_map_err(dev, xdp_prog, fwd, map, index, err)
+		: _trace_xdp_redirect_err(dev, xdp_prog, index, err);
 	return err;
 }
 EXPORT_SYMBOL_GPL(xdp_do_generic_redirect);

Re: [V3 PATCH net] xdp: implement xdp_redirect_map for generic XDP

[David Miller]

From: Jesper Dangaard Brouer <brouer@redhat.com>
Date: Sun, 10 Sep 2017 09:47:02 +0200

> Using bpf_redirect_map is allowed for generic XDP programs, but the
> appropriate map lookup was never performed in xdp_do_generic_redirect().
> 
> Instead the map-index is directly used as the ifindex.  For the
> xdp_redirect_map sample in SKB-mode '-S', this resulted in trying
> sending on ifindex 0 which isn't valid, resulting in getting SKB
> packets dropped.  Thus, the reported performance numbers are wrong in
> commit 24251c264798 ("samples/bpf: add option for native and skb mode
> for redirect apps") for the 'xdp_redirect_map -S' case.
> 
> Before commit 109980b894e9 ("bpf: don't select potentially stale
> ri->map from buggy xdp progs") it could crash the kernel.  Like this
> commit also check that the map_owner owner is correct before
> dereferencing the map pointer.  But make sure that this API misusage
> can be caught by a tracepoint. Thus, allowing userspace via
> tracepoints to detect misbehaving bpf_progs.
> 
> Fixes: 6103aa96ec07 ("net: implement XDP_REDIRECT for xdp generic")
> Fixes: 24251c264798 ("samples/bpf: add option for native and skb mode for redirect apps")
> Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>

Applied, thanks Jesper.