Skip to contentSkip to conversation

bpf: couple of fixes

PATCH In review Daniel Borkmann 2 participants 4 messages Jun 10, 2016 – Jun 11, 2016

Subsystems (4)
Networking Network drivers BPF BPF networking
lore
Share & tools
lore

Daniel Borkmann

author maintainer
#0 lore raw

cover bpf: couple of fixes

These are two fixes for BPF, one to introduce xmit recursion limiter for tc bpf programs and the other one to reject filters a bit earlier. For more details please see individual patches. I have no strong opinion to which tree they should go, they apply to both, but I think net-next seems okay to me.

Thanks!

Daniel Borkmann (2):
bpf: enforce recursion limit on redirects
bpf: reject wrong sized filters earlier

 include/linux/netdevice.h |  2 ++
 net/core/dev.c            |  6 ++--
 net/core/filter.c         | 78 +++++++++++++++++++++++++++++------------------
 3 files changed, 53 insertions(+), 33 deletions(-)
Signature · 1 line
1.9.3
author maintainer
#1 lore raw

PATCH 1/2 bpf: enforce recursion limit on redirects

Respect the stack's xmit_recursion limit for calls into dev_queue_xmit(). Currently, they are not handeled by the limiter when attached to clsact's egress parent, for example, and a buggy program redirecting it to the same device again could run into stack overflow eventually. It would be good if we could notify an admin to give him a chance to react. We reuse xmit_recursion instead of having one private to eBPF, so that the stack's current recursion depth will be taken into account as well. Follow-up to commit 3896d655f4d4 ("bpf: introduce bpf_clone_redirect() helper") and 27b29f63058d ("bpf: add bpf_redirect() helper").

Signed-off-by Daniel Borkmann <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            |  6 ++----
 net/core/filter.c         | 55 +++++++++++++++++++++++++++++------------------
 3 files changed, 38 insertions(+), 25 deletions(-)
include/linux/netdevice.h +2 -0
index 4f234b1..94eef35 100644
@@ -2389,6 +2389,8 @@ void synchronize_net(void);
 int init_dummy_netdev(struct net_device *dev);
 
 DECLARE_PER_CPU(int, xmit_recursion);
+#define XMIT_RECURSION_LIMIT	10
+
 static inline int dev_recursion_level(void)
 {
 	return this_cpu_read(xmit_recursion);
net/core/dev.c +2 -4
index c43c9d2..b148357 100644
@@ -3144,8 +3144,6 @@ static void skb_update_prio(struct sk_buff *skb)
 DEFINE_PER_CPU(int, xmit_recursion);
 EXPORT_SYMBOL(xmit_recursion);
 
-#define RECURSION_LIMIT 10
-
 /**
  *	dev_loopback_xmit - loop back @skb
  *	@net: network namespace this loopback is happening in

@@ -3388,8 +3386,8 @@ static int __dev_queue_xmit(struct sk_buff *skb, void *accel_priv)
 		int cpu = smp_processor_id(); /* ok because BHs are off */
 
 		if (txq->xmit_lock_owner != cpu) {
-
-			if (__this_cpu_read(xmit_recursion) > RECURSION_LIMIT)
+			if (unlikely(__this_cpu_read(xmit_recursion) >
+				     XMIT_RECURSION_LIMIT))
 				goto recursion_alert;
 
 			skb = validate_xmit_skb(skb, dev);
net/core/filter.c +34 -21

77 lines not on this page · raw message

Signature · 1 line
1.9.3
author maintainer
#2 lore raw

PATCH 2/2 bpf: reject wrong sized filters earlier

In reply to Daniel Borkmann

Add a bpf_check_basics_ok() and reject filters that are of invalid size much earlier, so we don't do any useless work such as invoking bpf_prog_alloc(). Currently, rejection happens in bpf_check_classic() only, but it's really unnecessarily late and they should be rejected at earliest point. While at it, also clean up one bpf_prog_size() to make it consistent with the remaining invocations.

Signed-off-by Daniel Borkmann <[email protected]>
Acked-by Alexei Starovoitov <[email protected]>
---
 net/core/filter.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)
net/core/filter.c +15 -8
index d11744d..df6860c 100644
@@ -748,6 +748,17 @@ static bool chk_code_allowed(u16 code_to_probe)
 	return codes[code_to_probe];
 }
 
+static bool bpf_check_basics_ok(const struct sock_filter *filter,
+				unsigned int flen)
+{
+	if (filter == NULL)
+		return false;
+	if (flen == 0 || flen > BPF_MAXINSNS)
+		return false;
+
+	return true;
+}
+
 /**
  *	bpf_check_classic - verify socket filter code
  *	@filter: filter to verify

@@ -768,9 +779,6 @@ static int bpf_check_classic(const struct sock_filter *filter,
 	bool anc_found;
 	int pc;
 
-	if (flen == 0 || flen > BPF_MAXINSNS)
-		return -EINVAL;
-
 	/* Check the filter code now */
 	for (pc = 0; pc < flen; pc++) {
 		const struct sock_filter *ftest = &filter[pc];

@@ -1065,7 +1073,7 @@ int bpf_prog_create(struct bpf_prog **pfp, struct sock_fprog_kern *fprog)
 	struct bpf_prog *fp;
 
 	/* Make sure new filter is there and in the right amounts. */
-	if (fprog->filter == NULL)
+	if (!bpf_check_basics_ok(fprog->filter, fprog->len))
 		return -EINVAL;
 
 	fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0);

@@ -1112,7 +1120,7 @@ int bpf_prog_create_from_user(struct bpf_prog **pfp, struct sock_fprog *fprog,
 	int err;
 
 	/* Make sure new filter is there and in the right amounts. */
-	if (fprog->filter == NULL)
+	if (!bpf_check_basics_ok(fprog->filter, fprog->len))
 		return -EINVAL;
 
 	fp = bpf_prog_alloc(bpf_prog_size(fprog->len), 0);

@@ -1207,7 +1215,6 @@ static
 struct bpf_prog *__get_filter(struct sock_fprog *fprog, struct sock *sk)
 {
 	unsigned int fsize = bpf_classic_proglen(fprog);
-	unsigned int bpf_fsize = bpf_prog_size(fprog->len);
 	struct bpf_prog *prog;
 	int err;
 

@@ -1215,10 +1222,10 @@ struct bpf_prog *__get_filter(struct sock_fprog *fprog, struct sock *sk)
 		return ERR_PTR(-EPERM);
 
 	/* Make sure new filter is there and in the right amounts. */
-	if (fprog->filter == NULL)
+	if (!bpf_check_basics_ok(fprog->filter, fprog->len))
 		return ERR_PTR(-EINVAL);
 
-	prog = bpf_prog_alloc(bpf_fsize, 0);
+	prog = bpf_prog_alloc(bpf_prog_size(fprog->len), 0);
 	if (!prog)
 		return ERR_PTR(-ENOMEM);
Signature · 1 line
1.9.3
maintainer
#3 lore raw
In reply to Daniel Borkmann

From: Daniel Borkmann <[email protected]>
Date: Fri, 10 Jun 2016 21:19:05 +0200

Quoted 5 lines

These are two fixes for BPF, one to introduce xmit recursion limiter for tc bpf programs and the other one to reject filters a bit earlier. For more details please see individual patches. I have no strong opinion to which tree they should go, they apply to both, but I think net-next seems okay to me.

Series applied to net-next, thanks Daniel.