[PATCHv2] net: bpf: reject invalid shifts

[Daniel Borkmann]

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
>>> architectures.
>>>
>>
>> Hmm...
>>
>>> 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
>> applications.
>>
>> 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()?

>> 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 [1]. 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.

   [1] https://github.com/the-tcpdump-group/libpcap/commit/273455d58b3bbd83d757bda4f4544e3e5cc8c20a