From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Borkmann Subject: Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts Date: Thu, 07 Jan 2016 13:48:48 +0100 Message-ID: <568E5EB0.8020102@iogearbox.net> References: <1452015543-6790-1-git-send-email-rabin@rab.in> <20160105175557.GC83548@ast-mbp.thefacebook.com> <20160106203127.GA16059@debian> <20160106221247.GA95332@ast-mbp.thefacebook.com> <063D6719AE5E284EB5DD2968C1650D6D1CCBF08E@AcuExch.aculab.com> Mime-Version: 1.0 Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Cc: "davem@davemloft.net" , "netdev@vger.kernel.org" , "zlim.lnx@gmail.com" , "yang.shi@linaro.org" , "will.deacon@arm.com" , "catalin.marinas@arm.com" , "linux-arm-kernel@lists.infradead.org" To: David Laight , 'Alexei Starovoitov' , Rabin Vincent Return-path: Received: from www62.your-server.de ([213.133.104.62]:59254 "EHLO www62.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753431AbcAGMtG (ORCPT ); Thu, 7 Jan 2016 07:49:06 -0500 In-Reply-To: <063D6719AE5E284EB5DD2968C1650D6D1CCBF08E@AcuExch.aculab.com> Sender: netdev-owner@vger.kernel.org List-ID: 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.