[PATCHv2] net: bpf: reject invalid shifts

[Hannes Frederic Sowa]

On 13.01.2016 01:19, Alexei Starovoitov wrote:
> On Wed, Jan 13, 2016 at 12:59:46AM +0100, Hannes Frederic Sowa wrote:
>> On 13.01.2016 00:47, Alexei Starovoitov wrote:
>>> On Tue, Jan 12, 2016 at 03:28:22PM -0800, Eric Dumazet wrote:
>>>> On Tue, 2016-01-12 at 12:46 -0800, Alexei Starovoitov wrote:
>>>>> On Tue, Jan 12, 2016 at 09:42:39PM +0100, Daniel Borkmann wrote:
>>>>
>>>>>>> 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.
>>>>>
>>>>> I would rather see broken classic bpf program fixed instead of continue
>>>>> running them with undefined behavior.
>>>>
>>>> This is your choice, because you are a developer.
>>>>
>>>> Some people might be stuck with old software they can not update,
>>>> because they do not have the money to pay developers.
>>>>
>>>> And no, I did not code BPF programs like that, but maybe others did, and
>>>> I feel the pain of customers that might be stuck.
>>>>
>>>> Linus Torvalds always made clear we must provide backward compatibility,
>>>> and really this discussion should not even take place.
>>>>
>>>> As I said, we used to load such BPF program in the past.
>>>>
>>>> The fact that ARM64 crashes because of a faulty JIT implementation is
>>>> not an excuse.
>>>
>>> I would agree if those loaded programs would do something sensible,
>>> but they're broken. As shown arm and arm64 would execute them
>>> differently without JIT, because HW treats such shifts differently.
>>> I also checked that libpcap is sane and doesn't generate broken shifts.
>>> imo we're not breaking backward compatiblity here.
>>
>> But on one specific platform those programs did something deterministic,
>> reproducible and observable, no? Probably most developers only cared about
>> that, probably especially in the embedded segment.
>
> No, they were not. Say we do mask K&31 instead. That may match
> what x86 cpu do, but it will not match arm. You just cannot
> define previously undefined behavior without breaking something.
> And with error the users can actually fix their stuff.

At least the ARM spec says only the least significant byte is used for 
variable input.

My idea was to simply do nothing and leave it as is, getting rid of the 
BUG_ONs in arm64, maybe replacing them with something sensible according 
to the arm64 spec so it matches non-immediate input. It would define new 
behavior only for arm64.

(Hmm, non-optimizing gcc seems to simply not emit such an instruction 
with constant out of the bound. Maybe also an idea to just skip them in 
arm64? Sounds inelegant...)

> If their software is so old and cannot be upgraded, then
> they shouldn't be upgrading the kernel either, something else will break.
> Starting from kernel version. Remember 2.x -> 3.x -> 4.x ?
> Also the arm64 JIT crash was noticed only because of fancy fuzzing,
> so let's be sensible in our risk estimations.

But that wasn't a crash during execution of the program but during the 
generation of the op codes?

I don't have a strong opinion on that and it seems a fix has already 
been applied.

Bye,
Hannes