From patchwork Fri May 26 18:41:25 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eduard Zingerman X-Patchwork-Id: 13257263 X-Patchwork-Delegate: bpf@iogearbox.net Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 467D618B0D for ; Fri, 26 May 2023 18:42:16 +0000 (UTC) Received: from mail-lf1-x132.google.com (mail-lf1-x132.google.com [IPv6:2a00:1450:4864:20::132]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 631511B1 for ; Fri, 26 May 2023 11:41:51 -0700 (PDT) Received: by mail-lf1-x132.google.com with SMTP id 2adb3069b0e04-4f13d8f74abso1157355e87.0 for ; Fri, 26 May 2023 11:41:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685126499; x=1687718499; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=MgB4/yqrcnRs3g67JcbLOGDczqccKnRLWG2kwpPPWhg=; b=rU+ebHPyJsyaDyKwLHHhD+h2mp/WhTfdn5aKi2Ur4tt65kG+f/kz2V/h+64Owrwa4D F9IPGghnqJyhuFfUWDRXzXO41Pa9nvWB76Ft0okzyC71D+4XxGuX7KdBSewF4NE1GZe4 Ym9CJHD0m3Pwg3o0E/XkNVebTyausm2jMIvREY1U4b3mRgjXeVZmtAuRoX8007DiPppg +7hROgDj5Odz2BO3ZXTAKI2946/yfqxWfSV0P3zBK6ldBZavpgW9TR0YB8LIPcvsXUGZ 8UnsPG785psHVcp8sCaCsUr3+ovW0miAqFAnjstPwCI1kPQ2UENy58k/yANRYwZCHNq4 7XzA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685126499; x=1687718499; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=MgB4/yqrcnRs3g67JcbLOGDczqccKnRLWG2kwpPPWhg=; b=VtZsUGZU9rw0WID0QLYAjl5nhO+Le//l+blnh8hcRk70h9brKWtlDD6pWOLPOHR5Uo gXvkOyiWmNsOzmqA8+fnQbkhyLUl2HH0DwGNzgmi3m4dNlRwzsYGVDj44UsTNOaLTVbU S2mDrsyNIOpg8Xzt0AjDDmACAYpgAxV29q3gZBKIDe16Rc161FhfuBx/spDT2G8Ii1+K J54liP89HKygY8XKk3FMESNWnStcCbslMprtsly0i00+ovnAJgC0y1WJwskf8sgG95fA Stz6Nx5AUL34+lZDIwvwfNqJcYDkiqbq21AoNgznEjlUMjAhdWk8KooQPxC0DfJ6ZkSD Cvxw== X-Gm-Message-State: AC+VfDzKU66HK6CL4xWlh59F6q0nG7rR0Bcvj4N2zK7m1XYzyHSB8ymM qyQ2JBR8ys0Ol0gqkhbRtvgNKhBp4tETGA== X-Google-Smtp-Source: ACHHUZ73Yf3093yIvdYGE0T94VqykNykrFudj0Qi8DeKzxyA72RfxruFLbLf+9JDoDlTduwb5tNtmw== X-Received: by 2002:ac2:47eb:0:b0:4f3:7c24:1029 with SMTP id b11-20020ac247eb000000b004f37c241029mr677887lfp.60.1685126498553; Fri, 26 May 2023 11:41:38 -0700 (PDT) Received: from bigfoot.. (host-176-36-0-241.b024.la.net.ua. [176.36.0.241]) by smtp.gmail.com with ESMTPSA id x7-20020ac25dc7000000b004f155762085sm735767lfq.122.2023.05.26.11.41.37 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 May 2023 11:41:38 -0700 (PDT) From: Eduard Zingerman To: bpf@vger.kernel.org, ast@kernel.org Cc: andrii@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev, kernel-team@fb.com, yhs@fb.com, Eduard Zingerman Subject: [PATCH bpf-next v1 1/2] bpf: verify scalar ids mapping in regsafe() using check_ids() Date: Fri, 26 May 2023 21:41:25 +0300 Message-Id: <20230526184126.3104040-2-eddyz87@gmail.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230526184126.3104040-1-eddyz87@gmail.com> References: <20230526184126.3104040-1-eddyz87@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Spam-Status: No, score=-1.8 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS, T_FILL_THIS_FORM_SHORT,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net X-Patchwork-Delegate: bpf@iogearbox.net Make sure that the following unsafe example is rejected by verifier: 1: r9 = ... some pointer with range X ... 2: r6 = ... unbound scalar ID=a ... 3: r7 = ... unbound scalar ID=b ... 4: if (r6 > r7) goto +1 5: r6 = r7 6: if (r6 > X) goto ... --- checkpoint --- 7: r9 += r7 8: *(u64 *)r9 = Y This example is unsafe because not all execution paths verify r7 range. Because of the jump at (4) the verifier would arrive at (6) in two states: I. r6{.id=b}, r7{.id=b} via path 1-6; II. r6{.id=a}, r7{.id=b} via path 1-4, 6. Currently regsafe() does not call check_ids() for scalar registers, thus from POV of regsafe() states (I) and (II) are identical. If the path 1-6 is taken by verifier first, and checkpoint is created at (6) the path [1-4, 6] would be considered safe. This commit updates regsafe() to call check_ids() for scalar registers. The change in check_alu_op() to avoid assigning scalar id to constants is performance optimization. W/o it the regsafe() change becomes costly for some programs, e.g. for tools/testing/selftests/bpf/progs/pyperf600.c the difference is: File Program States (A) States (B) States (DIFF) --------------- -------- ---------- ---------- ---------------- pyperf600.bpf.o on_event 22200 37060 +14860 (+66.94%) Where A -- this patch, B -- this patch but w/o check_alu_op() changes. Fixes: 75748837b7e5 ("bpf: Propagate scalar ranges through register assignments.") Signed-off-by: Eduard Zingerman --- kernel/bpf/verifier.c | 31 ++++++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index af70dad655ab..624556eda430 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -12806,10 +12806,12 @@ static int check_alu_op(struct bpf_verifier_env *env, struct bpf_insn *insn) /* case: R1 = R2 * copy register state to dest reg */ - if (src_reg->type == SCALAR_VALUE && !src_reg->id) + if (src_reg->type == SCALAR_VALUE && !src_reg->id && + !tnum_is_const(src_reg->var_off)) /* Assign src and dst registers the same ID * that will be used by find_equal_scalars() * to propagate min/max range. + * Skip constants to avoid allocation of useless ID. */ src_reg->id = ++env->id_gen; copy_register_state(dst_reg, src_reg); @@ -15151,6 +15153,33 @@ static bool regsafe(struct bpf_verifier_env *env, struct bpf_reg_state *rold, switch (base_type(rold->type)) { case SCALAR_VALUE: + /* Why check_ids() for precise registers? + * + * Consider the following BPF code: + * 1: r6 = ... unbound scalar, ID=a ... + * 2: r7 = ... unbound scalar, ID=b ... + * 3: if (r6 > r7) goto +1 + * 4: r6 = r7 + * 5: if (r6 > X) goto ... + * 6: ... memory operation using r7 ... + * + * First verification path is [1-6]: + * - at (4) same bpf_reg_state::id (b) would be assigned to r6 and r7; + * - at (5) r6 would be marked <= X, find_equal_scalars() would also mark + * r7 <= X, because r6 and r7 share same id. + * + * Next verification path would start from (5), because of the jump at (3). + * The only state difference between first and second visits of (5) is + * bpf_reg_state::id assignments for r6 and r7: (b, b) vs (a, b). + * Thus, use check_ids() to distinguish these states. + * + * The `rold->precise` check is a performance optimization. If `rold->id` + * was ever used to access memory / predict jump, the `rold` or any + * register used in `rold = r?` / `r? = rold` operations would be marked + * as precise, otherwise it's ID is not really interesting. + */ + if (rold->precise && rold->id && !check_ids(rold->id, rcur->id, idmap)) + return false; if (regs_exact(rold, rcur, idmap)) return true; if (env->explore_alu_limits) From patchwork Fri May 26 18:41:26 2023 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Eduard Zingerman X-Patchwork-Id: 13257264 X-Patchwork-Delegate: bpf@iogearbox.net Received: from lindbergh.monkeyblade.net (lindbergh.monkeyblade.net [23.128.96.19]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3168618B0D for ; Fri, 26 May 2023 18:42:19 +0000 (UTC) Received: from mail-lj1-x232.google.com (mail-lj1-x232.google.com [IPv6:2a00:1450:4864:20::232]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id C1658E4A for ; Fri, 26 May 2023 11:41:52 -0700 (PDT) Received: by mail-lj1-x232.google.com with SMTP id 38308e7fff4ca-2af1e290921so10472661fa.3 for ; Fri, 26 May 2023 11:41:52 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20221208; t=1685126500; x=1687718500; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=XivNM9zFbiTSzZT8cp+iPiEmsRKTyl7mrvgg13pkMuM=; b=lDUhghWH0+41WZdL5doj/UNG78Y6aMVswBS3lwGL6UZNvJGw/k4Mg5RFDkWKDuLepV DX0Dgs5mnBowsF1h4Uj/Urk8Dap+u0jWVoXeUZgItjGwmHqbjtOXvz+OvaqWM6N6y9/p qfWMSEvx0jOPV1jHPScTtKyZn0XhjnTRw0uPWodKtsetRL0fzr3E5vf5tC/u0dMOa5l7 GbPxxGbI1TF1GcRxzx/TpEau6rJPBKnfH0hu+BXGi4WjslPW1vvpbuTSQ6B0DYUr6nHe 4sATepEdDuNGtbLA0xLagOmDAlnZtz7ak49kof9fXfzH32RazI/v88mzQb2pElj6a4Ts 5TMQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20221208; t=1685126500; x=1687718500; h=content-transfer-encoding:mime-version:references:in-reply-to :message-id:date:subject:cc:to:from:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=XivNM9zFbiTSzZT8cp+iPiEmsRKTyl7mrvgg13pkMuM=; b=Sp7QmZ9Vo2Xwh1BCPUajumP2V1t9JDspouFFSkLkhVcsfr+Ljc+OvBSix5LYOrhogp 0K/ecfrKeO//7nFRlqT3SPyeAMb4d4zQEXYd8/9MH4TXUe+eVbbL1fYdnufrpvx7HeJU ulGDYh9n0Zrw6DeLaPyZDOqucY4fJhm+zSRRnZAkhCSm1DMpLx9rmvV+6WXett8IJgKG xaG0q0SEEYmPqxrVPdpsSY/v3r9oeYSsfnIoql9Pa8Xn8zZqogKxOrXog2BZfWL2trIM HBNZ1SpHJmw2b321aI0NRCoSPzIXOxC/RUM1n9htBIgJEk9WdcR24/pwlBEbN0b497F1 9vtA== X-Gm-Message-State: AC+VfDxhIWQaH7aHtWu9VDrcWYvqLQAPI16rahE/yLeu2NG1aZs6A/5P FnKd9Jv3KnG+EMWnlSyS2nKCuNG0S4ZAuA== X-Google-Smtp-Source: ACHHUZ5/ax6zRHLUhfJowbo3sQT1OS1uWc3xd7KgyCc+AecfiQWHxR96BRvXpnHWlYWZ2P6B/nJSkA== X-Received: by 2002:a2e:9c93:0:b0:2af:1db6:8f13 with SMTP id x19-20020a2e9c93000000b002af1db68f13mr905662lji.34.1685126499773; Fri, 26 May 2023 11:41:39 -0700 (PDT) Received: from bigfoot.. (host-176-36-0-241.b024.la.net.ua. [176.36.0.241]) by smtp.gmail.com with ESMTPSA id x7-20020ac25dc7000000b004f155762085sm735767lfq.122.2023.05.26.11.41.38 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 26 May 2023 11:41:39 -0700 (PDT) From: Eduard Zingerman To: bpf@vger.kernel.org, ast@kernel.org Cc: andrii@kernel.org, daniel@iogearbox.net, martin.lau@linux.dev, kernel-team@fb.com, yhs@fb.com, Eduard Zingerman Subject: [PATCH bpf-next v1 2/2] selftests/bpf: verify that check_ids() is used for scalars in regsafe() Date: Fri, 26 May 2023 21:41:26 +0300 Message-Id: <20230526184126.3104040-3-eddyz87@gmail.com> X-Mailer: git-send-email 2.40.1 In-Reply-To: <20230526184126.3104040-1-eddyz87@gmail.com> References: <20230526184126.3104040-1-eddyz87@gmail.com> Precedence: bulk X-Mailing-List: bpf@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on lindbergh.monkeyblade.net X-Patchwork-Delegate: bpf@iogearbox.net Verify that the following example is rejected by verifier: r9 = ... some pointer with range X ... r6 = ... unbound scalar ID=a ... r7 = ... unbound scalar ID=b ... if (r6 > r7) goto +1 r6 = r7 if (r6 > X) goto exit r9 += r7 *(u64 *)r9 = Y Signed-off-by: Eduard Zingerman --- .../selftests/bpf/prog_tests/verifier.c | 2 + .../selftests/bpf/progs/verifier_scalar_ids.c | 59 +++++++++++++++++++ 2 files changed, 61 insertions(+) create mode 100644 tools/testing/selftests/bpf/progs/verifier_scalar_ids.c diff --git a/tools/testing/selftests/bpf/prog_tests/verifier.c b/tools/testing/selftests/bpf/prog_tests/verifier.c index 531621adef42..070a13833c3f 100644 --- a/tools/testing/selftests/bpf/prog_tests/verifier.c +++ b/tools/testing/selftests/bpf/prog_tests/verifier.c @@ -50,6 +50,7 @@ #include "verifier_regalloc.skel.h" #include "verifier_ringbuf.skel.h" #include "verifier_runtime_jit.skel.h" +#include "verifier_scalar_ids.skel.h" #include "verifier_search_pruning.skel.h" #include "verifier_sock.skel.h" #include "verifier_spill_fill.skel.h" @@ -150,6 +151,7 @@ void test_verifier_ref_tracking(void) { RUN(verifier_ref_tracking); } void test_verifier_regalloc(void) { RUN(verifier_regalloc); } void test_verifier_ringbuf(void) { RUN(verifier_ringbuf); } void test_verifier_runtime_jit(void) { RUN(verifier_runtime_jit); } +void test_verifier_scalar_ids(void) { RUN(verifier_scalar_ids); } void test_verifier_search_pruning(void) { RUN(verifier_search_pruning); } void test_verifier_sock(void) { RUN(verifier_sock); } void test_verifier_spill_fill(void) { RUN(verifier_spill_fill); } diff --git a/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c new file mode 100644 index 000000000000..c5c7cfbd98d3 --- /dev/null +++ b/tools/testing/selftests/bpf/progs/verifier_scalar_ids.c @@ -0,0 +1,59 @@ +// SPDX-License-Identifier: GPL-2.0 + +#include +#include +#include "bpf_misc.h" + +/* Verify that check_ids() is used by regsafe() for scalars. + * + * r9 = ... some pointer with range X ... + * r6 = ... unbound scalar ID=a ... + * r7 = ... unbound scalar ID=b ... + * if (r6 > r7) goto +1 + * r6 = r7 + * if (r6 > X) goto exit + * r9 += r7 + * *(u8 *)r9 = Y + * + * The memory access is safe only if r7 is bounded, + * which is true for one branch and not true for another. + */ +SEC("socket") +__description("scalar ids: ID mapping in regsafe()") +__failure __msg("register with unbounded min value") +__flag(BPF_F_TEST_STATE_FREQ) +__naked void ids_id_mapping_in_regsafe(void) +{ + asm volatile ( + /* Bump allocated stack */ + "r1 = 0;" + "*(u64*)(r10 - 8) = r1;" + /* r9 = pointer to stack */ + "r9 = r10;" + "r9 += -8;" + /* r7 = ktime_get_ns() */ + "call %[bpf_ktime_get_ns];" + "r7 = r0;" + /* r6 = ktime_get_ns() */ + "call %[bpf_ktime_get_ns];" + "r6 = r0;" + /* if r6 > r7 is an unpredictable jump */ + "if r6 > r7 goto l1_%=;" + "r6 = r7;" +"l1_%=:" + /* a noop to get to add new parent state */ + "r0 = r0;" + /* if r6 > 4 exit(0) */ + "if r6 > 4 goto l2_%=;" + /* Access memory at r9[r7] */ + "r9 += r7;" + "r0 = *(u8*)(r9 + 0);" +"l2_%=:" + "r0 = 0;" + "exit;" + : + : __imm(bpf_ktime_get_ns) + : __clobber_all); +} + +char _license[] SEC("license") = "GPL";