Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

[Daniel Borkmann <daniel@iogearbox.net>]

On 01/07/2016 12:07 PM, David Laight wrote:
> From: Alexei Starovoitov
>> Sent: 06 January 2016 22:13
>> On Wed, Jan 06, 2016 at 09:31:27PM +0100, Rabin Vincent wrote:
>>> On Tue, Jan 05, 2016 at 09:55:58AM -0800, Alexei Starovoitov wrote:
>>>> this one is better to be addressed in verifier instead of eBPF JITs.
>>>> Please reject it in check_alu_op() instead.
>>>
>>> AFAICS the eBPF verifier is not called on the eBPF filters generated by
>>> the BPF->eBPF conversion in net/core/filter.c, so performing this check
>>> only in check_alu_op() will be insufficient.  So I think we'd need to
>>> add this check to bpf_check_classic() too.  Or am I missing something?
>>
>> correct. the check is needed in bpf_check_classic() too and I believe
>> it's ok to tighten it up in this case, since >32 shift is
>> invalid/undefined anyway. We can either accept it as nop or K&=31
>> or error. I think returning error is more user friendly long term, though
>> there is a small risk of rejecting previously loadable broken programs.
>
> Or replace with an assignment of zero?

The question is what is less risky in terms of uabi. To reject such
filters with such K shift vals upfront in verifier, or to just allow
[0, reg_size - 1] values and handle outliers silently. I think both
might be possible, the latter just needs to be clearly specified in
the documentation somewhere. If we go for the latter, then probably
just rewriting that K value as masked one might seem better. Broken
programs might then still be loadable (and still be broken) ... afaik
in case of register (case of shifts with X) with large shift vals
ARM64 is doing 'modulo reg_size' implicitly.