netdev - Re: [PATCH] arm64: net: bpf: don't BUG() on large 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: <568E5EB0.8020102@iogearbox.net>
Date:	Thu, 07 Jan 2016 13:48:48 +0100
From:	Daniel Borkmann <daniel@...earbox.net>
To:	David Laight <David.Laight@...LAB.COM>,
	'Alexei Starovoitov' <alexei.starovoitov@...il.com>,
	Rabin Vincent <rabin@....in>
CC:	"davem@...emloft.net" <davem@...emloft.net>,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>,
	"zlim.lnx@...il.com" <zlim.lnx@...il.com>,
	"yang.shi@...aro.org" <yang.shi@...aro.org>,
	"will.deacon@....com" <will.deacon@....com>,
	"catalin.marinas@....com" <catalin.marinas@....com>,
	"linux-arm-kernel@...ts.infradead.org" 
	<linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts

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.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html