From patchwork Sun Nov 8 22:29:48 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Zi Shen Lim X-Patchwork-Id: 56166 Delivered-To: patch@linaro.org Received: by 10.112.1.169 with SMTP id 9csp159798lbn; Sun, 8 Nov 2015 14:29:54 -0800 (PST) X-Received: by 10.66.234.194 with SMTP id ug2mr35998063pac.122.1447021793963; Sun, 08 Nov 2015 14:29:53 -0800 (PST) Return-Path: Received: from vger.kernel.org (vger.kernel.org. [209.132.180.67]) by mx.google.com with ESMTP id zx6si17931480pbc.51.2015.11.08.14.29.53; Sun, 08 Nov 2015 14:29:53 -0800 (PST) Received-SPF: pass (google.com: best guess record for domain of netdev-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67; Authentication-Results: mx.google.com; spf=pass (google.com: best guess record for domain of netdev-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) smtp.mailfrom=netdev-owner@vger.kernel.org; dkim=neutral (body hash did not verify) header.i=@gmail.com; dmarc=fail (p=NONE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751724AbbKHW3v (ORCPT + 4 others); Sun, 8 Nov 2015 17:29:51 -0500 Received: from mail-ig0-f174.google.com ([209.85.213.174]:35282 "EHLO mail-ig0-f174.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750894AbbKHW3t (ORCPT ); Sun, 8 Nov 2015 17:29:49 -0500 Received: by igl9 with SMTP id 9so19499743igl.0; Sun, 08 Nov 2015 14:29:49 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:in-reply-to:references:date:message-id:subject:from:to :cc:content-type; bh=wjEetioBHv7g+RfFilHLBb6wZ6dyoKu+WWpPoQLUVOw=; b=bs0ncmWGRuHy1pqf2+37u5Uru/o3lLGRd+QceEYODh+/U4rQX4GLUR6ZpN1IxbTyjs GByF+1lbEQdhvbojYeFANEmSH67VAugqjjINCggfTloAqv0TMlfj+QnN79GHKosahU7B EpSBoCFm+NhqCjiC2f9BLi/j45pR/xBE5A/8/r2CBR8gPh5wTcJrqRBMtWhNtQS55bSP CjPURwwrsy8P4QicJ7KJvaQLBgSlGccELU88dX7Hms88d8fIndwAAVym1wg2QxBYLdgo YesF/q91Q/RDgkTDA1leCyGIYMXqJ3u67sXGkxf2B3Fw0Y2hYXhX8Ub8Ir6AUqhHHowk nfUg== MIME-Version: 1.0 X-Received: by 10.50.45.100 with SMTP id l4mr17365661igm.48.1447021789007; Sun, 08 Nov 2015 14:29:49 -0800 (PST) Received: by 10.79.76.23 with HTTP; Sun, 8 Nov 2015 14:29:48 -0800 (PST) In-Reply-To: <20151108022726.GB39441@ast-mbp.thefacebook.com> References: <1446874577-14539-1-git-send-email-yang.shi@linaro.org> <20151108022726.GB39441@ast-mbp.thefacebook.com> Date: Sun, 8 Nov 2015 14:29:48 -0800 Message-ID: Subject: Re: [PATCH] arm64: bpf: fix JIT stack setup From: Z Lim To: Alexei Starovoitov , Yang Shi Cc: Alexei Starovoitov , daniel@iogearbox.net, Catalin Marinas , Will Deacon , Xi Wang , LKML , Network Development , "linux-arm-kernel@lists.infradead.org" , linaro-kernel@lists.linaro.org Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, Nov 7, 2015 at 6:27 PM, Alexei Starovoitov wrote: > On Fri, Nov 06, 2015 at 09:36:17PM -0800, Yang Shi wrote: >> ARM64 JIT used FP (x29) as eBPF fp register, but FP is subjected to >> change during function call so it may cause the BPF prog stack base address >> change too. Whenever, it pointed to the bottom of BPF prog stack instead of >> the top. >> >> So, when copying data via bpf_probe_read, it will be copied to (SP - offset), >> then it may overwrite the saved FP/LR. >> >> Use x25 to replace FP as BPF stack base register (fp). Since x25 is callee >> saved register, so it will keep intact during function call. >> It is initialized in BPF prog prologue when BPF prog is started to run >> everytime. When BPF prog exits, it could be just tossed. >> >> Other than this the BPf prog stack base need to be setup before function >> call stack. >> >> So, the BPF stack layout looks like: >> >> high >> original A64_SP => 0:+-----+ BPF prologue >> | | FP/LR and callee saved registers >> BPF fp register => +64:+-----+ >> | | >> | ... | BPF prog stack >> | | >> | | >> current A64_SP => +-----+ >> | | >> | ... | Function call stack >> | | >> +-----+ >> low >> >> Signed-off-by: Yang Shi >> CC: Zi Shen Lim >> CC: Xi Wang > > Thanks for tracking it down. > That looks like fundamental bug in arm64 jit. I'm surprised function calls worked at all. > Zi please review. > For function calls (BPF_JMP | BPF_CALL), we are compliant with AAPCS64 [1]. That part is okay. bpf_probe_read accesses the BPF program stack, which is based on BPF_REG_FP. This exposes an issue with how BPF_REG_FP was setup, as Yang pointed out. Instead of having BPF_REG_FP point to top of stack, we erroneously point it to the bottom of stack. When there are function calls, we run the risk of clobbering of BPF stack. Bad idea. Otherwise, since BPF_REG_FP is read-only, and is setup exactly once in prologue, it remains consistent throughout lifetime of the BPF program. Yang, can you please try the following? 8<----- ----->8 [1] http://infocenter.arm.com/help/topic/com.arm.doc.ihi0055b/IHI0055B_aapcs64.pdf -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html --- a/arch/arm64/net/bpf_jit_comp.c +++ b/arch/arm64/net/bpf_jit_comp.c @@ -161,12 +161,12 @@ static void build_prologue(struct jit_ctx *ctx) if (ctx->tmp_used) emit(A64_PUSH(tmp1, tmp2, A64_SP), ctx); - /* Set up BPF stack */ - emit(A64_SUB_I(1, A64_SP, A64_SP, stack_size), ctx); - /* Set up frame pointer */ emit(A64_MOV(1, fp, A64_SP), ctx); + /* Set up BPF stack */ + emit(A64_SUB_I(1, A64_SP, A64_SP, stack_size), ctx); + /* Clear registers A and X */ emit_a64_mov_i64(ra, 0, ctx); emit_a64_mov_i64(rx, 0, ctx);