netdev - Re: [PATCHv2] net: bpf: reject invalid shifts

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <5695653F.6030001@iogearbox.net>
Date:	Tue, 12 Jan 2016 21:42:39 +0100
From:	Daniel Borkmann <daniel@...earbox.net>
To:	Alexei Starovoitov <alexei.starovoitov@...il.com>
CC:	Eric Dumazet <eric.dumazet@...il.com>,
	Rabin Vincent <rabin@....in>, davem@...emloft.net,
	netdev@...r.kernel.org, ast@...nel.org,
	linux-arm-kernel@...ts.infradead.org
Subject: Re: [PATCHv2] net: bpf: reject invalid shifts

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