[PATCHv2] net: bpf: reject invalid shifts
alexei.starovoitov at gmail.com
Tue Jan 12 12:46:21 PST 2016
On Tue, Jan 12, 2016 at 09:42:39PM +0100, Daniel Borkmann wrote:
> On 01/12/2016 08:53 PM, Alexei Starovoitov wrote:
> >On Tue, Jan 12, 2016 at 11:48:38AM -0800, Eric Dumazet wrote:
> >>On Tue, 2016-01-12 at 20:17 +0100, Rabin Vincent wrote:
> >>>On ARM64, a BUG() is triggered in the eBPF JIT if a filter with a
> >>>constant shift that can't be encoded in the immediate field of the
> >>>UBFM/SBFM instructions is passed to the JIT. Since these shifts
> >>>amounts, which are negative or >= regsize, are invalid, reject them in
> >>>the eBPF verifier and the classic BPF filter checker, for all
> >>>diff --git a/net/core/filter.c b/net/core/filter.c
> >>>index 672eefbfbe99..37157c4c1a78 100644
> >>>--- a/net/core/filter.c
> >>>+++ b/net/core/filter.c
> >>>@@ -777,6 +777,11 @@ static int bpf_check_classic(const struct sock_filter *filter,
> >>> if (ftest->k == 0)
> >>> return -EINVAL;
> >>> break;
> >>>+ case BPF_ALU | BPF_LSH | BPF_K:
> >>>+ case BPF_ALU | BPF_RSH | BPF_K:
> >>>+ if (ftest->k >= 32)
> >>>+ return -EINVAL;
> >>>+ break;
> >>> case BPF_LD | BPF_MEM:
> >>> case BPF_LDX | BPF_MEM:
> >>> case BPF_ST:
> >>These weak filters used to have undefined behavior, maybe in a never
> >>taken branch, and will now fail hard, possibly breaking old
> >>I believe we should add a one time warning to give a clue to poor users
> >>hitting this problem.
> >you mean like warn_on_once() here?
> >Makes sense I guess.
> Hmm, WARN_ON_ONCE() would then throw a stack trace also for unprived users,
> I doubt we want to scare admins. ;)
> Or, you mean pr_warn_once()?
yes. there is no need for stack trace of course.
> >>Not everybody has perfect BPF filters, since most of the time they were
> >>hand coded.
> >yep and we all know who was able to code hundreds of cBPF insns by hand ;)
> >But I'm sure that code doesn't have such broken shifts. :)))
> libpcap certainly supports raw filters now thanks to Chema . Alternative
> could be to just mask them here, but not in eBPF verifier, but that would be
> even more inconsistent (on the other hand, we also allow holes in BPF but not
> in eBPF, so wouldn't be the first time we make things different), hmm.
I would rather see broken classic bpf program fixed instead of continue
running them with undefined behavior.
More information about the linux-arm-kernel