Message ID | 20231011-jmp-into-reserved-fields-v3-1-97d2aa979788@gmail.com |
---|---|
State | New |
Headers |
Return-Path: <linux-kernel-owner@vger.kernel.org> Delivered-To: ouuuleilei@gmail.com Received: by 2002:a05:612c:2908:b0:403:3b70:6f57 with SMTP id ib8csp395815vqb; Wed, 11 Oct 2023 02:03:13 -0700 (PDT) X-Google-Smtp-Source: AGHT+IEH/t+OHVYzDjUXlkQbZOI/24mnkztwTdvEQspAEkIWyOHjj4EBzfffksWIvPhpw+DWRqNx X-Received: by 2002:a05:6a20:7d85:b0:163:57ba:2ad4 with SMTP id v5-20020a056a207d8500b0016357ba2ad4mr25088158pzj.2.1697014993341; Wed, 11 Oct 2023 02:03:13 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1697014993; cv=none; d=google.com; s=arc-20160816; b=WxXhPHpT6jlTdNiLtpFJpOA4OMe4aqYxBICIyIY4nep3RY8P6LK7VuvQzxE/GQdR7I Pxv402/XZw1ZXo8e6/uG0OUgsZs2ignmSD+hzNmvxpy4A6oii3IzIIJWwRJSIZLgjB1i taBxRBa3dQ0R6wGQMQ9OwdCEgUt0mubt++GkX6AIPo/012/hY6nK0uKdPiKDUyA3foX/ HkL8Oj7FH4s8ClqhxG2GW3LuxkrYouvZr7pwfMHhPzTMFvsAjKIPhZBsScKjaPoPMuXh Q5TkT5tRwLRpS3CWlcxEXgbMP+qiWtsiFbGysGSp99x3Kk5NWZe6fFKZaC2UcUkHOz2Z JDYQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:cc:to:in-reply-to:references:message-id :content-transfer-encoding:mime-version:subject:date:from :dkim-signature; bh=f24fRQOC4uNiaUVlL6gWGvPT3XEkaGKsCGt0d1J4olo=; fh=mXBL/m2WqL131Kw3M+zu1H8f4iBK/X/nYOAw26JNs+8=; b=YQCj6szLNPpkf2tRdP64A55Kwx/pDnZXBvJOdfOFCEQeNeXzrefPvxruRocvvtEJ9D y2BanwsGDtLaMw/Xe3LIPNhnoPDHjbbbigUH0KIZqZIF0CV2wmnbbjStzjf3xECdpJ9r pup7c8jClZvbpY/wSsLURakhv4xchaeLKkIap+qegPZOKR4U6RT2HsY8Fp9H1DWVO6jx mlKH4GEMNRCOcvmaYzmHyc1mngr/jxZnaPzXt5gS7yw+mv7qai0zT7fxU1dvqssdJm6E xJsxfRh5TIFNeq33Zd59/GaPzt+Fblt1aki4AuI835afHhm0yM/ahPNZUs2kiP2OUuyT Qkug== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=NpG1SIAa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from agentk.vger.email (agentk.vger.email. [23.128.96.32]) by mx.google.com with ESMTPS id 26-20020a630f5a000000b005859e8c7c2dsi1560821pgp.639.2023.10.11.02.03.13 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 02:03:13 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) client-ip=23.128.96.32; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20230601 header.b=NpG1SIAa; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.32 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by agentk.vger.email (Postfix) with ESMTP id 32B6881130C9; Wed, 11 Oct 2023 02:03:08 -0700 (PDT) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.10 at agentk.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1346119AbjJKJBp (ORCPT <rfc822;rua109.linux@gmail.com> + 19 others); Wed, 11 Oct 2023 05:01:45 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51964 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S230256AbjJKJBR (ORCPT <rfc822;linux-kernel@vger.kernel.org>); Wed, 11 Oct 2023 05:01:17 -0400 Received: from mail-wr1-x42b.google.com (mail-wr1-x42b.google.com [IPv6:2a00:1450:4864:20::42b]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 43100B8; Wed, 11 Oct 2023 02:01:12 -0700 (PDT) Received: by mail-wr1-x42b.google.com with SMTP id ffacd0b85a97d-3248ac76acbso5792329f8f.1; Wed, 11 Oct 2023 02:01:11 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1697014870; x=1697619670; darn=vger.kernel.org; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:from:to:cc:subject:date:message-id :reply-to; bh=f24fRQOC4uNiaUVlL6gWGvPT3XEkaGKsCGt0d1J4olo=; b=NpG1SIAaJfF5CBPsNL3ABYl4dEjz2howZX3rw3QPRsUGytN9mO7mFkQtaQ8K3TazIF oIWUPn0T9J8Lcz/rTcqJIItu/lktlGuI7eK77BHKVO/Xh0nJvVO4xybaHjuF6DbJC5wH vnxOe9AC4X7KK43e86BSsRTijhxBtfnAQPB0aKFwOnQPWpCyX8fliJsuOzReujMvxlA+ g34zy7lnmAqTX7879vjTsaZd4Bs6a0PDJ1W06elvgN4Uk1DcSsHqGKOQY969nKasC6kS wYVSuVkwrqViOy9QGXibPNZYm/NMNGO04cjet8e9I1dUPZpwwIpvqlPq2r7yFQdzUP/O J6Sg== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1697014870; x=1697619670; h=cc:to:in-reply-to:references:message-id:content-transfer-encoding :mime-version:subject:date:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=f24fRQOC4uNiaUVlL6gWGvPT3XEkaGKsCGt0d1J4olo=; b=DlmAxzPiNot8oHCNb/3Yl5gTSYe2oHoVl3qF2+rDk7SV2P64k7Ydyx7A57JnE549Mi 9P8DWeeMPd38xQm9ElN37SgwqdjLZyWQRPgcHkdVnLu2hln64tAlfbrbynmZ8Gst/CYo GwoVNft8bqcw+p7z8Lm9DKcb4v83cQcRq5BUt3JN9CCav1OG0E7Yj7LzDu0YJTi972wE 5CRTVIEm5Ad8AnY3u2uMYRy9qJmuZwqREA0EjNDKC1aWV48vEqvcqQRviw/tsXTpsO64 muWMiTiM/uxySmvW11hFqbfhsmBH7mQvm2JclN5FZaPtuh6vUgN8s0Ab0mMqHmTFP2w5 wFPg== X-Gm-Message-State: AOJu0YxS59fRuKHv/ehUFQBMZZk0/djry1vOPQU1v8douXEVVqT/J8ts jvvNFz8EloMtg6bGQzjd8w== X-Received: by 2002:a5d:6c69:0:b0:32c:eeee:d438 with SMTP id r9-20020a5d6c69000000b0032ceeeed438mr4254852wrz.54.1697014870092; Wed, 11 Oct 2023 02:01:10 -0700 (PDT) Received: from amdsuplus2.inf.ethz.ch (amdsuplus2.inf.ethz.ch. [129.132.31.88]) by smtp.gmail.com with ESMTPSA id e28-20020adfa45c000000b0032d892e70b4sm554100wra.37.2023.10.11.02.01.09 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 11 Oct 2023 02:01:09 -0700 (PDT) From: Hao Sun <sunhao.th@gmail.com> Date: Wed, 11 Oct 2023 11:00:12 +0200 Subject: [PATCH bpf-next v3 1/3] bpf: Detect jumping to reserved code during check_cfg() MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <20231011-jmp-into-reserved-fields-v3-1-97d2aa979788@gmail.com> References: <20231011-jmp-into-reserved-fields-v3-0-97d2aa979788@gmail.com> In-Reply-To: <20231011-jmp-into-reserved-fields-v3-0-97d2aa979788@gmail.com> To: Alexei Starovoitov <ast@kernel.org>, Daniel Borkmann <daniel@iogearbox.net>, John Fastabend <john.fastabend@gmail.com>, Andrii Nakryiko <andrii@kernel.org>, Martin KaFai Lau <martin.lau@linux.dev>, Song Liu <song@kernel.org>, Yonghong Song <yonghong.song@linux.dev>, KP Singh <kpsingh@kernel.org>, Stanislav Fomichev <sdf@google.com>, Hao Luo <haoluo@google.com>, Jiri Olsa <jolsa@kernel.org> Cc: bpf@vger.kernel.org, linux-kernel@vger.kernel.org, Hao Sun <sunhao.th@gmail.com> X-Mailer: b4 0.12.3 X-Developer-Signature: v=1; a=ed25519-sha256; t=1697014868; l=2204; i=sunhao.th@gmail.com; s=20231009; h=from:subject:message-id; bh=7s+armAaYgTmOreMAmq1jM/+UHfkYy/CB5LBWoJG5Iw=; b=2JmzEYZ+jW5EuRoYwV8LO5vTobCF/hr7/JvRBRIxjqeRNiFvqhUCSds3+XZsFioYRH9g6xPy/ zB6WNafvlcTAn1hD/UEfkfB5z+yyyzMWkzsW6Q1xoeMh1M6bC4zHWb3 X-Developer-Key: i=sunhao.th@gmail.com; a=ed25519; pk=AHFxrImGtyqXOuw4f5xTNh4PGReb7hzD86ayyTZCXd4= X-Spam-Status: No, score=3.0 required=5.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,FREEMAIL_FORGED_FROMDOMAIN,FREEMAIL_FROM, HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,RCVD_IN_SBL_CSS, SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email Precedence: bulk List-ID: <linux-kernel.vger.kernel.org> X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Wed, 11 Oct 2023 02:03:08 -0700 (PDT) X-Spam-Level: ** X-getmail-retrieved-from-mailbox: INBOX X-GMAIL-THRID: 1779449193485052372 X-GMAIL-MSGID: 1779449193485052372 |
Series |
bpf: Detect jumping to reserved code of ld_imm64
|
|
Commit Message
Hao Sun
Oct. 11, 2023, 9 a.m. UTC
Currently, we don't check if the branch-taken of a jump is reserved code of
ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier
gives the following log in such case:
func#0 @0
0: R1=ctx(off=0,imm=0) R10=fp0
0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0)
2: (18) r1 = 0x1d ; R1_w=29
4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0)
5: (1c) w1 -= w1 ; R1_w=0
6: (18) r5 = 0x32 ; R5_w=50
8: (56) if w5 != 0xfffffff4 goto pc-2
mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1
mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32
7: R5_w=50
7: BUG_ld_00
invalid BPF_LD_IMM insn
Here the verifier rejects the program because it thinks insn at 7 is an
invalid BPF_LD_IMM, but such a error log is not accurate since the issue
is jumping to reserved code not because the program contains invalid insn.
Therefore, make the verifier check the jump target during check_cfg(). For
the same program, the verifier reports the following log:
func#0 @0
jump to reserved code from insn 8 to 7
Signed-off-by: Hao Sun <sunhao.th@gmail.com>
---
kernel/bpf/verifier.c | 7 +++++++
1 file changed, 7 insertions(+)
Comments
On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > Currently, we don't check if the branch-taken of a jump is reserved code of > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > gives the following log in such case: > > func#0 @0 > 0: R1=ctx(off=0,imm=0) R10=fp0 > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > 2: (18) r1 = 0x1d ; R1_w=29 > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > 5: (1c) w1 -= w1 ; R1_w=0 > 6: (18) r5 = 0x32 ; R5_w=50 > 8: (56) if w5 != 0xfffffff4 goto pc-2 > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > 7: R5_w=50 > 7: BUG_ld_00 > invalid BPF_LD_IMM insn > > Here the verifier rejects the program because it thinks insn at 7 is an > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > is jumping to reserved code not because the program contains invalid insn. > Therefore, make the verifier check the jump target during check_cfg(). For > the same program, the verifier reports the following log: > > func#0 @0 > jump to reserved code from insn 8 to 7 > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > --- > kernel/bpf/verifier.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > index eed7350e15f4..725ac0b464cf 100644 > --- a/kernel/bpf/verifier.c > +++ b/kernel/bpf/verifier.c > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > { > int *insn_stack = env->cfg.insn_stack; > int *insn_state = env->cfg.insn_state; > + struct bpf_insn *insns = env->prog->insnsi; > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > return DONE_EXPLORING; > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > return -EINVAL; > } > > + if (e == BRANCH && insns[w].code == 0) { > + verbose_linfo(env, t, "%d", t); > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > + return -EINVAL; > + } I don't think we should be changing the verifier to make fuzzer logs more readable. Same with patch 2. The code is fine as-is.
On Wed, Oct 11, 2023 at 3:39 PM Alexei Starovoitov <alexei.starovoitov@gmail.com> wrote: > > On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > > > Currently, we don't check if the branch-taken of a jump is reserved code of > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > > gives the following log in such case: > > > > func#0 @0 > > 0: R1=ctx(off=0,imm=0) R10=fp0 > > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > 2: (18) r1 = 0x1d ; R1_w=29 > > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > 5: (1c) w1 -= w1 ; R1_w=0 > > 6: (18) r5 = 0x32 ; R5_w=50 > > 8: (56) if w5 != 0xfffffff4 goto pc-2 > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > > 7: R5_w=50 > > 7: BUG_ld_00 > > invalid BPF_LD_IMM insn > > > > Here the verifier rejects the program because it thinks insn at 7 is an > > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > > is jumping to reserved code not because the program contains invalid insn. > > Therefore, make the verifier check the jump target during check_cfg(). For > > the same program, the verifier reports the following log: > > > > func#0 @0 > > jump to reserved code from insn 8 to 7 > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > > --- > > kernel/bpf/verifier.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index eed7350e15f4..725ac0b464cf 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > { > > int *insn_stack = env->cfg.insn_stack; > > int *insn_state = env->cfg.insn_state; > > + struct bpf_insn *insns = env->prog->insnsi; > > > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > > return DONE_EXPLORING; > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > return -EINVAL; > > } > > > > + if (e == BRANCH && insns[w].code == 0) { > > + verbose_linfo(env, t, "%d", t); > > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > > + return -EINVAL; > > + } > > I don't think we should be changing the verifier to make > fuzzer logs more readable. > > Same with patch 2. The code is fine as-is. Confused, the changes are not for fuzzer logs but to handle jumping to the middle of ld_imm64. Like jumping out of bounds, both are similar issues and can be handled in one place. The current code handles such incorrect jumps in check_ld_imm(), which is strange, and the error log "BAD_LD_IMM" rather than "bad jump" is also strange. The second one is just for verifier debugging because the only caller of check_ld_imm() is do_check(), before which we already have resolve_pseudo_ldimm64() which has opcode_in_insntable() to check the validity of insn code. The only reason we could see an invalid ld_imm64 in check_id_imm() is errors somewhere else.
On Wed, Oct 11, 2023 at 06:38:56AM -0700, Alexei Starovoitov wrote: > On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > > > Currently, we don't check if the branch-taken of a jump is reserved code of > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > > gives the following log in such case: > > > > func#0 @0 > > 0: R1=ctx(off=0,imm=0) R10=fp0 > > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > 2: (18) r1 = 0x1d ; R1_w=29 > > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > 5: (1c) w1 -= w1 ; R1_w=0 > > 6: (18) r5 = 0x32 ; R5_w=50 > > 8: (56) if w5 != 0xfffffff4 goto pc-2 > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > > 7: R5_w=50 > > 7: BUG_ld_00 > > invalid BPF_LD_IMM insn > > > > Here the verifier rejects the program because it thinks insn at 7 is an > > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > > is jumping to reserved code not because the program contains invalid insn. > > Therefore, make the verifier check the jump target during check_cfg(). For > > the same program, the verifier reports the following log: > > > > func#0 @0 > > jump to reserved code from insn 8 to 7 > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > > --- > > kernel/bpf/verifier.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > index eed7350e15f4..725ac0b464cf 100644 > > --- a/kernel/bpf/verifier.c > > +++ b/kernel/bpf/verifier.c > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > { > > int *insn_stack = env->cfg.insn_stack; > > int *insn_state = env->cfg.insn_state; > > + struct bpf_insn *insns = env->prog->insnsi; > > > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > > return DONE_EXPLORING; > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > return -EINVAL; > > } > > > > + if (e == BRANCH && insns[w].code == 0) { > > + verbose_linfo(env, t, "%d", t); > > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > > + return -EINVAL; > > + } > > I don't think we should be changing the verifier to make > fuzzer logs more readable. Taking fuzzer out of consideration, giving users clearer explanation for such verifier rejection could save a lot of head scratching. Compiler shouldn't generate such program, but its plausible to forget to account that BPF_LD_IMM64 consists of two instructions when writing assembly (especially with filter.h-like macros) and have it jump to the 2nd part of BPF_LD_IMM64. > Same with patch 2. The code is fine as-is. The only way BPF_SIZE(insn->code) != BPF_DW conditional in check_ld_imm() can be met right now is when we have a jump to the 2nd part of LD_IMM64; but what this conditional actually guard against is not straight-forward and quite confusing[1]. Shung-Hsi 1: https://lore.kernel.org/bpf/0cf50c32-ab67-ef23-7b84-ef1d4e007c33@fb.com/
On Thu, Oct 12, 2023 at 1:14 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote: > > On Wed, Oct 11, 2023 at 06:38:56AM -0700, Alexei Starovoitov wrote: > > On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > > > > > Currently, we don't check if the branch-taken of a jump is reserved code of > > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > > > gives the following log in such case: > > > > > > func#0 @0 > > > 0: R1=ctx(off=0,imm=0) R10=fp0 > > > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > 2: (18) r1 = 0x1d ; R1_w=29 > > > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > 5: (1c) w1 -= w1 ; R1_w=0 > > > 6: (18) r5 = 0x32 ; R5_w=50 > > > 8: (56) if w5 != 0xfffffff4 goto pc-2 > > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > > > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > > > 7: R5_w=50 > > > 7: BUG_ld_00 > > > invalid BPF_LD_IMM insn > > > > > > Here the verifier rejects the program because it thinks insn at 7 is an > > > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > > > is jumping to reserved code not because the program contains invalid insn. > > > Therefore, make the verifier check the jump target during check_cfg(). For > > > the same program, the verifier reports the following log: > > > > > > func#0 @0 > > > jump to reserved code from insn 8 to 7 > > > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > > > --- > > > kernel/bpf/verifier.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > index eed7350e15f4..725ac0b464cf 100644 > > > --- a/kernel/bpf/verifier.c > > > +++ b/kernel/bpf/verifier.c > > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > { > > > int *insn_stack = env->cfg.insn_stack; > > > int *insn_state = env->cfg.insn_state; > > > + struct bpf_insn *insns = env->prog->insnsi; > > > > > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > > > return DONE_EXPLORING; > > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > return -EINVAL; > > > } > > > > > > + if (e == BRANCH && insns[w].code == 0) { > > > + verbose_linfo(env, t, "%d", t); > > > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > > > + return -EINVAL; > > > + } > > > > I don't think we should be changing the verifier to make > > fuzzer logs more readable. > > Taking fuzzer out of consideration, giving users clearer explanation for > such verifier rejection could save a lot of head scratching. Users won't see such errors unless they are actively doing what is not recommended. > Compiler shouldn't generate such program, but its plausible to forget to > account that BPF_LD_IMM64 consists of two instructions when writing > assembly (especially with filter.h-like macros) and have it jump to the 2nd > part of BPF_LD_IMM64. Using macros to write bpf asm code is highly discouraged. All kinds of errors are possible. Bogus jump is just one of such mistakes. Use naked functions and inline asm in C code that both GCC and clang understand then you won't see bad jumps. See selftets/bpf/verifier_*.c as an example. > > Same with patch 2. The code is fine as-is. > > The only way BPF_SIZE(insn->code) != BPF_DW conditional in check_ld_imm() > can be met right now is when we have a jump to the 2nd part of LD_IMM64; but > what this conditional actually guard against is not straight-forward and > quite confusing[1]. There are plenty of cases in the verifier where we print an error message. Some of them should be impossible due to prior checks. In such cases we don't yell "verifier bug" and are not going to do that in this case either.
On Thu, Oct 12, 2023 at 08:02:00AM -0700, Alexei Starovoitov wrote: > On Thu, Oct 12, 2023 at 1:14 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote: > > On Wed, Oct 11, 2023 at 06:38:56AM -0700, Alexei Starovoitov wrote: > > > On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > > > > > > > Currently, we don't check if the branch-taken of a jump is reserved code of > > > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > > > > gives the following log in such case: > > > > > > > > func#0 @0 > > > > 0: R1=ctx(off=0,imm=0) R10=fp0 > > > > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > > 2: (18) r1 = 0x1d ; R1_w=29 > > > > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > > 5: (1c) w1 -= w1 ; R1_w=0 > > > > 6: (18) r5 = 0x32 ; R5_w=50 > > > > 8: (56) if w5 != 0xfffffff4 goto pc-2 > > > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > > > > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > > > > 7: R5_w=50 > > > > 7: BUG_ld_00 > > > > invalid BPF_LD_IMM insn > > > > > > > > Here the verifier rejects the program because it thinks insn at 7 is an > > > > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > > > > is jumping to reserved code not because the program contains invalid insn. > > > > Therefore, make the verifier check the jump target during check_cfg(). For > > > > the same program, the verifier reports the following log: > > > > > > > > func#0 @0 > > > > jump to reserved code from insn 8 to 7 > > > > > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > > > > --- > > > > kernel/bpf/verifier.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > index eed7350e15f4..725ac0b464cf 100644 > > > > --- a/kernel/bpf/verifier.c > > > > +++ b/kernel/bpf/verifier.c > > > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > > { > > > > int *insn_stack = env->cfg.insn_stack; > > > > int *insn_state = env->cfg.insn_state; > > > > + struct bpf_insn *insns = env->prog->insnsi; > > > > > > > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > > > > return DONE_EXPLORING; > > > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > > return -EINVAL; > > > > } > > > > > > > > + if (e == BRANCH && insns[w].code == 0) { > > > > + verbose_linfo(env, t, "%d", t); > > > > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > > > > + return -EINVAL; > > > > + } > > > > > > I don't think we should be changing the verifier to make > > > fuzzer logs more readable. > > > > Taking fuzzer out of consideration, giving users clearer explanation for > > such verifier rejection could save a lot of head scratching. > > Users won't see such errors unless they are actively doing what > is not recommended. > > > Compiler shouldn't generate such program, but its plausible to forget to > > account that BPF_LD_IMM64 consists of two instructions when writing > > assembly (especially with filter.h-like macros) and have it jump to the 2nd > > part of BPF_LD_IMM64. > > Using macros to write bpf asm code is highly discouraged. > All kinds of errors are possible. > Bogus jump is just one of such mistakes. > Use naked functions and inline asm in C code that > both GCC and clang understand then you won't see bad jumps. > See selftets/bpf/verifier_*.c as an example. Understood, thanks for the explanation! Found them under progs/verifier_*.c inside the bpf selftest directory. > > > Same with patch 2. The code is fine as-is. > > > > The only way BPF_SIZE(insn->code) != BPF_DW conditional in check_ld_imm() > > can be met right now is when we have a jump to the 2nd part of LD_IMM64; but > > what this conditional actually guard against is not straight-forward and > > quite confusing[1]. > > There are plenty of cases in the verifier where we print > an error message. Some of them should be impossible due > to prior checks. In such cases we don't yell "verifier bug" > and are not going to do that in this case either. I agree, without patch 1 applied, the change to "verfier bug" in patch 2 doesn't make sense and is just wrong. The point I'm trying to make is that the checks done by verifier are generally clear, you can make sense of why certain check are in place just by looking at the code, but BPF_SIZE(insn->code) != BPF_DW is _not_ one of them. I got confused, (reading between the lines I believe) this had Hao puzzled, and even Yongsong had to look twice[1] back then; so this check is certainly not on-par with others we have in the verifier in terms of clarity, which leads to patches here as well as mine a while back. Perhaps we could reconsider making it more obvious how verifier prevents jump to reserved code/2nd instruction of LD_IMM64? 1: the same https://lore.kernel.org/bpf/0cf50c32-ab67-ef23-7b84-ef1d4e007c33@fb.com/
On Thu, Oct 12, 2023 at 8:28 PM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote: > > On Thu, Oct 12, 2023 at 08:02:00AM -0700, Alexei Starovoitov wrote: > > On Thu, Oct 12, 2023 at 1:14 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote: > > > On Wed, Oct 11, 2023 at 06:38:56AM -0700, Alexei Starovoitov wrote: > > > > On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > > > > > > > > > Currently, we don't check if the branch-taken of a jump is reserved code of > > > > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > > > > > gives the following log in such case: > > > > > > > > > > func#0 @0 > > > > > 0: R1=ctx(off=0,imm=0) R10=fp0 > > > > > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > > > 2: (18) r1 = 0x1d ; R1_w=29 > > > > > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > > > 5: (1c) w1 -= w1 ; R1_w=0 > > > > > 6: (18) r5 = 0x32 ; R5_w=50 > > > > > 8: (56) if w5 != 0xfffffff4 goto pc-2 > > > > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > > > > > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > > > > > 7: R5_w=50 > > > > > 7: BUG_ld_00 > > > > > invalid BPF_LD_IMM insn > > > > > > > > > > Here the verifier rejects the program because it thinks insn at 7 is an > > > > > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > > > > > is jumping to reserved code not because the program contains invalid insn. > > > > > Therefore, make the verifier check the jump target during check_cfg(). For > > > > > the same program, the verifier reports the following log: > > > > > > > > > > func#0 @0 > > > > > jump to reserved code from insn 8 to 7 > > > > > > > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > > > > > --- > > > > > kernel/bpf/verifier.c | 7 +++++++ > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > index eed7350e15f4..725ac0b464cf 100644 > > > > > --- a/kernel/bpf/verifier.c > > > > > +++ b/kernel/bpf/verifier.c > > > > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > > > { > > > > > int *insn_stack = env->cfg.insn_stack; > > > > > int *insn_state = env->cfg.insn_state; > > > > > + struct bpf_insn *insns = env->prog->insnsi; > > > > > > > > > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > > > > > return DONE_EXPLORING; > > > > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > > > return -EINVAL; > > > > > } > > > > > > > > > > + if (e == BRANCH && insns[w].code == 0) { > > > > > + verbose_linfo(env, t, "%d", t); > > > > > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > > > > > + return -EINVAL; > > > > > + } > > > > > > > > I don't think we should be changing the verifier to make > > > > fuzzer logs more readable. > > > > > > Taking fuzzer out of consideration, giving users clearer explanation for > > > such verifier rejection could save a lot of head scratching. > > > > Users won't see such errors unless they are actively doing what > > is not recommended. > > > > > Compiler shouldn't generate such program, but its plausible to forget to > > > account that BPF_LD_IMM64 consists of two instructions when writing > > > assembly (especially with filter.h-like macros) and have it jump to the 2nd > > > part of BPF_LD_IMM64. > > > > Using macros to write bpf asm code is highly discouraged. > > All kinds of errors are possible. > > Bogus jump is just one of such mistakes. > > Use naked functions and inline asm in C code that > > both GCC and clang understand then you won't see bad jumps. > > See selftets/bpf/verifier_*.c as an example. > > Understood, thanks for the explanation! > > Found them under progs/verifier_*.c inside the bpf selftest directory. > > > > > Same with patch 2. The code is fine as-is. > > > > > > The only way BPF_SIZE(insn->code) != BPF_DW conditional in check_ld_imm() > > > can be met right now is when we have a jump to the 2nd part of LD_IMM64; but > > > what this conditional actually guard against is not straight-forward and > > > quite confusing[1]. > > > > There are plenty of cases in the verifier where we print > > an error message. Some of them should be impossible due > > to prior checks. In such cases we don't yell "verifier bug" > > and are not going to do that in this case either. > > I agree, without patch 1 applied, the change to "verfier bug" in patch 2 > doesn't make sense and is just wrong. The point I'm trying to make is that > the checks done by verifier are generally clear, you can make sense of why > certain check are in place just by looking at the code, but > BPF_SIZE(insn->code) != BPF_DW is _not_ one of them. > > I got confused, (reading between the lines I believe) this had Hao puzzled, > and even Yongsong had to look twice[1] back then; so this check is certainly > not on-par with others we have in the verifier in terms of clarity, which > leads to patches here as well as mine a while back. > > Perhaps we could reconsider making it more obvious how verifier prevents > jump to reserved code/2nd instruction of LD_IMM64? I agree that the message is confusing. My point is that people see it only when they code in asm with macros. Anyone who was doing that a lot saw that message and probably debugged much worse issues while inserting an asm macro and forgetting to adjust constants in branches. The code might even load, but will execute something totally different. asm macros are a nightmare to debug. Adding more code to the verifier to help with one particular case is not going to help much. Use inline asm in C is the right answer for folks that still need asm. UX of the verifier sucks and we need to improve. So please focus on impactful improvements instead of hacking on niche cases.
On Thu, Oct 19, 2023 at 05:25:26PM -0700, Alexei Starovoitov wrote: > On Thu, Oct 12, 2023 at 8:28 PM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote: > > On Thu, Oct 12, 2023 at 08:02:00AM -0700, Alexei Starovoitov wrote: > > > On Thu, Oct 12, 2023 at 1:14 AM Shung-Hsi Yu <shung-hsi.yu@suse.com> wrote: > > > > On Wed, Oct 11, 2023 at 06:38:56AM -0700, Alexei Starovoitov wrote: > > > > > On Wed, Oct 11, 2023 at 2:01 AM Hao Sun <sunhao.th@gmail.com> wrote: > > > > > > > > > > > > Currently, we don't check if the branch-taken of a jump is reserved code of > > > > > > ld_imm64. Instead, such a issue is captured in check_ld_imm(). The verifier > > > > > > gives the following log in such case: > > > > > > > > > > > > func#0 @0 > > > > > > 0: R1=ctx(off=0,imm=0) R10=fp0 > > > > > > 0: (18) r4 = 0xffff888103436000 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > > > > 2: (18) r1 = 0x1d ; R1_w=29 > > > > > > 4: (55) if r4 != 0x0 goto pc+4 ; R4_w=map_ptr(off=0,ks=4,vs=128,imm=0) > > > > > > 5: (1c) w1 -= w1 ; R1_w=0 > > > > > > 6: (18) r5 = 0x32 ; R5_w=50 > > > > > > 8: (56) if w5 != 0xfffffff4 goto pc-2 > > > > > > mark_precise: frame0: last_idx 8 first_idx 0 subseq_idx -1 > > > > > > mark_precise: frame0: regs=r5 stack= before 6: (18) r5 = 0x32 > > > > > > 7: R5_w=50 > > > > > > 7: BUG_ld_00 > > > > > > invalid BPF_LD_IMM insn > > > > > > > > > > > > Here the verifier rejects the program because it thinks insn at 7 is an > > > > > > invalid BPF_LD_IMM, but such a error log is not accurate since the issue > > > > > > is jumping to reserved code not because the program contains invalid insn. > > > > > > Therefore, make the verifier check the jump target during check_cfg(). For > > > > > > the same program, the verifier reports the following log: > > > > > > > > > > > > func#0 @0 > > > > > > jump to reserved code from insn 8 to 7 > > > > > > > > > > > > Signed-off-by: Hao Sun <sunhao.th@gmail.com> > > > > > > --- > > > > > > kernel/bpf/verifier.c | 7 +++++++ > > > > > > 1 file changed, 7 insertions(+) > > > > > > > > > > > > diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c > > > > > > index eed7350e15f4..725ac0b464cf 100644 > > > > > > --- a/kernel/bpf/verifier.c > > > > > > +++ b/kernel/bpf/verifier.c > > > > > > @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > > > > { > > > > > > int *insn_stack = env->cfg.insn_stack; > > > > > > int *insn_state = env->cfg.insn_state; > > > > > > + struct bpf_insn *insns = env->prog->insnsi; > > > > > > > > > > > > if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) > > > > > > return DONE_EXPLORING; > > > > > > @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, > > > > > > return -EINVAL; > > > > > > } > > > > > > > > > > > > + if (e == BRANCH && insns[w].code == 0) { > > > > > > + verbose_linfo(env, t, "%d", t); > > > > > > + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); > > > > > > + return -EINVAL; > > > > > > + } > > > > > > > > > > I don't think we should be changing the verifier to make > > > > > fuzzer logs more readable. > > > > > > > > Taking fuzzer out of consideration, giving users clearer explanation for > > > > such verifier rejection could save a lot of head scratching. > > > > > > Users won't see such errors unless they are actively doing what > > > is not recommended. > > > > > > > Compiler shouldn't generate such program, but its plausible to forget to > > > > account that BPF_LD_IMM64 consists of two instructions when writing > > > > assembly (especially with filter.h-like macros) and have it jump to the 2nd > > > > part of BPF_LD_IMM64. > > > > > > Using macros to write bpf asm code is highly discouraged. > > > All kinds of errors are possible. > > > Bogus jump is just one of such mistakes. > > > Use naked functions and inline asm in C code that > > > both GCC and clang understand then you won't see bad jumps. > > > See selftets/bpf/verifier_*.c as an example. > > > > Understood, thanks for the explanation! > > > > Found them under progs/verifier_*.c inside the bpf selftest directory. > > > > > > > Same with patch 2. The code is fine as-is. > > > > > > > > The only way BPF_SIZE(insn->code) != BPF_DW conditional in check_ld_imm() > > > > can be met right now is when we have a jump to the 2nd part of LD_IMM64; but > > > > what this conditional actually guard against is not straight-forward and > > > > quite confusing[1]. > > > > > > There are plenty of cases in the verifier where we print > > > an error message. Some of them should be impossible due > > > to prior checks. In such cases we don't yell "verifier bug" > > > and are not going to do that in this case either. > > > > I agree, without patch 1 applied, the change to "verfier bug" in patch 2 > > doesn't make sense and is just wrong. The point I'm trying to make is that > > the checks done by verifier are generally clear, you can make sense of why > > certain check are in place just by looking at the code, but > > BPF_SIZE(insn->code) != BPF_DW is _not_ one of them. > > > > I got confused, (reading between the lines I believe) this had Hao puzzled, > > and even Yongsong had to look twice[1] back then; so this check is certainly > > not on-par with others we have in the verifier in terms of clarity, which > > leads to patches here as well as mine a while back. > > > > Perhaps we could reconsider making it more obvious how verifier prevents > > jump to reserved code/2nd instruction of LD_IMM64? > > I agree that the message is confusing. > My point is that people see it only when they code in asm with macros. > Anyone who was doing that a lot saw that message and probably debugged > much worse issues while inserting an asm macro and forgetting to > adjust constants in branches. The code might even load, but will > execute something totally different. > asm macros are a nightmare to debug. Adding more code to the verifier > to help with one particular case is not going to help much. > Use inline asm in C is the right answer for folks that still need asm. > > UX of the verifier sucks and we need to improve. So please focus on impactful > improvements instead of hacking on niche cases. Ok, can't say I agree entirely, but it's a niche case alright, and I'll leave this alone.
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index eed7350e15f4..725ac0b464cf 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -14980,6 +14980,7 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, { int *insn_stack = env->cfg.insn_stack; int *insn_state = env->cfg.insn_state; + struct bpf_insn *insns = env->prog->insnsi; if (e == FALLTHROUGH && insn_state[t] >= (DISCOVERED | FALLTHROUGH)) return DONE_EXPLORING; @@ -14993,6 +14994,12 @@ static int push_insn(int t, int w, int e, struct bpf_verifier_env *env, return -EINVAL; } + if (e == BRANCH && insns[w].code == 0) { + verbose_linfo(env, t, "%d", t); + verbose(env, "jump to reserved code from insn %d to %d\n", t, w); + return -EINVAL; + } + if (e == BRANCH) { /* mark branch target for state pruning */ mark_prune_point(env, w);