All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/2] bpf: couple of fixes
@ 2016-06-10 19:19 Daniel Borkmann
  2016-06-10 19:19 ` [PATCH net-next 1/2] bpf: enforce recursion limit on redirects Daniel Borkmann
                   ` (2 more replies)
  0 siblings, 3 replies; 4+ messages in thread
From: Daniel Borkmann @ 2016-06-10 19:19 UTC (permalink / raw)
  To: davem; +Cc: alexei.starovoitov, hannes, netdev, Daniel Borkmann

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(-)

-- 
1.9.3

^ permalink raw reply	[flat|nested] 4+ messages in thread

* [PATCH net-next 1/2] bpf: enforce recursion limit on redirects
  2016-06-10 19:19 [PATCH net-next 0/2] bpf: couple of fixes Daniel Borkmann
@ 2016-06-10 19:19 ` Daniel Borkmann
  2016-06-10 19:19 ` [PATCH net-next 2/2] bpf: reject wrong sized filters earlier Daniel Borkmann
  2016-06-11  1:01 ` [PATCH net-next 0/2] bpf: couple of fixes David Miller
  2 siblings, 0 replies; 4+ messages in thread
From: Daniel Borkmann @ 2016-06-10 19:19 UTC (permalink / raw)
  To: davem; +Cc: alexei.starovoitov, hannes, netdev, Daniel Borkmann

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 <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 include/linux/netdevice.h |  2 ++
 net/core/dev.c            |  6 ++----
 net/core/filter.c         | 55 +++++++++++++++++++++++++++++------------------
 3 files changed, 38 insertions(+), 25 deletions(-)

diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
index 4f234b1..94eef35 100644
--- a/include/linux/netdevice.h
+++ b/include/linux/netdevice.h
@@ -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);
diff --git a/net/core/dev.c b/net/core/dev.c
index c43c9d2..b148357 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -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);
diff --git a/net/core/filter.c b/net/core/filter.c
index 68adb5f..d11744d 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -1603,9 +1603,36 @@ static const struct bpf_func_proto bpf_csum_diff_proto = {
 	.arg5_type	= ARG_ANYTHING,
 };
 
+static inline int __bpf_rx_skb(struct net_device *dev, struct sk_buff *skb)
+{
+	if (skb_at_tc_ingress(skb))
+		skb_postpush_rcsum(skb, skb_mac_header(skb), skb->mac_len);
+
+	return dev_forward_skb(dev, skb);
+}
+
+static inline int __bpf_tx_skb(struct net_device *dev, struct sk_buff *skb)
+{
+	int ret;
+
+	if (unlikely(__this_cpu_read(xmit_recursion) > XMIT_RECURSION_LIMIT)) {
+		net_crit_ratelimited("bpf: recursion limit reached on datapath, buggy bpf program?\n");
+		kfree_skb(skb);
+		return -ENETDOWN;
+	}
+
+	skb->dev = dev;
+
+	__this_cpu_inc(xmit_recursion);
+	ret = dev_queue_xmit(skb);
+	__this_cpu_dec(xmit_recursion);
+
+	return ret;
+}
+
 static u64 bpf_clone_redirect(u64 r1, u64 ifindex, u64 flags, u64 r4, u64 r5)
 {
-	struct sk_buff *skb = (struct sk_buff *) (long) r1, *skb2;
+	struct sk_buff *skb = (struct sk_buff *) (long) r1;
 	struct net_device *dev;
 
 	if (unlikely(flags & ~(BPF_F_INGRESS)))
@@ -1615,19 +1642,12 @@ static u64 bpf_clone_redirect(u64 r1, u64 ifindex, u64 flags, u64 r4, u64 r5)
 	if (unlikely(!dev))
 		return -EINVAL;
 
-	skb2 = skb_clone(skb, GFP_ATOMIC);
-	if (unlikely(!skb2))
+	skb = skb_clone(skb, GFP_ATOMIC);
+	if (unlikely(!skb))
 		return -ENOMEM;
 
-	if (flags & BPF_F_INGRESS) {
-		if (skb_at_tc_ingress(skb2))
-			skb_postpush_rcsum(skb2, skb_mac_header(skb2),
-					   skb2->mac_len);
-		return dev_forward_skb(dev, skb2);
-	}
-
-	skb2->dev = dev;
-	return dev_queue_xmit(skb2);
+	return flags & BPF_F_INGRESS ?
+	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
 }
 
 static const struct bpf_func_proto bpf_clone_redirect_proto = {
@@ -1671,15 +1691,8 @@ int skb_do_redirect(struct sk_buff *skb)
 		return -EINVAL;
 	}
 
-	if (ri->flags & BPF_F_INGRESS) {
-		if (skb_at_tc_ingress(skb))
-			skb_postpush_rcsum(skb, skb_mac_header(skb),
-					   skb->mac_len);
-		return dev_forward_skb(dev, skb);
-	}
-
-	skb->dev = dev;
-	return dev_queue_xmit(skb);
+	return ri->flags & BPF_F_INGRESS ?
+	       __bpf_rx_skb(dev, skb) : __bpf_tx_skb(dev, skb);
 }
 
 static const struct bpf_func_proto bpf_redirect_proto = {
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* [PATCH net-next 2/2] bpf: reject wrong sized filters earlier
  2016-06-10 19:19 [PATCH net-next 0/2] bpf: couple of fixes Daniel Borkmann
  2016-06-10 19:19 ` [PATCH net-next 1/2] bpf: enforce recursion limit on redirects Daniel Borkmann
