[PATCH net 0/2] Two cls_bpf fixes

[PATCH net 0/2] Two cls_bpf fixes

[Daniel Borkmann]

Found them while doing a review on act_bpf and going over the
cls_bpf code again. Will also address the first issue in act_bpf
as it needs to be fixed there, too.

Thanks!

Daniel Borkmann (2):
  net: cls_bpf: fix size mismatch on filter preparation
  net: cls_bpf: fix auto generation of per list handles

 net/sched/cls_bpf.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

-- 
1.7.11.7

[PATCH net 1/2] net: cls_bpf: fix size mismatch on filter preparation

[Daniel Borkmann]

In cls_bpf_modify_existing(), we read out the number of filter blocks,
do some sanity checks, allocate a block on that size, and copy over the
BPF instruction blob from user space, then pass everything through the
classic BPF checker prior to installation of the classifier.

We should reject mismatches here, there are 2 scenarios: the number of
filter blocks could be smaller than the provided instruction blob, so
we do a partial copy of the BPF program, and thus the instructions will
either be rejected from the verifier or a valid BPF program will be run;
in the other case, we'll end up copying more than we're supposed to,
and most likely the trailing garbage will be rejected by the verifier
as well (i.e. we need to fit instruction pattern, ret {A,K} needs to be
last instruction, load/stores must be correct, etc); in case not, we
would leak memory when dumping back instruction patterns. The code should
have only used nla_len() as Dave noted to avoid this from the beginning.
Anyway, lets fix it by rejecting such load attempts.

Fixes: 7d1d65cb84e1 ("net: sched: cls_bpf: add BPF-based classifier")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_bpf.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 84c8219..49e5fa8 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -180,6 +180,11 @@ static int cls_bpf_modify_existing(struct net *net, struct tcf_proto *tp,
 	}
 
 	bpf_size = bpf_len * sizeof(*bpf_ops);
+	if (bpf_size != nla_len(tb[TCA_BPF_OPS])) {
+		ret = -EINVAL;
+		goto errout;
+	}
+
 	bpf_ops = kzalloc(bpf_size, GFP_KERNEL);
 	if (bpf_ops == NULL) {
 		ret = -ENOMEM;
-- 
1.7.11.7

[PATCH net 2/2] net: cls_bpf: fix auto generation of per list handles

[Daniel Borkmann]

When creating a bpf classifier in tc with priority collisions and
invoking automatic unique handle assignment, cls_bpf_grab_new_handle()
will return a wrong handle id which in fact is non-unique. Usually
altering of specific filters is being addressed over major id, but
in case of collisions we result in a filter chain, where handle ids
address individual cls_bpf_progs inside the classifier.

Issue is, in cls_bpf_grab_new_handle() we probe for head->hgen handle
in cls_bpf_get() and in case we found a free handle, we're supposed
to use exactly head->hgen. In case of insufficient numbers of handles,
we bail out later as handle id 0 is not allowed.

Fixes: 7d1d65cb84e1 ("net: sched: cls_bpf: add BPF-based classifier")
Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
Acked-by: Jiri Pirko <jiri@resnulli.us>
---
 net/sched/cls_bpf.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/net/sched/cls_bpf.c b/net/sched/cls_bpf.c
index 49e5fa8..f59adf8 100644
--- a/net/sched/cls_bpf.c
+++ b/net/sched/cls_bpf.c
@@ -220,15 +220,21 @@ static u32 cls_bpf_grab_new_handle(struct tcf_proto *tp,
 				   struct cls_bpf_head *head)
 {
 	unsigned int i = 0x80000000;
+	u32 handle;
 
 	do {
 		if (++head->hgen == 0x7FFFFFFF)
 			head->hgen = 1;
 	} while (--i > 0 && cls_bpf_get(tp, head->hgen));
-	if (i == 0)
+
+	if (unlikely(i == 0)) {
 		pr_err("Insufficient number of handles\n");
+		handle = 0;
+	} else {
+		handle = head->hgen;
+	}
 
-	return i;
+	return handle;
 }
 
 static int cls_bpf_change(struct net *net, struct sk_buff *in_skb,
-- 
1.7.11.7

Re: [PATCH net 2/2] net: cls_bpf: fix auto generation of per list handles

[Alexei Starovoitov]

On Thu, Jan 22, 2015 at 1:41 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> When creating a bpf classifier in tc with priority collisions and
> invoking automatic unique handle assignment, cls_bpf_grab_new_handle()
> will return a wrong handle id which in fact is non-unique. Usually
> altering of specific filters is being addressed over major id, but
> in case of collisions we result in a filter chain, where handle ids
> address individual cls_bpf_progs inside the classifier.
>
> Issue is, in cls_bpf_grab_new_handle() we probe for head->hgen handle
> in cls_bpf_get() and in case we found a free handle, we're supposed
> to use exactly head->hgen. In case of insufficient numbers of handles,
> we bail out later as handle id 0 is not allowed.
>
> Fixes: 7d1d65cb84e1 ("net: sched: cls_bpf: add BPF-based classifier")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Acked-by: Jiri Pirko <jiri@resnulli.us>

Acked-by: Alexei Starovoitov <ast@plumgrid.com>

interesting bug. first time I'm looking at this bit and wondering
why everyone copy pasting such hgenerator instead of using idr...

Re: [PATCH net 2/2] net: cls_bpf: fix auto generation of per list handles

[Daniel Borkmann]

On 01/22/2015 07:52 PM, Alexei Starovoitov wrote:
...
> interesting bug. first time I'm looking at this bit and wondering
> why everyone copy pasting such hgenerator instead of using idr...

I think we should abstract these bits into a helper function. ;)
Hmm, looking into git history tree, hgenerator came later than
idr, so I presume the overhead of idr was not worth it.

Re: [PATCH net 0/2] Two cls_bpf fixes

[David Miller]

From: Daniel Borkmann <dborkman@redhat.com>
Date: Thu, 22 Jan 2015 10:41:00 +0100

> Found them while doing a review on act_bpf and going over the
> cls_bpf code again. Will also address the first issue in act_bpf
> as it needs to be fixed there, too.

Series applied, thanks.

Re: [PATCH net 1/2] net: cls_bpf: fix size mismatch on filter preparation

[Alexei Starovoitov]

On Thu, Jan 22, 2015 at 1:41 AM, Daniel Borkmann <dborkman@redhat.com> wrote:
> In cls_bpf_modify_existing(), we read out the number of filter blocks,
> do some sanity checks, allocate a block on that size, and copy over the
> BPF instruction blob from user space, then pass everything through the
> classic BPF checker prior to installation of the classifier.
>
> We should reject mismatches here, there are 2 scenarios: the number of
> filter blocks could be smaller than the provided instruction blob, so
> we do a partial copy of the BPF program, and thus the instructions will
> either be rejected from the verifier or a valid BPF program will be run;
> in the other case, we'll end up copying more than we're supposed to,
> and most likely the trailing garbage will be rejected by the verifier
> as well (i.e. we need to fit instruction pattern, ret {A,K} needs to be
> last instruction, load/stores must be correct, etc); in case not, we
> would leak memory when dumping back instruction patterns. The code should
> have only used nla_len() as Dave noted to avoid this from the beginning.
> Anyway, lets fix it by rejecting such load attempts.
>
> Fixes: 7d1d65cb84e1 ("net: sched: cls_bpf: add BPF-based classifier")
> Signed-off-by: Daniel Borkmann <dborkman@redhat.com>
> Acked-by: Jiri Pirko <jiri@resnulli.us>

Acked-by: Alexei Starovoitov <ast@plumgrid.com>