From patchwork Fri Jul 22 17:58:06 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joanne Koong X-Patchwork-Id: 12926701 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 03F81C43334 for ; Fri, 22 Jul 2022 18:00:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S235900AbiGVSAD (ORCPT ); Fri, 22 Jul 2022 14:00:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51342 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235856AbiGVSAC (ORCPT ); Fri, 22 Jul 2022 14:00:02 -0400 Received: from 66-220-155-178.mail-mxout.facebook.com (66-220-155-178.mail-mxout.facebook.com [66.220.155.178]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 3F3645B071 for ; Fri, 22 Jul 2022 11:00:01 -0700 (PDT) Received: by devbig010.atn6.facebook.com (Postfix, from userid 115148) id F250EF52A4BC; Fri, 22 Jul 2022 10:59:46 -0700 (PDT) From: Joanne Koong To: bpf@vger.kernel.org Cc: kafai@fb.com, jolsa@kernel.org, haoluo@google.com, andrii@kernel.org, daniel@iogearbox.net, ast@kernel.org, Joanne Koong Subject: [PATCH bpf-next v2 1/2] bpf: Fix ref_obj_id for dynptr data slices in verifier Date: Fri, 22 Jul 2022 10:58:06 -0700 Message-Id: <20220722175807.4038317-1-joannelkoong@gmail.com> X-Mailer: git-send-email 2.30.2 MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net When a data slice is obtained from a dynptr (through the bpf_dynptr_data API), the ref obj id of the dynptr must be found and then associated with the data slice. The ref obj id of the dynptr must be found *before* the caller saved regs are reset. Without this fix, the ref obj id tracking is not correct for dynptrs that are at an offset from the frame pointer. Please also note that the data slice's ref obj id must be assigned after the ret types are parsed, since RET_PTR_TO_ALLOC_MEM-type return regs get zero-marked. Fixes: 34d4ef5775f776ec4b0d53a02d588bf3195cada6 ("bpf: Add dynptr data slices"); Signed-off-by: Joanne Koong Acked-by: Martin KaFai Lau --- kernel/bpf/verifier.c | 62 ++++++++++++++++++++----------------------- 1 file changed, 29 insertions(+), 33 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index c59c3df0fea6..29987b2ea26f 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5830,7 +5830,8 @@ static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state static int check_func_arg(struct bpf_verifier_env *env, u32 arg, struct bpf_call_arg_meta *meta, - const struct bpf_func_proto *fn) + const struct bpf_func_proto *fn, + int func_id) { u32 regno = BPF_REG_1 + arg; struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; @@ -6040,23 +6041,33 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, } meta->uninit_dynptr_regno = regno; - } else if (!is_dynptr_reg_valid_init(env, reg, arg_type)) { - const char *err_extra = ""; + } else { + if (!is_dynptr_reg_valid_init(env, reg, arg_type)) { + const char *err_extra = ""; - switch (arg_type & DYNPTR_TYPE_FLAG_MASK) { - case DYNPTR_TYPE_LOCAL: - err_extra = "local "; - break; - case DYNPTR_TYPE_RINGBUF: - err_extra = "ringbuf "; - break; - default: - break; - } + switch (arg_type & DYNPTR_TYPE_FLAG_MASK) { + case DYNPTR_TYPE_LOCAL: + err_extra = "local "; + break; + case DYNPTR_TYPE_RINGBUF: + err_extra = "ringbuf "; + break; + default: + break; + } - verbose(env, "Expected an initialized %sdynptr as arg #%d\n", - err_extra, arg + 1); - return -EINVAL; + verbose(env, "Expected an initialized %sdynptr as arg #%d\n", + err_extra, arg + 1); + return -EINVAL; + } + if (func_id == BPF_FUNC_dynptr_data) { + if (meta->ref_obj_id) { + verbose(env, "verifier internal error: multiple refcounted args in BPF_FUNC_dynptr_data"); + return -EFAULT; + } + /* Find the id of the dynptr we're tracking the reference of */ + meta->ref_obj_id = stack_slot_get_id(env, reg); + } } break; case ARG_CONST_ALLOC_SIZE_OR_ZERO: @@ -7227,7 +7238,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn meta.func_id = func_id; /* check args */ for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) { - err = check_func_arg(env, i, &meta, fn); + err = check_func_arg(env, i, &meta, fn, func_id); if (err) return err; } @@ -7457,7 +7468,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn if (type_may_be_null(regs[BPF_REG_0].type)) regs[BPF_REG_0].id = ++env->id_gen; - if (is_ptr_cast_function(func_id)) { + if (is_ptr_cast_function(func_id) || func_id == BPF_FUNC_dynptr_data) { /* For release_reference() */ regs[BPF_REG_0].ref_obj_id = meta.ref_obj_id; } else if (is_acquire_function(func_id, meta.map_ptr)) { @@ -7469,21 +7480,6 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs[BPF_REG_0].id = id; /* For release_reference() */ regs[BPF_REG_0].ref_obj_id = id; - } else if (func_id == BPF_FUNC_dynptr_data) { - int dynptr_id = 0, i; - - /* Find the id of the dynptr we're acquiring a reference to */ - for (i = 0; i < MAX_BPF_FUNC_REG_ARGS; i++) { - if (arg_type_is_dynptr(fn->arg_type[i])) { - if (dynptr_id) { - verbose(env, "verifier internal error: multiple dynptr args in func\n"); - return -EFAULT; - } - dynptr_id = stack_slot_get_id(env, ®s[BPF_REG_1 + i]); - } - } - /* For release_reference() */ - regs[BPF_REG_0].ref_obj_id = dynptr_id; } do_refine_retval_range(regs, fn->ret_type, func_id, &meta); From patchwork Fri Jul 22 17:58:07 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Joanne Koong X-Patchwork-Id: 12926702 X-Patchwork-Delegate: bpf@iogearbox.net Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id DD69BC433EF for ; Fri, 22 Jul 2022 18:00:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S233965AbiGVSAE (ORCPT ); Fri, 22 Jul 2022 14:00:04 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51358 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S235960AbiGVSAD (ORCPT ); Fri, 22 Jul 2022 14:00:03 -0400 Received: from 69-171-232-181.mail-mxout.facebook.com (69-171-232-181.mail-mxout.facebook.com [69.171.232.181]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 90BD47436E for ; Fri, 22 Jul 2022 11:00:02 -0700 (PDT) Received: by devbig010.atn6.facebook.com (Postfix, from userid 115148) id CC09CF52A4C1; Fri, 22 Jul 2022 10:59:47 -0700 (PDT) From: Joanne Koong To: bpf@vger.kernel.org Cc: kafai@fb.com, jolsa@kernel.org, haoluo@google.com, andrii@kernel.org, daniel@iogearbox.net, ast@kernel.org, Joanne Koong Subject: [PATCH bpf-next v2 2/2] selftests/bpf: add extra test for using dynptr data slice after release Date: Fri, 22 Jul 2022 10:58:07 -0700 Message-Id: <20220722175807.4038317-2-joannelkoong@gmail.com> X-Mailer: git-send-email 2.30.2 In-Reply-To: <20220722175807.4038317-1-joannelkoong@gmail.com> References: <20220722175807.4038317-1-joannelkoong@gmail.com> MIME-Version: 1.0 Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net Add an additional test, "data_slice_use_after_release2", for ensuring that data slices are correctly invalidated by the verifier after the dynptr whose ref obj id they track is released. In particular, this tests data slice invalidation for dynptrs located at a non-zero offset from the frame pointer. Signed-off-by: Joanne Koong Acked-by: Martin KaFai Lau --- .../testing/selftests/bpf/prog_tests/dynptr.c | 3 +- .../testing/selftests/bpf/progs/dynptr_fail.c | 38 ++++++++++++++++++- 2 files changed, 39 insertions(+), 2 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/dynptr.c b/tools/testing/selftests/bpf/prog_tests/dynptr.c index 3c7aa82b98e2..bcf80b9f7c27 100644 --- a/tools/testing/selftests/bpf/prog_tests/dynptr.c +++ b/tools/testing/selftests/bpf/prog_tests/dynptr.c @@ -22,7 +22,8 @@ static struct { {"add_dynptr_to_map2", "invalid indirect read from stack"}, {"data_slice_out_of_bounds_ringbuf", "value is outside of the allowed memory range"}, {"data_slice_out_of_bounds_map_value", "value is outside of the allowed memory range"}, - {"data_slice_use_after_release", "invalid mem access 'scalar'"}, + {"data_slice_use_after_release1", "invalid mem access 'scalar'"}, + {"data_slice_use_after_release2", "invalid mem access 'scalar'"}, {"data_slice_missing_null_check1", "invalid mem access 'mem_or_null'"}, {"data_slice_missing_null_check2", "invalid mem access 'mem_or_null'"}, {"invalid_helper1", "invalid indirect read from stack"}, diff --git a/tools/testing/selftests/bpf/progs/dynptr_fail.c b/tools/testing/selftests/bpf/progs/dynptr_fail.c index d811cff73597..a8ee58ba3a84 100644 --- a/tools/testing/selftests/bpf/progs/dynptr_fail.c +++ b/tools/testing/selftests/bpf/progs/dynptr_fail.c @@ -248,7 +248,7 @@ int data_slice_out_of_bounds_map_value(void *ctx) /* A data slice can't be used after it has been released */ SEC("?raw_tp/sys_nanosleep") -int data_slice_use_after_release(void *ctx) +int data_slice_use_after_release1(void *ctx) { struct bpf_dynptr ptr; struct sample *sample; @@ -272,6 +272,42 @@ int data_slice_use_after_release(void *ctx) return 0; } +/* A data slice can't be used after it has been released. + * + * This tests the case where the data slice tracks a dynptr (ptr2) + * that is at a non-zero offset from the frame pointer (ptr1 is at fp, + * ptr2 is at fp - 16). + */ +SEC("?raw_tp/sys_nanosleep") +int data_slice_use_after_release2(void *ctx) +{ + struct bpf_dynptr ptr1, ptr2; + struct sample *sample; + + bpf_ringbuf_reserve_dynptr(&ringbuf, 64, 0, &ptr1); + bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(*sample), 0, &ptr2); + + sample = bpf_dynptr_data(&ptr2, 0, sizeof(*sample)); + if (!sample) + goto done; + + sample->pid = 23; + + bpf_ringbuf_submit_dynptr(&ptr2, 0); + + /* this should fail */ + sample->pid = 23; + + bpf_ringbuf_submit_dynptr(&ptr1, 0); + + return 0; + +done: + bpf_ringbuf_discard_dynptr(&ptr2, 0); + bpf_ringbuf_discard_dynptr(&ptr1, 0); + return 0; +} + /* A data slice must be first checked for NULL */ SEC("?raw_tp/sys_nanosleep") int data_slice_missing_null_check1(void *ctx)