@ 2016-06-10 19:19 ` Daniel Borkmann
  2016-06-11  1:01 ` [PATCH net-next 0/2] bpf: couple of fixes David Miller
  2 siblings, 0 replies; 4+ messages in thread
From: Daniel Borkmann @ 2016-06-10 19:19 UTC (permalink / raw)
  To: davem; +Cc: alexei.starovoitov, hannes, netdev, 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 <daniel@iogearbox.net>
Acked-by: Alexei Starovoitov <ast@kernel.org>
---
 net/core/filter.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/net/core/filter.c b/net/core/filter.c
index d11744d..df6860c 100644
--- a/net/core/filter.c
+++ b/net/core/filter.c
@@ -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);
 
-- 
1.9.3

^ permalink raw reply related	[flat|nested] 4+ messages in thread

* Re: [PATCH net-next 0/2] bpf: couple of fixes
  2016-06-10 19:19 [PATCH net-next 0/2] bpf: couple of fixes Daniel Borkmann
  2016-06-10 19:19 ` [PATCH net-next 1/2] bpf: enforce recursion limit on redirects Daniel Borkmann
  2016-06-10 19:19 ` [PATCH net-next 2/2] bpf: reject wrong sized filters earlier Daniel Borkmann
@ 2016-06-11  1:01 ` David Miller
  2 siblings, 0 replies; 4+ messages in thread
From: David Miller @ 2016-06-11  1:01 UTC (permalink / raw)
  To: daniel; +Cc: alexei.starovoitov, hannes, netdev

From: Daniel Borkmann <daniel@iogearbox.net>
Date: Fri, 10 Jun 2016 21:19:05 +0200

> 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.

^ permalink raw reply	[flat|nested] 4+ messages in thread

end of thread, other threads:[~2016-06-11  1:01 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-06-10 19:19 [PATCH net-next 0/2] bpf: couple of fixes Daniel Borkmann
2016-06-10 19:19 ` [PATCH net-next 1/2] bpf: enforce recursion limit on redirects Daniel Borkmann
2016-06-10 19:19 ` [PATCH net-next 2/2] bpf: reject wrong sized filters earlier Daniel Borkmann
2016-06-11  1:01 ` [PATCH net-next 0/2] bpf: couple of fixes David Miller

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.