From mboxrd@z Thu Jan 1 00:00:00 1970 From: alexei.starovoitov@gmail.com (Alexei Starovoitov) Date: Wed, 6 Jan 2016 14:12:49 -0800 Subject: [PATCH] arm64: net: bpf: don't BUG() on large shifts In-Reply-To: <20160106203127.GA16059@debian> References: <1452015543-6790-1-git-send-email-rabin@rab.in> <20160105175557.GC83548@ast-mbp.thefacebook.com> <20160106203127.GA16059@debian> Message-ID: <20160106221247.GA95332@ast-mbp.thefacebook.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Alexei Starovoitov Subject: Re: [PATCH] arm64: net: bpf: don't BUG() on large shifts Date: Wed, 6 Jan 2016 14:12:49 -0800 Message-ID: <20160106221247.GA95332@ast-mbp.thefacebook.com> References: <1452015543-6790-1-git-send-email-rabin@rab.in> <20160105175557.GC83548@ast-mbp.thefacebook.com> <20160106203127.GA16059@debian> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii 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: Rabin Vincent Return-path: Received: from mail-pa0-f47.google.com ([209.85.220.47]:33082 "EHLO mail-pa0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751905AbcAFWMz (ORCPT ); Wed, 6 Jan 2016 17:12:55 -0500 Received: by mail-pa0-f47.google.com with SMTP id cy9so241215061pac.0 for ; Wed, 06 Jan 2016 14:12:55 -0800 (PST) Content-Disposition: inline In-Reply-To: <20160106203127.GA16059@debian> Sender: netdev-owner@vger.kernel.org List-ID: 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.