From patchwork Wed Dec 7 20:41:35 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067589 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 5D71CC63705 for ; Wed, 7 Dec 2022 20:41:52 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229796AbiLGUlv (ORCPT ); Wed, 7 Dec 2022 15:41:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55646 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229847AbiLGUlt (ORCPT ); Wed, 7 Dec 2022 15:41:49 -0500 Received: from mail-pl1-x643.google.com (mail-pl1-x643.google.com [IPv6:2607:f8b0:4864:20::643]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 65BA130558 for ; Wed, 7 Dec 2022 12:41:48 -0800 (PST) Received: by mail-pl1-x643.google.com with SMTP id s7so18147770plk.5 for ; Wed, 07 Dec 2022 12:41:48 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=LzOn02ziw3M/3qoRJvJlC9A7pu6zScfFExcm584kBBo=; b=OvHI43Y2NEsjZn/3j+t0fWzuvivG6uawoMC5wv1HbgYw9y6enyhqcT/pBAFpr+HHP+ 68qcRcr8XuzzR1/vOt4EQSbZU8Ro++9OZBCTxTOuMmyrvU6XqBZWcjk2awMzls5fJlKA 4oOZgvaEdvxE9unEr4FxqernIzbRoMKOIQalkWra3yPaX1sL4ip6P883w18n61c8GJ8+ PGoHyN+1w51u71QCyMQbC613OLbgmbjgsOX6s/67EKVTxvdc+M85lsvJdiFlQ6CpRtAt IwAR2snpbAx+VgSSSCraC1sgGxIrYWKCbV9bSPxqvrhkex4fEXSyKztFg+FRW8ecTZFA yi/Q== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=LzOn02ziw3M/3qoRJvJlC9A7pu6zScfFExcm584kBBo=; b=PbLZ4eJuRdVvNfgCc7oNdeQlw3NyQr7pJY0Fl+I/psVNLpT0uoG5VfwvNuEXeORSbt 0w01y+/jDdNPOlDiTvXWp734ii2yXm4RUwOWz608kMpZp5j3o2fH2FY9n27r4xHU9unt vx4oiWT8JJjkoqPbHinh8hDbBBNN4gJL6DgIA4c372IqmPD5m4/7DBbolosBZuKI0Dcf Mtf395EQcY+qrA9iaSoDzgYu7tTpE/qCzCMCn4qoj9yFqTPMCrKYZIh9TT3lcyqJpZea qCd4pjh8qkcnPoMOuGtdETPfFFH/tRlcFLsyjo9qIPYBcbHy6Fv5cB5wegRZVYLKol1A fD0A== X-Gm-Message-State: ANoB5pmlbTsc/mOPi5XB4N98n7YxkfZDmgBUNoKCuyjc2fmmjB/rKgXU fj6FHQ+n+s2uzeCBNvUrkkFWWQUnSQfKpYCJ X-Google-Smtp-Source: AA0mqf43RsdNiCe5YTfUT4TOtpJ8IoQYKEkykaD0Sk0zUnSPb1BcH+Ce876131W/UCQHDRKYKRKxdQ== X-Received: by 2002:a17:902:e5c6:b0:189:a2d4:7ee with SMTP id u6-20020a170902e5c600b00189a2d407eemr39202665plf.18.1670445707551; Wed, 07 Dec 2022 12:41:47 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id i14-20020a17090332ce00b001892af9472esm8829073plr.261.2022.12.07.12.41.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:41:47 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: David Vernet , Joanne Koong , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau Subject: [PATCH bpf-next v2 1/7] bpf: Refactor ARG_PTR_TO_DYNPTR checks into process_dynptr_func Date: Thu, 8 Dec 2022 02:11:35 +0530 Message-Id: <20221207204141.308952-2-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=9676; i=memxor@gmail.com; h=from:subject; bh=YTL37cfGNuxkKC6reZyo4bQbzGQOb6/1EE30AXlrgdM=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOoLL+B1X26N3Ql+5/u48SxpCxiE8mzeSKwYt4x UBw2/ZaJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqAAKCRBM4MiGSL8RyueZD/ sHS7Mtf/Gstq5M24aOfo7V6oP1rCwm3yW9msZ8SQkEZtGmijqikJtcVF97L4xnuEBkOI3kbcPCpKKH V/lOJSQTLqSe0RBGvu5JgGH4Fi+ioGCSKy5S+dRjGwYgr9v2ocamPzrNVgv5MVqRyqfGHFwV1yqP8T vSJFEyGZtVH2bUXlmXqbuZApzGY2s7vNrGULnCMfsozBtmeOQv5XVaONqkPl2/wO5O8HeyWVJqmpzk VYY9SkJZ9Y8sVJaF3Da+qPnxXcJ0XCt35sgghZraLU+rnvvmn0VfmMoWt+NOf0BK6FMdkR9rmzaidb AM+SoXQ0/pPoCqPrL3CFysXUfku4++FgGnw7rB6GZowdv9XKa2Vx+sPJtpwzZ78leeSYxgeOkJYp1R bPYCmqYLgFlmE728qJgNSfk35xSzctWqME4h9tm+/Gs+og4/mDOA/Y8GTHX1MxZd1l8nXiVjV1jh58 vK3Z27Jb3vVjwxMNUgS4CFhledOGMLr8WzXw1aKZIch+9Rkm5I5pFSoQ7/J/918LnU7MCYgjvXJcil NcXjrYUclbnRC3QbeDGz6Ceys3fwyf+IQyG7XQmh2yKCmCybchH7iuoeIReyu5OiVrW9y3E+oePJTQ 79ozncPrNFlCuu7ZrnTm+1FWcsX5qFgIzPW2O8qLrL53eneEJtStJxFSigzw== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net ARG_PTR_TO_DYNPTR is akin to ARG_PTR_TO_TIMER, ARG_PTR_TO_KPTR, where the underlying register type is subjected to more special checks to determine the type of object represented by the pointer and its state consistency. Move dynptr checks to their own 'process_dynptr_func' function so that is consistent and in-line with existing code. This also makes it easier to reuse this code for kfunc handling. Then, reuse this consolidated function in kfunc dynptr handling too. Note that for kfuncs, the arg_type constraint of DYNPTR_TYPE_LOCAL has been lifted. Acked-by: David Vernet Acked-by: Joanne Koong Signed-off-by: Kumar Kartikeya Dwivedi --- include/linux/bpf_verifier.h | 8 +- kernel/bpf/verifier.c | 134 +++++++++--------- .../bpf/prog_tests/kfunc_dynptr_param.c | 7 +- .../bpf/progs/test_kfunc_dynptr_param.c | 12 -- 4 files changed, 75 insertions(+), 86 deletions(-) diff --git a/include/linux/bpf_verifier.h b/include/linux/bpf_verifier.h index 70d06a99f0b8..df0cb825e0e3 100644 --- a/include/linux/bpf_verifier.h +++ b/include/linux/bpf_verifier.h @@ -615,11 +615,9 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, enum bpf_arg_type arg_type); int check_mem_reg(struct bpf_verifier_env *env, struct bpf_reg_state *reg, u32 regno, u32 mem_size); -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, - struct bpf_reg_state *reg); -bool is_dynptr_type_expected(struct bpf_verifier_env *env, - struct bpf_reg_state *reg, - enum bpf_arg_type arg_type); +struct bpf_call_arg_meta; +int process_dynptr_func(struct bpf_verifier_env *env, int regno, + enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta); /* this lives here instead of in bpf.h because it needs to dereference tgt_prog */ static inline u64 bpf_trampoline_compute_key(const struct bpf_prog *tgt_prog, diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 8c5f0adbbde3..882181b14cf1 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -810,8 +810,7 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ return true; } -bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, - struct bpf_reg_state *reg) +static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); int spi = get_spi(reg->off); @@ -830,9 +829,8 @@ bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, return true; } -bool is_dynptr_type_expected(struct bpf_verifier_env *env, - struct bpf_reg_state *reg, - enum bpf_arg_type arg_type) +static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg_state *reg, + enum bpf_arg_type arg_type) { struct bpf_func_state *state = func(env, reg); enum bpf_dynptr_type dynptr_type; @@ -5859,6 +5857,65 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, return 0; } +int process_dynptr_func(struct bpf_verifier_env *env, int regno, + enum bpf_arg_type arg_type, + struct bpf_call_arg_meta *meta) +{ + struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; + + /* We only need to check for initialized / uninitialized helper + * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the + * assumption is that if it is, that a helper function + * initialized the dynptr on behalf of the BPF program. + */ + if (base_type(reg->type) == PTR_TO_DYNPTR) + return 0; + if (arg_type & MEM_UNINIT) { + if (!is_dynptr_reg_valid_uninit(env, reg)) { + verbose(env, "Dynptr has to be an uninitialized dynptr\n"); + return -EINVAL; + } + + /* We only support one dynptr being uninitialized at the moment, + * which is sufficient for the helper functions we have right now. + */ + if (meta->uninit_dynptr_regno) { + verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); + return -EFAULT; + } + + meta->uninit_dynptr_regno = regno; + } else { + if (!is_dynptr_reg_valid_init(env, reg)) { + verbose(env, + "Expected an initialized dynptr as arg #%d\n", + regno); + return -EINVAL; + } + + if (!is_dynptr_type_expected(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: + err_extra = ""; + break; + } + verbose(env, + "Expected a dynptr of type %s as arg #%d\n", + err_extra, regno); + return -EINVAL; + } + } + return 0; +} + static bool arg_type_is_mem_size(enum bpf_arg_type type) { return type == ARG_CONST_SIZE || @@ -6390,52 +6447,8 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, err = check_mem_size_reg(env, reg, regno, true, meta); break; case ARG_PTR_TO_DYNPTR: - /* We only need to check for initialized / uninitialized helper - * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the - * assumption is that if it is, that a helper function - * initialized the dynptr on behalf of the BPF program. - */ - if (base_type(reg->type) == PTR_TO_DYNPTR) - break; - if (arg_type & MEM_UNINIT) { - if (!is_dynptr_reg_valid_uninit(env, reg)) { - verbose(env, "Dynptr has to be an uninitialized dynptr\n"); - return -EINVAL; - } - - /* We only support one dynptr being uninitialized at the moment, - * which is sufficient for the helper functions we have right now. - */ - if (meta->uninit_dynptr_regno) { - verbose(env, "verifier internal error: multiple uninitialized dynptr args\n"); - return -EFAULT; - } - - meta->uninit_dynptr_regno = regno; - } else if (!is_dynptr_reg_valid_init(env, reg)) { - verbose(env, - "Expected an initialized dynptr as arg #%d\n", - arg + 1); - return -EINVAL; - } else if (!is_dynptr_type_expected(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: - err_extra = ""; - break; - } - verbose(env, - "Expected a dynptr of type %s as arg #%d\n", - err_extra, arg + 1); - return -EINVAL; - } + if (process_dynptr_func(env, regno, arg_type, meta)) + return -EACCES; break; case ARG_CONST_ALLOC_SIZE_OR_ZERO: if (!tnum_is_const(reg->var_off)) { @@ -8829,22 +8842,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ return ret; break; case KF_ARG_PTR_TO_DYNPTR: - if (reg->type != PTR_TO_STACK) { - verbose(env, "arg#%d expected pointer to stack\n", i); - return -EINVAL; - } - - if (!is_dynptr_reg_valid_init(env, reg)) { - verbose(env, "arg#%d pointer type %s %s must be valid and initialized\n", - i, btf_type_str(ref_t), ref_tname); + if (reg->type != PTR_TO_STACK && + reg->type != PTR_TO_DYNPTR) { + verbose(env, "arg#%d expected pointer to stack or dynptr_ptr\n", i); return -EINVAL; } - if (!is_dynptr_type_expected(env, reg, ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL)) { - verbose(env, "arg#%d pointer type %s %s points to unsupported dynamic pointer type\n", - i, btf_type_str(ref_t), ref_tname); - return -EINVAL; - } + ret = process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR, NULL); + if (ret < 0) + return ret; break; case KF_ARG_PTR_TO_LIST_HEAD: if (reg->type != PTR_TO_MAP_VALUE && diff --git a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c index 55d641c1f126..a9229260a6ce 100644 --- a/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c +++ b/tools/testing/selftests/bpf/prog_tests/kfunc_dynptr_param.c @@ -18,11 +18,8 @@ static struct { const char *expected_verifier_err_msg; int expected_runtime_err; } kfunc_dynptr_tests[] = { - {"dynptr_type_not_supp", - "arg#0 pointer type STRUCT bpf_dynptr_kern points to unsupported dynamic pointer type", 0}, - {"not_valid_dynptr", - "arg#0 pointer type STRUCT bpf_dynptr_kern must be valid and initialized", 0}, - {"not_ptr_to_stack", "arg#0 expected pointer to stack", 0}, + {"not_valid_dynptr", "Expected an initialized dynptr as arg #1", 0}, + {"not_ptr_to_stack", "arg#0 expected pointer to stack or dynptr_ptr", 0}, {"dynptr_data_null", NULL, -EBADMSG}, }; diff --git a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c index ce39d096bba3..f4a8250329b2 100644 --- a/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c +++ b/tools/testing/selftests/bpf/progs/test_kfunc_dynptr_param.c @@ -32,18 +32,6 @@ int err, pid; char _license[] SEC("license") = "GPL"; -SEC("?lsm.s/bpf") -int BPF_PROG(dynptr_type_not_supp, int cmd, union bpf_attr *attr, - unsigned int size) -{ - char write_data[64] = "hello there, world!!"; - struct bpf_dynptr ptr; - - bpf_ringbuf_reserve_dynptr(&ringbuf, sizeof(write_data), 0, &ptr); - - return bpf_verify_pkcs7_signature(&ptr, &ptr, NULL); -} - SEC("?lsm.s/bpf") int BPF_PROG(not_valid_dynptr, int cmd, union bpf_attr *attr, unsigned int size) { From patchwork Wed Dec 7 20:41:36 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067590 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 4D314C4708D for ; Wed, 7 Dec 2022 20:41:54 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229851AbiLGUlx (ORCPT ); Wed, 7 Dec 2022 15:41:53 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55668 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229847AbiLGUlw (ORCPT ); Wed, 7 Dec 2022 15:41:52 -0500 Received: from mail-pg1-x544.google.com (mail-pg1-x544.google.com [IPv6:2607:f8b0:4864:20::544]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9079630558 for ; Wed, 7 Dec 2022 12:41:51 -0800 (PST) Received: by mail-pg1-x544.google.com with SMTP id r18so17392140pgr.12 for ; Wed, 07 Dec 2022 12:41:51 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=fxrWU7NnT6vHGsB7NFeIFzMUup1ImOCgJhxtDl+Rl34=; b=BUnZfdp4BvMWFVrBCcxMUBFNYqiMBgikhBDxNv2IYjZJ5wXOtScnUAQ/TQ8Eg3ZmbL tM5LMeokB6BIbKtEzxn5a57YpdZbqfmjhjQJ4OflMxRKImo/dyTMMiTQ131T/q/4NRAG UtLsMX+rJK6+GB+o15e8Ibr7i4eqo+BOGVlhiwteqSS0KuR4Q+nYCZDDZfOECilxf8sn 0PRpgfvO3PEMHRSVEPlKEEt/OxIeM5HHHEKr5KCGaRmeG1wQBFAUUzH+rMh78EV0cL4P xHFIZaPlf10jLoq2Hc4Dj/OkTzwMi4CeM9q/sY7ge5zj6nBIyydngStU0zi0eiQhAKhy jvjA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=fxrWU7NnT6vHGsB7NFeIFzMUup1ImOCgJhxtDl+Rl34=; b=xvUxYuhz0K6YhMHI7Ek0dv5bgKakWhfEPhqn9tuFoK3Joj0KOLUbSkjUKK4mUPlDYZ mVcQwVIq/O0Nq7zmgfVOsM9y73/2ninz8DgHj8z3T6bem023w4RMCttR5588wVqEkJGp iwaA6CYsaX3+NVlisUqdi3iY3V3fmOf03Y2d82bxSATuQGe7H5X+SR7/t2av9VYUGxIk 5COI+uwxB9Cm/4DpaoRvIaBMzbqm0FUPqEn8HuTnGECQK0JxwYsT3NW6HMAEc46L9FUS dDK1lpfjOxASaRnRJ+haFYabfvs5DuXQJcrazKSLrMpfznpsV8WjAIxgdWeY9ZME0fK+ EANA== X-Gm-Message-State: ANoB5pnIss55DgeGveb0s/5EegLWXgLPzwfQ27X7Q071khtENUuhQ3+d T5d7KzIK9fgVRh5Ii4yP8AsQS/Dqy20ivA7D X-Google-Smtp-Source: AA0mqf5jI0nzsKW6jAg8a5XymN2pRcRp1pQBhCsCpvDyJ5ucFqmgQrCMQJ4hP/wbGQ3gUqPXw1j3vw== X-Received: by 2002:a63:de14:0:b0:477:4a61:eb99 with SMTP id f20-20020a63de14000000b004774a61eb99mr75566516pgg.48.1670445710952; Wed, 07 Dec 2022 12:41:50 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id t184-20020a6281c1000000b00577adb71f92sm135711pfd.219.2022.12.07.12.41.50 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:41:50 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: Joanne Koong , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , David Vernet Subject: [PATCH bpf-next v2 2/7] bpf: Propagate errors from process_* checks in check_func_arg Date: Thu, 8 Dec 2022 02:11:36 +0530 Message-Id: <20221207204141.308952-3-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2195; i=memxor@gmail.com; h=from:subject; bh=gmdyq/+tBMx3WETs8zPL/dlheSlesIH0Ru/dgcZYGJ8=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOoGOqNPniWr+Y3uHGnPyVtWIFGcvgDLXD0T+7n hF5VOF2JAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqAAKCRBM4MiGSL8Ryi8kEA CKKaQTj1bJCh62C2pJtz19rxnmyYJa2et18eNx0WVzXOWqFKgmBU1XHXNzI/7pFGU1QIKiTwqm30oK +WrguC7QvRGqFKHVWBuTC49Rom8yZ2dMCI67aTg6Ce+J5T7AvHf3ZbpNZESKwYd1MtuUjj7i9bWs3H CdMFbPf7NRL0SU+Kvrf6hjcuUEh0AfdQ0aorxfQ+pbgcqLb2C0djz3gs3qAhvredxKTXhwAGbkfjDf 6VHRhA5aquX37Du9jvj5o5PQTsoSy28zB9r4aE+RgJnBctYJeww9rqi7t6Xne5gCiKFiKMneFM5h2C vhR+PE3F+Z/dKOzS7icZkQdJ3nOXh0mcY6nE6G+Q3mFx7m6/k+sGP/R1J2csLHN+SdIoWfCBBISRcG PBYJ0ryvxoomB5rIzEALZC19RYBpyXKR+D9EGj9MHqpdDEuP5JzqDvWihpTQGNYPzoEoU1Isqf0FBT mwVuZW/+e7keq+CDIPVLY0QdYmL+BwyBmOdMTRLhH+4/Sze461F0OBW+DKu1+yCFvtiQ53AUATZdQz q+8HWtM+vI7/zhlioJZQcF/7rVLB5q/z/r2MFgYxGHiuPK3c7avCU0CqOL3gENFI2I4A8G+BK2N4l7 ESXEERLGkYdjISlMWd6Sr0AjzKtqLU66VJLhWDLLb66X1qgWJIhy81jpphKw== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net Currently, we simply ignore the errors in process_spin_lock, process_timer_func, process_kptr_func, process_dynptr_func. Instead, bubble up the error by storing and checking err variable. Acked-by: Joanne Koong Signed-off-by: Kumar Kartikeya Dwivedi --- kernel/bpf/verifier.c | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 882181b14cf1..1af449c20a10 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6412,19 +6412,22 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, break; case ARG_PTR_TO_SPIN_LOCK: if (meta->func_id == BPF_FUNC_spin_lock) { - if (process_spin_lock(env, regno, true)) - return -EACCES; + err = process_spin_lock(env, regno, true); + if (err) + return err; } else if (meta->func_id == BPF_FUNC_spin_unlock) { - if (process_spin_lock(env, regno, false)) - return -EACCES; + err = process_spin_lock(env, regno, false); + if (err) + return err; } else { verbose(env, "verifier internal error\n"); return -EFAULT; } break; case ARG_PTR_TO_TIMER: - if (process_timer_func(env, regno, meta)) - return -EACCES; + err = process_timer_func(env, regno, meta); + if (err) + return err; break; case ARG_PTR_TO_FUNC: meta->subprogno = reg->subprogno; @@ -6447,8 +6450,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, err = check_mem_size_reg(env, reg, regno, true, meta); break; case ARG_PTR_TO_DYNPTR: - if (process_dynptr_func(env, regno, arg_type, meta)) - return -EACCES; + err = process_dynptr_func(env, regno, arg_type, meta); + if (err) + return err; break; case ARG_CONST_ALLOC_SIZE_OR_ZERO: if (!tnum_is_const(reg->var_off)) { @@ -6515,8 +6519,9 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, break; } case ARG_PTR_TO_KPTR: - if (process_kptr_func(env, regno, meta)) - return -EACCES; + err = process_kptr_func(env, regno, meta); + if (err) + return err; break; } From patchwork Wed Dec 7 20:41:37 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067592 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 925B9C63705 for ; Wed, 7 Dec 2022 20:42:03 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229663AbiLGUmC (ORCPT ); Wed, 7 Dec 2022 15:42:02 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55742 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229917AbiLGUl5 (ORCPT ); Wed, 7 Dec 2022 15:41:57 -0500 Received: from mail-pf1-x442.google.com (mail-pf1-x442.google.com [IPv6:2607:f8b0:4864:20::442]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 7534A30558 for ; Wed, 7 Dec 2022 12:41:55 -0800 (PST) Received: by mail-pf1-x442.google.com with SMTP id 140so18514210pfz.6 for ; Wed, 07 Dec 2022 12:41:55 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=sVJJ1TBXIeuiDVafpChCYPBWBDEdHKmbMIWBXkU4gwU=; b=gGlIR43fpiZQ8VVKywI8oewX+ZrLilhAySPBO8R39pTEVdyGeQzD0YvTA+p0IZuev0 jADZyt7wtsDgBrHiLBV7APEsIqaZTDarbRMeZUlUION+Jm5dxLf1s1uWf9/CnNKhR2pN hAZPkXT7Klje7MnsY8aXBxF9DISoWobmiEvAxy5++Ny3yQoX72A7Bd5v+HK+4PGzPfk5 Eej/7VfcYi3DPS4k5o+XnMw2z6R7TVEaej7j2K5ZBVTnp+JAyPSoJvSuyT5H4Jvut/sk QnOqcPRhMrJeIZPF9skVMKkJLcANX9EJbCBHg/NAqIi8YTTfdRETUDXjSjPqY4V/2fEF jVlw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=sVJJ1TBXIeuiDVafpChCYPBWBDEdHKmbMIWBXkU4gwU=; b=4x7hCXsnlA08qP3DftWKn45+jVWNTvBGRqyVlB0SNfdhW7DGr7va9Chb1ZrWKJ5CiS KfrvhH2iuXNtFmJTAGVUHsCPpCB+nlT6vh8EhGmYdTK4Tp2x9WLVwjzDTFcQjhWotCWY uvbWpvI9fAbAG0NtCW5kv0Ic9BGrpmWxylEeHy8y8RHT3I4axMnFB3UBSOzaP0A97Fcm pU8NR562Pu7xPdywHkqtIyCGPcinI3m7uFb1YpBbaD4wMZQrvbugZjSyIY4xSdQ6+5fj tpk8wAerBSa/VxCPtLV/fsXXTDW4pRMiolohMSN7tEmgxci4j7MWC4PRrJDp+FYMiAZx 5vYQ== X-Gm-Message-State: ANoB5plZUP1VV77k0WP8WbXb7CXdwdv8SSHWkYJY7eu5sKb20dlZFePj CnbuXAe4xqWn4oZaFiHv79OUV/ixbmfOmuas X-Google-Smtp-Source: AA0mqf5tJ2ykBpREBp7Rt0zShErw7YgqKmqESSByOTuOW3jd6l8Hqp5PwJuhtKo4mLoJBluNosTBxA== X-Received: by 2002:a62:8641:0:b0:577:6515:f257 with SMTP id x62-20020a628641000000b005776515f257mr5691014pfd.10.1670445714644; Wed, 07 Dec 2022 12:41:54 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id 13-20020a62180d000000b00573769811d6sm11647261pfy.44.2022.12.07.12.41.53 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:41:54 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: Joanne Koong , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , David Vernet Subject: [PATCH bpf-next v2 3/7] bpf: Rework process_dynptr_func Date: Thu, 8 Dec 2022 02:11:37 +0530 Message-Id: <20221207204141.308952-4-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=28270; i=memxor@gmail.com; h=from:subject; bh=E62m2OtYRTWsK1TraRoFKVviBq5MROTPLnzaqK8uOLw=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOopUvQPjCAXVKSXGPevby39bvJAooZ2je5DUvr F2PcG7SJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqAAKCRBM4MiGSL8RysopD/ 0eG59XnRMBmVjp5VopSB9QA0+Xw+wGsAq23PvCce3sEOjrJHu/7YTwb49dJBr6WlRNgHnrjQmaK8qq ANBCmdrcjqpqDQoP7saX6JO8FyQDkJWT9RQaCfP0W3lgQYy/srOCSVDIZtuhlpZIlGlxl8Rs8rTJrp 4zsT4++e1U/mqBXfmxuzBTD7UiR9rUv48IOQXzLpt899jYApFUxgsu+IM0wFt2EYg59c6XK7HLXHQ+ tyHV3oNELmskmrEM6pcRD5n4kZ42OBUeL8aMQ60ax1sQXKu2933A4uW7gPK+Zamhzfzv51v5NDmjTH 1V8Zu8PpeLoKZy5QKm9b9m3h/1oW8kXS/psihWTD+gjBwozgSqlGQ2PjSQKRf3T/KoKESZp1uXPUdE YZkSCDCVGnyyYqjykixHsB3q/l5dnt4xpGaFu9nDHfiadk7rYw6nT8xoUaOVqM6W6ixE9l29sXjvr8 MrU+w+TltPCQDj+YD67BCQt8q3xtvxSr+i4DTzPOoOO9RjmoLmKhQmLUeiG6DTgCcn8gA1Jx6I1J3r 72DA1Z+aiMqT9xvOnkEBoEA3/DZAHG9kxdm/RzUx0oH/wae6fRf6hu9XruHf9rhht9pngUb6Wi+7xx dPi132U6cXTR3ZAngevNvEf5zu8uPpj/fc7E1bWiMdOVA3v0kbBdFoPkxQxw== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net Recently, user ringbuf support introduced a PTR_TO_DYNPTR register type for use in callback state, because in case of user ringbuf helpers, there is no dynptr on the stack that is passed into the callback. To reflect such a state, a special register type was created. However, some checks have been bypassed incorrectly during the addition of this feature. First, for arg_type with MEM_UNINIT flag which initialize a dynptr, they must be rejected for such register type. Secondly, in the future, there are plans to add dynptr helpers that operate on the dynptr itself and may change its offset and other properties. In all of these cases, PTR_TO_DYNPTR shouldn't be allowed to be passed to such helpers, however the current code simply returns 0. The rejection for helpers that release the dynptr is already handled. For fixing this, we take a step back and rework existing code in a way that will allow fitting in all classes of helpers and have a coherent model for dealing with the variety of use cases in which dynptr is used. First, for ARG_PTR_TO_DYNPTR, it can either be set alone or together with a DYNPTR_TYPE_* constant that denotes the only type it accepts. Next, helpers which initialize a dynptr use MEM_UNINIT to indicate this fact. To make the distinction clear, use MEM_RDONLY flag to indicate that the helper only operates on the memory pointed to by the dynptr, not the dynptr itself. In C parlance, it would be equivalent to taking the dynptr as a point to const argument. When either of these flags are not present, the helper is allowed to mutate both the dynptr itself and also the memory it points to. Currently, the read only status of the memory is not tracked in the dynptr, but it would be trivial to add this support inside dynptr state of the register. With these changes and renaming PTR_TO_DYNPTR to CONST_PTR_TO_DYNPTR to better reflect its usage, it can no longer be passed to helpers that initialize a dynptr, i.e. bpf_dynptr_from_mem, bpf_ringbuf_reserve_dynptr. A note to reviewers is that in code that does mark_stack_slots_dynptr, and unmark_stack_slots_dynptr, we implicitly rely on the fact that PTR_TO_STACK reg is the only case that can reach that code path, as one cannot pass CONST_PTR_TO_DYNPTR to helpers that don't set MEM_RDONLY. In both cases such helpers won't be setting that flag. The next patch will add a couple of selftest cases to make sure this doesn't break. Fixes: 205715673844 ("bpf: Add bpf_user_ringbuf_drain() helper") Acked-by: Joanne Koong Signed-off-by: Kumar Kartikeya Dwivedi --- include/linux/bpf.h | 4 +- include/uapi/linux/bpf.h | 8 +- kernel/bpf/helpers.c | 18 +- kernel/bpf/verifier.c | 227 +++++++++++++----- scripts/bpf_doc.py | 1 + tools/include/uapi/linux/bpf.h | 8 +- .../selftests/bpf/prog_tests/user_ringbuf.c | 4 +- 7 files changed, 191 insertions(+), 79 deletions(-) diff --git a/include/linux/bpf.h b/include/linux/bpf.h index 4920ac252754..3de24cfb7a3d 100644 --- a/include/linux/bpf.h +++ b/include/linux/bpf.h @@ -775,7 +775,7 @@ enum bpf_reg_type { PTR_TO_MEM, /* reg points to valid memory region */ PTR_TO_BUF, /* reg points to a read/write buffer */ PTR_TO_FUNC, /* reg points to a bpf program function */ - PTR_TO_DYNPTR, /* reg points to a dynptr */ + CONST_PTR_TO_DYNPTR, /* reg points to a const struct bpf_dynptr */ __BPF_REG_TYPE_MAX, /* Extended reg_types. */ @@ -2828,7 +2828,7 @@ void bpf_dynptr_init(struct bpf_dynptr_kern *ptr, void *data, enum bpf_dynptr_type type, u32 offset, u32 size); void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr); int bpf_dynptr_check_size(u32 size); -u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr); +u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr); #ifdef CONFIG_BPF_LSM void bpf_cgroup_atype_get(u32 attach_btf_id, int cgroup_atype); diff --git a/include/uapi/linux/bpf.h b/include/uapi/linux/bpf.h index f89de51a45db..464ca3f01fe7 100644 --- a/include/uapi/linux/bpf.h +++ b/include/uapi/linux/bpf.h @@ -5293,7 +5293,7 @@ union bpf_attr { * Return * Nothing. Always succeeds. * - * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset, u64 flags) + * long bpf_dynptr_read(void *dst, u32 len, const struct bpf_dynptr *src, u32 offset, u64 flags) * Description * Read *len* bytes from *src* into *dst*, starting from *offset* * into *src*. @@ -5303,7 +5303,7 @@ union bpf_attr { * of *src*'s data, -EINVAL if *src* is an invalid dynptr or if * *flags* is not 0. * - * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags) + * long bpf_dynptr_write(const struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags) * Description * Write *len* bytes from *src* into *dst*, starting from *offset* * into *dst*. @@ -5313,7 +5313,7 @@ union bpf_attr { * of *dst*'s data, -EINVAL if *dst* is an invalid dynptr or if *dst* * is a read-only dynptr or if *flags* is not 0. * - * void *bpf_dynptr_data(struct bpf_dynptr *ptr, u32 offset, u32 len) + * void *bpf_dynptr_data(const struct bpf_dynptr *ptr, u32 offset, u32 len) * Description * Get a pointer to the underlying dynptr data. * @@ -5414,7 +5414,7 @@ union bpf_attr { * Drain samples from the specified user ring buffer, and invoke * the provided callback for each such sample: * - * long (\*callback_fn)(struct bpf_dynptr \*dynptr, void \*ctx); + * long (\*callback_fn)(const struct bpf_dynptr \*dynptr, void \*ctx); * * If **callback_fn** returns 0, the helper will continue to try * and drain the next sample, up to a maximum of diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index 284b3ffdbe48..bf9a6a646254 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1404,7 +1404,7 @@ static const struct bpf_func_proto bpf_kptr_xchg_proto = { #define DYNPTR_SIZE_MASK 0xFFFFFF #define DYNPTR_RDONLY_BIT BIT(31) -static bool bpf_dynptr_is_rdonly(struct bpf_dynptr_kern *ptr) +static bool bpf_dynptr_is_rdonly(const struct bpf_dynptr_kern *ptr) { return ptr->size & DYNPTR_RDONLY_BIT; } @@ -1414,7 +1414,7 @@ static void bpf_dynptr_set_type(struct bpf_dynptr_kern *ptr, enum bpf_dynptr_typ ptr->size |= type << DYNPTR_TYPE_SHIFT; } -u32 bpf_dynptr_get_size(struct bpf_dynptr_kern *ptr) +u32 bpf_dynptr_get_size(const struct bpf_dynptr_kern *ptr) { return ptr->size & DYNPTR_SIZE_MASK; } @@ -1438,7 +1438,7 @@ void bpf_dynptr_set_null(struct bpf_dynptr_kern *ptr) memset(ptr, 0, sizeof(*ptr)); } -static int bpf_dynptr_check_off_len(struct bpf_dynptr_kern *ptr, u32 offset, u32 len) +static int bpf_dynptr_check_off_len(const struct bpf_dynptr_kern *ptr, u32 offset, u32 len) { u32 size = bpf_dynptr_get_size(ptr); @@ -1483,7 +1483,7 @@ static const struct bpf_func_proto bpf_dynptr_from_mem_proto = { .arg4_type = ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL | MEM_UNINIT, }; -BPF_CALL_5(bpf_dynptr_read, void *, dst, u32, len, struct bpf_dynptr_kern *, src, +BPF_CALL_5(bpf_dynptr_read, void *, dst, u32, len, const struct bpf_dynptr_kern *, src, u32, offset, u64, flags) { int err; @@ -1506,12 +1506,12 @@ static const struct bpf_func_proto bpf_dynptr_read_proto = { .ret_type = RET_INTEGER, .arg1_type = ARG_PTR_TO_UNINIT_MEM, .arg2_type = ARG_CONST_SIZE_OR_ZERO, - .arg3_type = ARG_PTR_TO_DYNPTR, + .arg3_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY, .arg4_type = ARG_ANYTHING, .arg5_type = ARG_ANYTHING, }; -BPF_CALL_5(bpf_dynptr_write, struct bpf_dynptr_kern *, dst, u32, offset, void *, src, +BPF_CALL_5(bpf_dynptr_write, const struct bpf_dynptr_kern *, dst, u32, offset, void *, src, u32, len, u64, flags) { int err; @@ -1532,14 +1532,14 @@ static const struct bpf_func_proto bpf_dynptr_write_proto = { .func = bpf_dynptr_write, .gpl_only = false, .ret_type = RET_INTEGER, - .arg1_type = ARG_PTR_TO_DYNPTR, + .arg1_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY, .arg2_type = ARG_ANYTHING, .arg3_type = ARG_PTR_TO_MEM | MEM_RDONLY, .arg4_type = ARG_CONST_SIZE_OR_ZERO, .arg5_type = ARG_ANYTHING, }; -BPF_CALL_3(bpf_dynptr_data, struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) +BPF_CALL_3(bpf_dynptr_data, const struct bpf_dynptr_kern *, ptr, u32, offset, u32, len) { int err; @@ -1560,7 +1560,7 @@ static const struct bpf_func_proto bpf_dynptr_data_proto = { .func = bpf_dynptr_data, .gpl_only = false, .ret_type = RET_PTR_TO_DYNPTR_MEM_OR_NULL, - .arg1_type = ARG_PTR_TO_DYNPTR, + .arg1_type = ARG_PTR_TO_DYNPTR | MEM_RDONLY, .arg2_type = ARG_ANYTHING, .arg3_type = ARG_CONST_ALLOC_SIZE_OR_ZERO, }; diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index 1af449c20a10..fdbaf22cdaf2 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -592,7 +592,7 @@ static const char *reg_type_str(struct bpf_verifier_env *env, [PTR_TO_BUF] = "buf", [PTR_TO_FUNC] = "func", [PTR_TO_MAP_KEY] = "map_key", - [PTR_TO_DYNPTR] = "dynptr_ptr", + [CONST_PTR_TO_DYNPTR] = "dynptr_ptr", }; if (type & PTR_MAYBE_NULL) { @@ -725,6 +725,28 @@ static bool dynptr_type_refcounted(enum bpf_dynptr_type type) return type == BPF_DYNPTR_TYPE_RINGBUF; } +static void __mark_dynptr_reg(struct bpf_reg_state *reg, + enum bpf_dynptr_type type, + bool first_slot); + +static void __mark_reg_not_init(const struct bpf_verifier_env *env, + struct bpf_reg_state *reg); + +static void mark_dynptr_stack_regs(struct bpf_reg_state *sreg1, + struct bpf_reg_state *sreg2, + enum bpf_dynptr_type type) +{ + __mark_dynptr_reg(sreg1, type, true); + __mark_dynptr_reg(sreg2, type, false); +} + +static void mark_dynptr_cb_reg(struct bpf_reg_state *reg, + enum bpf_dynptr_type type) +{ + __mark_dynptr_reg(reg, type, true); +} + + static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_state *reg, enum bpf_arg_type arg_type, int insn_idx) { @@ -746,9 +768,8 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ if (type == BPF_DYNPTR_TYPE_INVALID) return -EINVAL; - state->stack[spi].spilled_ptr.dynptr.first_slot = true; - state->stack[spi].spilled_ptr.dynptr.type = type; - state->stack[spi - 1].spilled_ptr.dynptr.type = type; + mark_dynptr_stack_regs(&state->stack[spi].spilled_ptr, + &state->stack[spi - 1].spilled_ptr, type); if (dynptr_type_refcounted(type)) { /* The id is used to track proper releasing */ @@ -756,8 +777,8 @@ static int mark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_reg_ if (id < 0) return id; - state->stack[spi].spilled_ptr.id = id; - state->stack[spi - 1].spilled_ptr.id = id; + state->stack[spi].spilled_ptr.ref_obj_id = id; + state->stack[spi - 1].spilled_ptr.ref_obj_id = id; } return 0; @@ -779,25 +800,23 @@ static int unmark_stack_slots_dynptr(struct bpf_verifier_env *env, struct bpf_re } /* Invalidate any slices associated with this dynptr */ - if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) { - release_reference(env, state->stack[spi].spilled_ptr.id); - state->stack[spi].spilled_ptr.id = 0; - state->stack[spi - 1].spilled_ptr.id = 0; - } - - state->stack[spi].spilled_ptr.dynptr.first_slot = false; - state->stack[spi].spilled_ptr.dynptr.type = 0; - state->stack[spi - 1].spilled_ptr.dynptr.type = 0; + if (dynptr_type_refcounted(state->stack[spi].spilled_ptr.dynptr.type)) + WARN_ON_ONCE(release_reference(env, state->stack[spi].spilled_ptr.ref_obj_id)); + __mark_reg_not_init(env, &state->stack[spi].spilled_ptr); + __mark_reg_not_init(env, &state->stack[spi - 1].spilled_ptr); return 0; } static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); - int spi = get_spi(reg->off); - int i; + int spi, i; + + if (reg->type == CONST_PTR_TO_DYNPTR) + return false; + spi = get_spi(reg->off); if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS)) return true; @@ -813,9 +832,14 @@ static bool is_dynptr_reg_valid_uninit(struct bpf_verifier_env *env, struct bpf_ static bool is_dynptr_reg_valid_init(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); - int spi = get_spi(reg->off); + int spi; int i; + /* This already represents first slot of initialized bpf_dynptr */ + if (reg->type == CONST_PTR_TO_DYNPTR) + return true; + + spi = get_spi(reg->off); if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) || !state->stack[spi].spilled_ptr.dynptr.first_slot) return false; @@ -834,15 +858,19 @@ static bool is_dynptr_type_expected(struct bpf_verifier_env *env, struct bpf_reg { struct bpf_func_state *state = func(env, reg); enum bpf_dynptr_type dynptr_type; - int spi = get_spi(reg->off); + int spi; /* ARG_PTR_TO_DYNPTR takes any type of dynptr */ if (arg_type == ARG_PTR_TO_DYNPTR) return true; dynptr_type = arg_to_dynptr_type(arg_type); - - return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; + if (reg->type == CONST_PTR_TO_DYNPTR) { + return reg->dynptr.type == dynptr_type; + } else { + spi = get_spi(reg->off); + return state->stack[spi].spilled_ptr.dynptr.type == dynptr_type; + } } /* The reg state of a pointer or a bounded scalar was saved when @@ -1354,9 +1382,6 @@ static const int caller_saved[CALLER_SAVED_REGS] = { BPF_REG_0, BPF_REG_1, BPF_REG_2, BPF_REG_3, BPF_REG_4, BPF_REG_5 }; -static void __mark_reg_not_init(const struct bpf_verifier_env *env, - struct bpf_reg_state *reg); - /* This helper doesn't clear reg->id */ static void ___mark_reg_known(struct bpf_reg_state *reg, u64 imm) { @@ -1419,6 +1444,19 @@ static void mark_reg_known_zero(struct bpf_verifier_env *env, __mark_reg_known_zero(regs + regno); } +static void __mark_dynptr_reg(struct bpf_reg_state *reg, enum bpf_dynptr_type type, + bool first_slot) +{ + /* reg->type has no meaning for STACK_DYNPTR, but when we set reg for + * callback arguments, it does need to be CONST_PTR_TO_DYNPTR, so simply + * set it unconditionally as it is ignored for STACK_DYNPTR anyway. + */ + __mark_reg_known_zero(reg); + reg->type = CONST_PTR_TO_DYNPTR; + reg->dynptr.type = type; + reg->dynptr.first_slot = first_slot; +} + static void mark_ptr_not_null_reg(struct bpf_reg_state *reg) { if (base_type(reg->type) == PTR_TO_MAP_VALUE) { @@ -5857,19 +5895,58 @@ static int process_kptr_func(struct bpf_verifier_env *env, int regno, return 0; } +/* There are two register types representing a bpf_dynptr, one is PTR_TO_STACK + * which points to a stack slot, and the other is CONST_PTR_TO_DYNPTR. + * + * In both cases we deal with the first 8 bytes, but need to mark the next 8 + * bytes as STACK_DYNPTR in case of PTR_TO_STACK. In case of + * CONST_PTR_TO_DYNPTR, we are guaranteed to get the beginning of the object. + * + * Mutability of bpf_dynptr is at two levels, one is at the level of struct + * bpf_dynptr itself, i.e. whether the helper is receiving a pointer to struct + * bpf_dynptr or pointer to const struct bpf_dynptr. In the former case, it can + * mutate the view of the dynptr and also possibly destroy it. In the latter + * case, it cannot mutate the bpf_dynptr itself but it can still mutate the + * memory that dynptr points to. + * + * The verifier will keep track both levels of mutation (bpf_dynptr's in + * reg->type and the memory's in reg->dynptr.type), but there is no support for + * readonly dynptr view yet, hence only the first case is tracked and checked. + * + * This is consistent with how C applies the const modifier to a struct object, + * where the pointer itself inside bpf_dynptr becomes const but not what it + * points to. + * + * Helpers which do not mutate the bpf_dynptr set MEM_RDONLY in their argument + * type, and declare it as 'const struct bpf_dynptr *' in their prototype. + */ int process_dynptr_func(struct bpf_verifier_env *env, int regno, - enum bpf_arg_type arg_type, - struct bpf_call_arg_meta *meta) + enum bpf_arg_type arg_type, struct bpf_call_arg_meta *meta) { struct bpf_reg_state *regs = cur_regs(env), *reg = ®s[regno]; - /* We only need to check for initialized / uninitialized helper - * dynptr args if the dynptr is not PTR_TO_DYNPTR, as the - * assumption is that if it is, that a helper function - * initialized the dynptr on behalf of the BPF program. + /* MEM_UNINIT and MEM_RDONLY are exclusive, when applied to an + * ARG_PTR_TO_DYNPTR (or ARG_PTR_TO_DYNPTR | DYNPTR_TYPE_*): + */ + if ((arg_type & (MEM_UNINIT | MEM_RDONLY)) == (MEM_UNINIT | MEM_RDONLY)) { + verbose(env, "verifier internal error: misconfigured dynptr helper type flags\n"); + return -EFAULT; + } + /* MEM_UNINIT - Points to memory that is an appropriate candidate for + * constructing a mutable bpf_dynptr object. + * + * Currently, this is only possible with PTR_TO_STACK + * pointing to a region of at least 16 bytes which doesn't + * contain an existing bpf_dynptr. + * + * MEM_RDONLY - Points to a initialized bpf_dynptr that will not be + * mutated or destroyed. However, the memory it points to + * may be mutated. + * + * None - Points to a initialized dynptr that can be mutated and + * destroyed, including mutation of the memory it points + * to. */ - if (base_type(reg->type) == PTR_TO_DYNPTR) - return 0; if (arg_type & MEM_UNINIT) { if (!is_dynptr_reg_valid_uninit(env, reg)) { verbose(env, "Dynptr has to be an uninitialized dynptr\n"); @@ -5885,7 +5962,13 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, } meta->uninit_dynptr_regno = regno; - } else { + } else /* MEM_RDONLY and None case from above */ { + /* For the reg->type == PTR_TO_STACK case, bpf_dynptr is never const */ + if (reg->type == CONST_PTR_TO_DYNPTR && !(arg_type & MEM_RDONLY)) { + verbose(env, "cannot pass pointer to const bpf_dynptr, the helper mutates it\n"); + return -EINVAL; + } + if (!is_dynptr_reg_valid_init(env, reg)) { verbose(env, "Expected an initialized dynptr as arg #%d\n", @@ -5893,7 +5976,8 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, return -EINVAL; } - if (!is_dynptr_type_expected(env, reg, arg_type)) { + /* Fold modifiers (in this case, MEM_RDONLY) when checking expected type */ + if (!is_dynptr_type_expected(env, reg, arg_type & ~MEM_RDONLY)) { const char *err_extra = ""; switch (arg_type & DYNPTR_TYPE_FLAG_MASK) { @@ -6056,7 +6140,7 @@ static const struct bpf_reg_types kptr_types = { .types = { PTR_TO_MAP_VALUE } } static const struct bpf_reg_types dynptr_types = { .types = { PTR_TO_STACK, - PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL, + CONST_PTR_TO_DYNPTR, } }; @@ -6241,12 +6325,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); } -static u32 stack_slot_get_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) +static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) { struct bpf_func_state *state = func(env, reg); - int spi = get_spi(reg->off); + int spi; + + if (reg->type == CONST_PTR_TO_DYNPTR) + return reg->ref_obj_id; - return state->stack[spi].spilled_ptr.id; + spi = get_spi(reg->off); + return state->stack[spi].spilled_ptr.ref_obj_id; } static int check_func_arg(struct bpf_verifier_env *env, u32 arg, @@ -6311,11 +6399,22 @@ static int check_func_arg(struct bpf_verifier_env *env, u32 arg, if (arg_type_is_release(arg_type)) { if (arg_type_is_dynptr(arg_type)) { struct bpf_func_state *state = func(env, reg); - int spi = get_spi(reg->off); + int spi; - if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) || - !state->stack[spi].spilled_ptr.id) { - verbose(env, "arg %d is an unacquired reference\n", regno); + /* Only dynptr created on stack can be released, thus + * the get_spi and stack state checks for spilled_ptr + * should only be done before process_dynptr_func for + * PTR_TO_STACK. + */ + if (reg->type == PTR_TO_STACK) { + spi = get_spi(reg->off); + if (!is_spi_bounds_valid(state, spi, BPF_DYNPTR_NR_SLOTS) || + !state->stack[spi].spilled_ptr.ref_obj_id) { + verbose(env, "arg %d is an unacquired reference\n", regno); + return -EINVAL; + } + } else { + verbose(env, "cannot release unowned const bpf_dynptr\n"); return -EINVAL; } } else if (!reg->ref_obj_id && !register_is_null(reg)) { @@ -7289,11 +7388,10 @@ static int set_user_ringbuf_callback_state(struct bpf_verifier_env *env, { /* bpf_user_ringbuf_drain(struct bpf_map *map, void *callback_fn, void * callback_ctx, u64 flags); - * callback_fn(struct bpf_dynptr_t* dynptr, void *callback_ctx); + * callback_fn(const struct bpf_dynptr_t* dynptr, void *callback_ctx); */ __mark_reg_not_init(env, &callee->regs[BPF_REG_0]); - callee->regs[BPF_REG_1].type = PTR_TO_DYNPTR | DYNPTR_TYPE_LOCAL; - __mark_reg_known_zero(&callee->regs[BPF_REG_1]); + mark_dynptr_cb_reg(&callee->regs[BPF_REG_1], BPF_DYNPTR_TYPE_LOCAL); callee->regs[BPF_REG_2] = caller->regs[BPF_REG_3]; /* unused */ @@ -7687,7 +7785,15 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn regs = cur_regs(env); + /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot + * be reinitialized by any dynptr helper. Hence, mark_stack_slots_dynptr + * is safe to do directly. + */ if (meta.uninit_dynptr_regno) { + if (regs[meta.uninit_dynptr_regno].type == CONST_PTR_TO_DYNPTR) { + verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be initialized\n"); + return -EFAULT; + } /* we write BPF_DW bits (8 bytes) at a time */ for (i = 0; i < BPF_DYNPTR_SIZE; i += 8) { err = check_mem_access(env, insn_idx, meta.uninit_dynptr_regno, @@ -7705,15 +7811,24 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn if (meta.release_regno) { err = -EINVAL; - if (arg_type_is_dynptr(fn->arg_type[meta.release_regno - BPF_REG_1])) + /* This can only be set for PTR_TO_STACK, as CONST_PTR_TO_DYNPTR cannot + * be released by any dynptr helper. Hence, unmark_stack_slots_dynptr + * is safe to do directly. + */ + if (arg_type_is_dynptr(fn->arg_type[meta.release_regno - BPF_REG_1])) { + if (regs[meta.release_regno].type == CONST_PTR_TO_DYNPTR) { + verbose(env, "verifier internal error: CONST_PTR_TO_DYNPTR cannot be released\n"); + return -EFAULT; + } err = unmark_stack_slots_dynptr(env, ®s[meta.release_regno]); - else if (meta.ref_obj_id) + } else if (meta.ref_obj_id) { err = release_reference(env, meta.ref_obj_id); - /* meta.ref_obj_id can only be 0 if register that is meant to be - * released is NULL, which must be > R0. - */ - else if (register_is_null(®s[meta.release_regno])) + } else if (register_is_null(®s[meta.release_regno])) { + /* meta.ref_obj_id can only be 0 if register that is meant to be + * released is NULL, which must be > R0. + */ err = 0; + } if (err) { verbose(env, "func %s#%d reference has not been acquired before\n", func_id_name(func_id), func_id); @@ -7787,11 +7902,7 @@ static int check_helper_call(struct bpf_verifier_env *env, struct bpf_insn *insn return -EFAULT; } - if (base_type(reg->type) != PTR_TO_DYNPTR) - /* Find the id of the dynptr we're - * tracking the reference of - */ - meta.ref_obj_id = stack_slot_get_id(env, reg); + meta.ref_obj_id = dynptr_ref_obj_id(env, reg); break; } } @@ -8848,12 +8959,12 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_ break; case KF_ARG_PTR_TO_DYNPTR: if (reg->type != PTR_TO_STACK && - reg->type != PTR_TO_DYNPTR) { + reg->type != CONST_PTR_TO_DYNPTR) { verbose(env, "arg#%d expected pointer to stack or dynptr_ptr\n", i); return -EINVAL; } - ret = process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR, NULL); + ret = process_dynptr_func(env, regno, ARG_PTR_TO_DYNPTR | MEM_RDONLY, NULL); if (ret < 0) return ret; break; diff --git a/scripts/bpf_doc.py b/scripts/bpf_doc.py index fdb0aff8cb5a..e8d90829f23e 100755 --- a/scripts/bpf_doc.py +++ b/scripts/bpf_doc.py @@ -752,6 +752,7 @@ class PrinterHelpers(Printer): 'struct bpf_timer', 'struct mptcp_sock', 'struct bpf_dynptr', + 'const struct bpf_dynptr', 'struct iphdr', 'struct ipv6hdr', } diff --git a/tools/include/uapi/linux/bpf.h b/tools/include/uapi/linux/bpf.h index f89de51a45db..464ca3f01fe7 100644 --- a/tools/include/uapi/linux/bpf.h +++ b/tools/include/uapi/linux/bpf.h @@ -5293,7 +5293,7 @@ union bpf_attr { * Return * Nothing. Always succeeds. * - * long bpf_dynptr_read(void *dst, u32 len, struct bpf_dynptr *src, u32 offset, u64 flags) + * long bpf_dynptr_read(void *dst, u32 len, const struct bpf_dynptr *src, u32 offset, u64 flags) * Description * Read *len* bytes from *src* into *dst*, starting from *offset* * into *src*. @@ -5303,7 +5303,7 @@ union bpf_attr { * of *src*'s data, -EINVAL if *src* is an invalid dynptr or if * *flags* is not 0. * - * long bpf_dynptr_write(struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags) + * long bpf_dynptr_write(const struct bpf_dynptr *dst, u32 offset, void *src, u32 len, u64 flags) * Description * Write *len* bytes from *src* into *dst*, starting from *offset* * into *dst*. @@ -5313,7 +5313,7 @@ union bpf_attr { * of *dst*'s data, -EINVAL if *dst* is an invalid dynptr or if *dst* * is a read-only dynptr or if *flags* is not 0. * - * void *bpf_dynptr_data(struct bpf_dynptr *ptr, u32 offset, u32 len) + * void *bpf_dynptr_data(const struct bpf_dynptr *ptr, u32 offset, u32 len) * Description * Get a pointer to the underlying dynptr data. * @@ -5414,7 +5414,7 @@ union bpf_attr { * Drain samples from the specified user ring buffer, and invoke * the provided callback for each such sample: * - * long (\*callback_fn)(struct bpf_dynptr \*dynptr, void \*ctx); + * long (\*callback_fn)(const struct bpf_dynptr \*dynptr, void \*ctx); * * If **callback_fn** returns 0, the helper will continue to try * and drain the next sample, up to a maximum of diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c index 02b18d018b36..aefa0a474e58 100644 --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c @@ -673,8 +673,8 @@ static struct { {"user_ringbuf_callback_write_forbidden", "invalid mem access 'dynptr_ptr'"}, {"user_ringbuf_callback_null_context_write", "invalid mem access 'scalar'"}, {"user_ringbuf_callback_null_context_read", "invalid mem access 'scalar'"}, - {"user_ringbuf_callback_discard_dynptr", "arg 1 is an unacquired reference"}, - {"user_ringbuf_callback_submit_dynptr", "arg 1 is an unacquired reference"}, + {"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"}, + {"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"}, {"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"}, }; From patchwork Wed Dec 7 20:41:38 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067591 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 9D7E3C4708D for ; Wed, 7 Dec 2022 20:42:04 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229847AbiLGUmD (ORCPT ); Wed, 7 Dec 2022 15:42:03 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55760 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229949AbiLGUmA (ORCPT ); Wed, 7 Dec 2022 15:42:00 -0500 Received: from mail-pl1-x641.google.com (mail-pl1-x641.google.com [IPv6:2607:f8b0:4864:20::641]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 235F845085 for ; Wed, 7 Dec 2022 12:41:59 -0800 (PST) Received: by mail-pl1-x641.google.com with SMTP id jn7so18146199plb.13 for ; Wed, 07 Dec 2022 12:41:59 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=knaUyolnxJAvd8/eFssmGbs+/xV2LhLuI7H5j6fS97E=; b=pGlGqUxvdDYsHCzi+UWu6qYVJ6zELFXkm1GrYqA6EH71qh6AemADUu+v+1minTjDE0 QeQ8290VUH2OnGURkPOT9NOYTwmWyf8BSRsz4NKpnJF1hFC2G4n1HJnNXZzKSwM1i6XF i/xBZvHq1axBXi9l3sFhUfg4WkxQy0b9qUdKZ1RdW0WUuDnzplrtrf8NIyVXVAm3QUOM lrdLt2cwpsqp7pXbulB1pUPT7qUYphS/U1RT7v1nqQieAYvNqX6eIMCDt19cHPmGa3Ql ONWX9ZXsN9TP5bMo/IGgjn8/Imym+yhvQZbpeNyVr3gtsimfoFCkh+NQsiu5u1GNFT6+ mJHQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=knaUyolnxJAvd8/eFssmGbs+/xV2LhLuI7H5j6fS97E=; b=DfNvLnvPB4J4HtdaG3R0z8zdRLRIauq6oPhiT5PvtMNimB8TidnbBHj5mxf0sW0dUb a20eLXGrqdjyVNN7PnTWP+oOPH2yywfenajyLYqZuF0H2guy08YgHGTsS9DOmUFXB9Kr LQt3KVoDKKz08sOpRWc7Cc2AwVfnhkzsZBQ5vBfOmwUsfRCZoQ/uVP4Xy3TQUkd/Zl6o g+65z1NC2eYL7bhrkj6ncKbN3gB53pvp0jQMr5yVCObPUv1jDTyH0rE+go3sAjTKUEu2 YF8bajWgEcxEtoaMa+H6AEtLbijmFFvgMn8lwCwNPQ2zhvt750rluYXdP3T4Iint4iOV wgJA== X-Gm-Message-State: ANoB5plY2V3BXqqCLBQjKxcHaU5w3tigBUdZkPJK6DFRLiWQiFD3BkEN Ul1VdPLIzAIGUjOqptG06jpAeFvhZ+j61sog X-Google-Smtp-Source: AA0mqf7dpdLsM86aPmm1GnQHpkMaHA7+DMX0eqMObmxo6xCGWcB3VIaWbZD++2Mu56wzqATmneDG8g== X-Received: by 2002:a17:903:1341:b0:189:9a36:a5a8 with SMTP id jl1-20020a170903134100b001899a36a5a8mr42572253plb.151.1670445718218; Wed, 07 Dec 2022 12:41:58 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id x14-20020a170902a38e00b0017f36638010sm14919318pla.276.2022.12.07.12.41.57 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:41:57 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: Joanne Koong , David Vernet , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau Subject: [PATCH bpf-next v2 4/7] bpf: Rework check_func_arg_reg_off Date: Thu, 8 Dec 2022 02:11:38 +0530 Message-Id: <20221207204141.308952-5-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=7324; i=memxor@gmail.com; h=from:subject; bh=vMfVcMCU2fw+LZ7cLmesTLvOwK5XSQr2Rp7hNnX0sp4=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOoK9DUOYjXP+91W1OsmBY1XjziCe3bkvWrLg8j /3A6fMeJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqAAKCRBM4MiGSL8RyiffD/ 9Uk4M5/EMJV9O80AD3RSa/yxB50HckKxBXMepji1feZxewqagdGCUL0sdAGPLWrvP2MrlNMCDHYAR6 BQLEYLmh2Y5ISu9kP3CvzNnECTfX/m6ekLPEH/T5hiS855m62CmW+XV80A0CiSkyi8+n1yOy3mqiAt QweuI6YIWu8w1rs3FqW3fdYp93R5AhPVbdH4z9+gFuAdY21SuL5W/TySMkhmzVKY+01RHyXCDTLtgT +tqtJjVyHeOV0UBxC8TjMzhzUPoEtV/loz8TG6nBziDKDXsmJVASew2xzzHJSwaX++4Sj/ABgkPKSY PAJ4VWZ++0FZulHzevNFeRwUX+SR1mEGca4n/zPyQ0ZsMNB4uHA02/kktbiomeZr6aP6Gen53LSOXx /h5IOiAFhLBIvjORUydzlvrft51TNz34n/kO25mCM+ZdgC/7PIhaZvkACSfS5VQqJqFV9Yv+qhogB1 m+yigT4A+Gmtg//cSOLpA7hF8GMu4TVDSr1UBjUA5i7ZINatNw43C6rpjUEmKEoJQn7klGrNLUE3R7 w85b1Ohae0r1KyzlmFqxkHFXsWF/RlGMLhlOnVAddRb2r3oWgwuysj9vnFi5kip3Gikp10e3GxDUF8 /FfpCpwkBCcj6egLsWUPpOq7ISzPPQwvf4BNKkIIhXK/8mLzUjnpnsFCIFUg== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net While check_func_arg_reg_off is the place which performs generic checks needed by various candidates of reg->type, there is some handling for special cases, like ARG_PTR_TO_DYNPTR, OBJ_RELEASE, and ARG_PTR_TO_ALLOC_MEM. This commit aims to streamline these special cases and instead leave other things up to argument type specific code to handle. The function will be restrictive by default, and cover all possible cases when OBJ_RELEASE is set, without having to update the function again (and missing to do that being a bug). This is done primarily for two reasons: associating back reg->type to its argument leaves room for the list getting out of sync when a new reg->type is supported by an arg_type. The other case is ARG_PTR_TO_ALLOC_MEM. The problem there is something we already handle, whenever a release argument is expected, it should be passed as the pointer that was received from the acquire function. Hence zero fixed and variable offset. There is nothing special about ARG_PTR_TO_ALLOC_MEM, where technically its target register type PTR_TO_MEM | MEM_ALLOC can already be passed with non-zero offset to other helper functions, which makes sense. Hence, lift the arg_type_is_release check for reg->off and cover all possible register types, instead of duplicating the same kind of check twice for current OBJ_RELEASE arg_types (alloc_mem and ptr_to_btf_id). For the release argument, arg_type_is_dynptr is the special case, where we go to actual object being freed through the dynptr, so the offset of the pointer still needs to allow fixed and variable offset and process_dynptr_func will verify them later for the release argument case as well. This is not specific to ARG_PTR_TO_DYNPTR though, we will need to make this exception for any future object on the stack that needs to be released. In this sense, PTR_TO_STACK as a candidate for object on stack argument is a special case for release offset checks, and they need to be done by the helper releasing the object on stack. Since the check has been lifted above all register type checks, remove the duplicated check that is being done for PTR_TO_BTF_ID. Acked-by: Joanne Koong Acked-by: David Vernet Signed-off-by: Kumar Kartikeya Dwivedi --- kernel/bpf/verifier.c | 64 +++++++++++-------- tools/testing/selftests/bpf/verifier/calls.c | 2 +- .../testing/selftests/bpf/verifier/ringbuf.c | 2 +- 3 files changed, 41 insertions(+), 27 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index fdbaf22cdaf2..cadcf0233326 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -6269,11 +6269,38 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, const struct bpf_reg_state *reg, int regno, enum bpf_arg_type arg_type) { - enum bpf_reg_type type = reg->type; - bool fixed_off_ok = false; + u32 type = reg->type; - switch ((u32)type) { - /* Pointer types where reg offset is explicitly allowed: */ + /* When referenced register is passed to release function, its fixed + * offset must be 0. + * + * We will check arg_type_is_release reg has ref_obj_id when storing + * meta->release_regno. + */ + if (arg_type_is_release(arg_type)) { + /* ARG_PTR_TO_DYNPTR with OBJ_RELEASE is a bit special, as it + * may not directly point to the object being released, but to + * dynptr pointing to such object, which might be at some offset + * on the stack. In that case, we simply to fallback to the + * default handling. + */ + if (!arg_type_is_dynptr(arg_type) || type != PTR_TO_STACK) { + /* Doing check_ptr_off_reg check for the offset will + * catch this because fixed_off_ok is false, but + * checking here allows us to give the user a better + * error message. + */ + if (reg->off) { + verbose(env, "R%d must have zero offset when passed to release func or trusted arg to kfunc\n", + regno); + return -EINVAL; + } + return __check_ptr_off_reg(env, reg, regno, false); + } + /* Fallback to default handling */ + } + switch (type) { + /* Pointer types where both fixed and variable offset is explicitly allowed: */ case PTR_TO_STACK: if (arg_type_is_dynptr(arg_type) && reg->off % BPF_REG_SIZE) { verbose(env, "cannot pass in dynptr at an offset\n"); @@ -6290,12 +6317,7 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, case PTR_TO_BUF: case PTR_TO_BUF | MEM_RDONLY: case SCALAR_VALUE: - /* Some of the argument types nevertheless require a - * zero register offset. - */ - if (base_type(arg_type) != ARG_PTR_TO_RINGBUF_MEM) - return 0; - break; + return 0; /* All the rest must be rejected, except PTR_TO_BTF_ID which allows * fixed offset. */ @@ -6305,24 +6327,16 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, case PTR_TO_BTF_ID | MEM_RCU: case PTR_TO_BTF_ID | MEM_ALLOC | PTR_TRUSTED: /* When referenced PTR_TO_BTF_ID is passed to release function, - * it's fixed offset must be 0. In the other cases, fixed offset - * can be non-zero. + * its fixed offset must be 0. In the other cases, fixed offset + * can be non-zero. This was already checked above. So pass + * fixed_off_ok as true to allow fixed offset for all other + * cases. var_off always must be 0 for PTR_TO_BTF_ID, hence we + * still need to do checks instead of returning. */ - if (arg_type_is_release(arg_type) && reg->off) { - verbose(env, "R%d must have zero offset when passed to release func\n", - regno); - return -EINVAL; - } - /* For arg is release pointer, fixed_off_ok must be false, but - * we already checked and rejected reg->off != 0 above, so set - * to true to allow fixed offset for all other cases. - */ - fixed_off_ok = true; - break; + return __check_ptr_off_reg(env, reg, regno, true); default: - break; + return __check_ptr_off_reg(env, reg, regno, false); } - return __check_ptr_off_reg(env, reg, regno, fixed_off_ok); } static u32 dynptr_ref_obj_id(struct bpf_verifier_env *env, struct bpf_reg_state *reg) diff --git a/tools/testing/selftests/bpf/verifier/calls.c b/tools/testing/selftests/bpf/verifier/calls.c index 3193915c5ee6..babcec123251 100644 --- a/tools/testing/selftests/bpf/verifier/calls.c +++ b/tools/testing/selftests/bpf/verifier/calls.c @@ -76,7 +76,7 @@ }, .prog_type = BPF_PROG_TYPE_SCHED_CLS, .result = REJECT, - .errstr = "arg#0 expected pointer to ctx, but got PTR", + .errstr = "R1 must have zero offset when passed to release func or trusted arg to kfunc", .fixup_kfunc_btf_id = { { "bpf_kfunc_call_test_pass_ctx", 2 }, }, diff --git a/tools/testing/selftests/bpf/verifier/ringbuf.c b/tools/testing/selftests/bpf/verifier/ringbuf.c index 84838feba47f..92e3f6a61a79 100644 --- a/tools/testing/selftests/bpf/verifier/ringbuf.c +++ b/tools/testing/selftests/bpf/verifier/ringbuf.c @@ -28,7 +28,7 @@ }, .fixup_map_ringbuf = { 1 }, .result = REJECT, - .errstr = "dereference of modified ringbuf_mem ptr R1", + .errstr = "R1 must have zero offset when passed to release func", }, { "ringbuf: invalid reservation offset 2", From patchwork Wed Dec 7 20:41:39 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067593 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 A8914C63708 for ; Wed, 7 Dec 2022 20:42:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229868AbiLGUmE (ORCPT ); Wed, 7 Dec 2022 15:42:04 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55766 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229853AbiLGUmD (ORCPT ); Wed, 7 Dec 2022 15:42:03 -0500 Received: from mail-pg1-x543.google.com (mail-pg1-x543.google.com [IPv6:2607:f8b0:4864:20::543]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9CC6430558 for ; Wed, 7 Dec 2022 12:42:02 -0800 (PST) Received: by mail-pg1-x543.google.com with SMTP id h33so17402183pgm.9 for ; Wed, 07 Dec 2022 12:42:02 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=5kwACFs/56Sr1TbaQVBLjvh3U0y1UG0rc5mW2Tqi41s=; b=BeSC+nLCtrw1dUP40A3A9r6Fl2460eBMLkd8OUWIRpnK7O8khsVaNImGdCgjDJVhne a3TcWPB2d9sVo1YInFls/XXPX8BaymsXfAuBcVWq7w135k7wKF8Xph7jAAk4PiFUrUQ1 Ho11Ww3NS+ri3PwimBPF7EWMorAkZ1dXRyX1TNCWyp1GgMiOAavBfGlO+xc0keI303mN 4sWPyMCTxCg18kia55DnqhHB9R9z9NkaNFAcbT2kCKArt4gBYvN3DniL4mApeteitgj4 a45CmZdUs591cOLjR/O5/xAdAK0P918FhOY+DwO8XTlUoHu2Jq4k50g6ppoUKU0vvVAd 7zTw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=5kwACFs/56Sr1TbaQVBLjvh3U0y1UG0rc5mW2Tqi41s=; b=1LLH2NZDH+TAqPegJFC419sW48Dlplx/XVZeokHMNIJUC1nXZLD9J4JC2aSIYUKwXD mhyro6TsrazZB865u74FAY0Ax73MjpBpeHgE2FDTKCL2wquFpCs2C/C1DApC9OdpszzD y5otSb5W5O0YOs0UgF6cfoKKILxbNEXkdyzX/xZwYA2CpaB8kTty1CS9YsIrpRRsWxAJ 13eiL90OKAqywu6ZJHVQQbKHt/8nzJpv+95nKM4JfHBbsboUOWmhF0+lrq/DiGBeIaUE 3c15N/cej+ojKypY6BnGNmynxtRAQrC+i+DscJgk5CTX6psiPYjYaZxN7UAlk6wjqey0 8LHg== X-Gm-Message-State: ANoB5pmqlZBFM3shjlb1bK178pRmO973SbRVfgHJlh5bNy1kPHP6Mbxn rDH/D8n5aL/ymOGpdsKXJlIcV1SPbtWrJiTC X-Google-Smtp-Source: AA0mqf4nyCuJK5KZ8/I0q50Y9QUNVyck7fXFYEqvK+dsLODr5gdwmPE+VUHZXPtByPQiB415/m4zKA== X-Received: by 2002:a63:1f09:0:b0:478:fd9b:b6c8 with SMTP id f9-20020a631f09000000b00478fd9bb6c8mr2376041pgf.509.1670445721938; Wed, 07 Dec 2022 12:42:01 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id c23-20020a170902b69700b0018958a913a2sm14930923pls.223.2022.12.07.12.42.01 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:42:01 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: Joanne Koong , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau , David Vernet Subject: [PATCH bpf-next v2 5/7] bpf: Move PTR_TO_STACK alignment check to process_dynptr_func Date: Thu, 8 Dec 2022 02:11:39 +0530 Message-Id: <20221207204141.308952-6-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=2294; i=memxor@gmail.com; h=from:subject; bh=u4A5lAfb9eBYzsDtC/pHkdFQ7LgH+0ur5obBt+XfFRs=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOpD2ObU5oQ6Bs51Io7RP4XAjcmRoaYLK3bGIu0 aqxYdJyJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqQAKCRBM4MiGSL8RytlBEA Czv/71sdhwlgNzz+atyE+a1ZkfW3j4wkjOjzqXht5d7xIl/+1qxMdfljCnO5emmh2jQTR/UFVbHVLF /+9gcOGVmb/hNdMnfCn4KstAx0UH+DhmSPYR0S16prSmgJ8UzmCK0lvdht3ghviYuddxowJXpu5S4G aDUjpyhPKYVOg0AFEYVKJnSdlFnwAUZNImkrIu2W6WlGrhDcnyrC/G4ZPZso3MpVAt+wbble/YNl8e yNEOux91AdJ+0jIifuIgsX2I6MAVn0GELn95AO96Nz5yJTuckokLMOeFIKCmWvPPiRblWQ6rCNfXN3 LyZ8HPbY+sbMYPGoHvfAzTKqghT7cxw8PzE+KLTrIP1/FC7SSrhf9sKexhLKnfBdt0l+Uhbvea+N2Q dyuh46/M5SGyTQlAXjL5bUTv2H2xL7RSD78E6+ZWbrqyfPCwsr3IZeOGyMPVbBnJK5gDhAfLErlteH PabBIqxrsGdmgVcGxfP4JRkUoJdKQwM/Ig0J7PChO9cylAK/JP+BkNcW6VTjE8vn/OYEVQ+gcmOQnE W2FO1nR7aXTLpWQXnUJiVWJnPcAgVYPVCQVmA8zm5cDXPls0iO+SWFdX8aoith8aUn12DWLhYCOZIk VdDDquTIn4Jhpc3cK8ZOaVw135USIubScw5iK/hzzthybJeW6Loa1oI5gBBg== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net After previous commit, we are minimizing helper specific assumptions from check_func_arg_reg_off, making it generic, and offloading checks for a specific argument type to their respective functions called after check_func_arg_reg_off has been called. This allows relying on a consistent set of guarantees after that call and then relying on them in code that deals with registers for each argument type later. This is in line with how process_spin_lock, process_timer_func, process_kptr_func check reg->var_off to be constant. The same reasoning is used here to move the alignment check into process_dynptr_func. Note that it also needs to check for constant var_off, and accumulate the constant var_off when computing the spi in get_spi, but that fix will come in later changes. Acked-by: Joanne Koong Signed-off-by: Kumar Kartikeya Dwivedi --- kernel/bpf/verifier.c | 13 ++++++++----- 1 file changed, 8 insertions(+), 5 deletions(-) diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c index cadcf0233326..e10a21cd21a8 100644 --- a/kernel/bpf/verifier.c +++ b/kernel/bpf/verifier.c @@ -5932,6 +5932,14 @@ int process_dynptr_func(struct bpf_verifier_env *env, int regno, verbose(env, "verifier internal error: misconfigured dynptr helper type flags\n"); return -EFAULT; } + /* CONST_PTR_TO_DYNPTR already has fixed and var_off as 0 due to + * check_func_arg_reg_off's logic. We only need to check offset + * alignment for PTR_TO_STACK. + */ + if (reg->type == PTR_TO_STACK && (reg->off % BPF_REG_SIZE)) { + verbose(env, "cannot pass in dynptr at an offset=%d\n", reg->off); + return -EINVAL; + } /* MEM_UNINIT - Points to memory that is an appropriate candidate for * constructing a mutable bpf_dynptr object. * @@ -6302,11 +6310,6 @@ int check_func_arg_reg_off(struct bpf_verifier_env *env, switch (type) { /* Pointer types where both fixed and variable offset is explicitly allowed: */ case PTR_TO_STACK: - if (arg_type_is_dynptr(arg_type) && reg->off % BPF_REG_SIZE) { - verbose(env, "cannot pass in dynptr at an offset\n"); - return -EINVAL; - } - fallthrough; case PTR_TO_PACKET: case PTR_TO_PACKET_META: case PTR_TO_MAP_KEY: From patchwork Wed Dec 7 20:41:40 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067594 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 7A7ACC63705 for ; Wed, 7 Dec 2022 20:42:08 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229853AbiLGUmH (ORCPT ); Wed, 7 Dec 2022 15:42:07 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55814 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229876AbiLGUmG (ORCPT ); Wed, 7 Dec 2022 15:42:06 -0500 Received: from mail-pl1-x644.google.com (mail-pl1-x644.google.com [IPv6:2607:f8b0:4864:20::644]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id D73144AF36 for ; Wed, 7 Dec 2022 12:42:05 -0800 (PST) Received: by mail-pl1-x644.google.com with SMTP id a9so18150818pld.7 for ; Wed, 07 Dec 2022 12:42:05 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=7CDnX8EoSYIgJ6yK465jEVxSIns6OgEgCzmYMYB1z3s=; b=eIhIsifOtuDDT/e3ofYkkhM18fyqadxfgrLcPSEelEDct7akvmRXj6n8D0G/GMJEyP 1tJqX5dHkUJQ4ZSXLqi7JDFojlNHy3sOsj7WdAJ7p9frQjfYt6LtFxIB1Psj22bcnb6+ TvWDxbVIuxvuElXkT6Qy8dki5tbqSjR3EY/yd3XVdvE7A6tp6xEdJBcuvbMKj4S4hvkx 0iybXbD4RoqD1Um3QOo/7Vyl7pisCzdTC9VyK24BswyBxHcosXc1CrM99ftB9k26rX9R vJRXEdqAdZ4xBxFLk8jQC79+jIJaKw2YbuA6BwqqMTTgsSFMjeNx0/4hCr3IQVFryzNW z15A== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=7CDnX8EoSYIgJ6yK465jEVxSIns6OgEgCzmYMYB1z3s=; b=FpcUePM6zPoeST1GM929NkqRgiM+Ap35EHuVmg5Y/amOR2AYscP2Z75Fe2IeE3ZlwA 2jFCgKdznY25viHVJrNlbHPiotjqEWTpLvMRxk7XhM7fuI57iYE/vc9Guc0dyY1/1byM mctsRd3877T4zavB9YyyozUQO4/T+t75xKiPw3jXMRyUIaq9a2HXTf2I5oBmmEZYr/a9 eBMcrDV574xsO51GiUpHoxVESSkxxSdUUmAROOcCcoyIceZ1EA3+hguPDIOX6zNWJTrV 6UdsmzffnUsYXlGVsWy62zLq+aw/Wp+NO2zkOeLwHaorSIH4rKaC4Bw8wA64mCKkJzvj tbZA== X-Gm-Message-State: ANoB5pm3xc2fykyzNPLGCi/HUK7L7NK3sw6DhmozJLmpito1lIUXnsx9 riw6E6ncMp1ku1ePl9QRJqi771g3m04dGEyQ X-Google-Smtp-Source: AA0mqf7Dr8r5IacTjidNpD+jopyjSrG7WYrYZi8/wp/tW5cvTPcTkqF5D/jcWHI7SMqDLow3shh+jA== X-Received: by 2002:a17:902:820b:b0:188:9ae7:bb7d with SMTP id x11-20020a170902820b00b001889ae7bb7dmr79837304pln.113.1670445725094; Wed, 07 Dec 2022 12:42:05 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id t26-20020aa7947a000000b005779110635asm1507355pfq.51.2022.12.07.12.42.04 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:42:04 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: Joanne Koong , David Vernet , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau Subject: [PATCH bpf-next v2 6/7] bpf: Use memmove for bpf_dynptr_{read,write} Date: Thu, 8 Dec 2022 02:11:40 +0530 Message-Id: <20221207204141.308952-7-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=1727; i=memxor@gmail.com; h=from:subject; bh=LDqr3sPsObi4Z0BDbi2OYbns1fiOWuI8Zt27lLRBGeU=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOpHzNFoUpEXusxB1IzJKKFNtQBS0EVx1Qhy5ZD DVAs7V6JAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqQAKCRBM4MiGSL8RyuLkD/ 9BjPxLkmQrKIsQIwOYCNgQfrdC8Trbw4NCwWMxJC+UPqVYf+ikkwxggOSCuH4kuVJ1cC9a5eCm/lGi 0sZeolysKiEC3vjdQ0ysHwZHAb5CKg0xLV/QnOkVe4kUhPooplKyGgNupHqTsGx0Q2ZOKEYgKmwosO xJsr59DgShEunwi/g0XMFjBYf4HXGa0txczMOZ7/M3G8uBQPmY8LMJW6S3gooTL48rgf1/yi2/GzJU jw4uWH8d6zDS8lOYIuHiHx1R2wh0UuZDhysrrLXZEAw7+Ngh4ecIEBsoAzsELFqu5sFVPmiFyH6mK6 0CNInB7EHDclrtqVNhif2w8IyZD0DVW84YXmJjgisgZLlJqyV6Xb7AUgb09VOH66iqTGjXQ2S/MpWi /dcP5G/HompBnNGox3dOXwwxjI6kHf4Ow/B/pzi+klyL3Rold/UhwFxbsQpsVcQfFCjS5osHEsE+Se MMxAOFhXNkZTxF6RRiyQsXMQOvC69ABO6ptzAOY0JxN2K0CsyQENsG8CUz3X0JedO7r1sJ42iXCG3w 9yvL7GWlGfx+PK6viPO+fgCtDxz2uNX/elJxTzIolcW7q4SW2uuB0tcpCUlSe2CY+lQpyLcAuoRQHQ OsC9OfnGCfYSso2QbMYN1z2vMblXYSfbtMfgnk0zCxrlt0h1jPHn7UAMmmaw== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net It may happen that destination buffer memory overlaps with memory dynptr points to. Hence, we must use memmove to correctly copy from dynptr to destination buffer, or source buffer to dynptr. This actually isn't a problem right now, as memcpy implementation falls back to memmove on detecting overlap and warns about it, but we shouldn't be relying on that. Acked-by: Joanne Koong Acked-by: David Vernet Signed-off-by: Kumar Kartikeya Dwivedi --- kernel/bpf/helpers.c | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/kernel/bpf/helpers.c b/kernel/bpf/helpers.c index bf9a6a646254..842229671af0 100644 --- a/kernel/bpf/helpers.c +++ b/kernel/bpf/helpers.c @@ -1495,7 +1495,11 @@ BPF_CALL_5(bpf_dynptr_read, void *, dst, u32, len, const struct bpf_dynptr_kern if (err) return err; - memcpy(dst, src->data + src->offset + offset, len); + /* Source and destination may possibly overlap, hence use memmove to + * copy the data. E.g. bpf_dynptr_from_mem may create two dynptr + * pointing to overlapping PTR_TO_MAP_VALUE regions. + */ + memmove(dst, src->data + src->offset + offset, len); return 0; } @@ -1523,7 +1527,11 @@ BPF_CALL_5(bpf_dynptr_write, const struct bpf_dynptr_kern *, dst, u32, offset, v if (err) return err; - memcpy(dst->data + dst->offset + offset, src, len); + /* Source and destination may possibly overlap, hence use memmove to + * copy the data. E.g. bpf_dynptr_from_mem may create two dynptr + * pointing to overlapping PTR_TO_MAP_VALUE regions. + */ + memmove(dst->data + dst->offset + offset, src, len); return 0; } From patchwork Wed Dec 7 20:41:41 2022 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kumar Kartikeya Dwivedi X-Patchwork-Id: 13067595 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 2FC36C63705 for ; Wed, 7 Dec 2022 20:42:13 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S229875AbiLGUmL (ORCPT ); Wed, 7 Dec 2022 15:42:11 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:55874 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229876AbiLGUmK (ORCPT ); Wed, 7 Dec 2022 15:42:10 -0500 Received: from mail-pf1-x443.google.com (mail-pf1-x443.google.com [IPv6:2607:f8b0:4864:20::443]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 2719530558 for ; Wed, 7 Dec 2022 12:42:09 -0800 (PST) Received: by mail-pf1-x443.google.com with SMTP id x66so18532314pfx.3 for ; Wed, 07 Dec 2022 12:42:09 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; 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=01wauH19eqmzO4s5n5a0PMWF5ZMamX+v9IeWG7qrBA4=; b=DMT/zHN6OOwxU2R61XCl0XP8ml2rluCzHotD5KBI+eZHb2iY9Rma5U46CMxOblNXCZ Q6x/mqDDb0ysPihqEK4hjQvSQbFfz08Ss1NZDJZqXS2CUZMWeMMADUdfx3ai4Lr0PH1P 2VXz3qXDJrZL55oqdRCN09s6TwbYSTeuJlC9vqD7yvB5+eT1quDp/K33Aak4+rID8dNW AQ1x7vGcUghCQHHoT0SwHl7j+6H1QeLutqiWEqIc/Jti+gSQQkS2Iqrr4wx34pcKgEE1 9i/qujMEnsXNQ8JKJtWFmI32bjXFGRKANKubfq86zxxLSpNwaC9LQMN0GzEoO6QgAqtj hirQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; 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=01wauH19eqmzO4s5n5a0PMWF5ZMamX+v9IeWG7qrBA4=; b=ancwnPAk6ZxuSYuKGyGlJ89p4NFj0iLRXQc+ibTZuLsgmlNnYekN3+aYzFSupLGtFh Fxjql+tV5LGoL3qr2VVuhQtViioPoEfhgL+bVF0hahr3iBwkBegGpns5Cll0nTuT4etf kJ1SHFuB+cE+ad8cOBbafjcerrbBiy7R1UdwzuQVMcIi9T/89cBY3d86SZGAn2LCbotq nLF/wcpLvRXKDLGoQyZSACWDciYxq1y1pXj5uzdKNaf3edYZo3Q5gs1WCX22DJmAJX7A Ed2oyIuPx0TJIu3r7ntSDsnNQ97bqhbfIbZq21XSns1MEtX9nMTFF/ZY3JrIquuUC2o2 GBiQ== X-Gm-Message-State: ANoB5plfjXGGMC0fuc8SPvtrEeHBQciqwYkCyfNyp6ql2dwzi2gVllvE IZPB7D1rFRY4iBqPb4GuZ4zXvYBAV6vwiRB3 X-Google-Smtp-Source: AA0mqf5GIe7QpOodQ8ccv5eGGDOuJdnlyMOJK+Rbl/2ZcEXkK4LAk0Of/vepWsmzbGbKuFeJ8HtL6Q== X-Received: by 2002:a63:921b:0:b0:478:f30b:8f5f with SMTP id o27-20020a63921b000000b00478f30b8f5fmr3986237pgd.527.1670445728350; Wed, 07 Dec 2022 12:42:08 -0800 (PST) Received: from localhost ([2405:201:6014:d8bc:6259:1a87:ebdd:30a7]) by smtp.gmail.com with ESMTPSA id d9-20020aa797a9000000b00576489088c7sm8302771pfq.37.2022.12.07.12.42.07 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 07 Dec 2022 12:42:08 -0800 (PST) From: Kumar Kartikeya Dwivedi To: bpf@vger.kernel.org Cc: David Vernet , Joanne Koong , Alexei Starovoitov , Andrii Nakryiko , Daniel Borkmann , Martin KaFai Lau Subject: [PATCH bpf-next v2 7/7] selftests/bpf: Add test for dynptr reinit in user_ringbuf callback Date: Thu, 8 Dec 2022 02:11:41 +0530 Message-Id: <20221207204141.308952-8-memxor@gmail.com> X-Mailer: git-send-email 2.38.1 In-Reply-To: <20221207204141.308952-1-memxor@gmail.com> References: <20221207204141.308952-1-memxor@gmail.com> MIME-Version: 1.0 X-Developer-Signature: v=1; a=openpgp-sha256; l=6299; i=memxor@gmail.com; h=from:subject; bh=KrAgt8E55V4soRSbWm0Gn0wfos9fuJxLAXqsuZnNVWE=; b=owEBbQKS/ZANAwAIAUzgyIZIvxHKAcsmYgBjkPOpOjVosuvB+BMgAIfcdG2PRmInjvZ9DD/P6F7H ElGvnuCJAjMEAAEIAB0WIQRLvip+Buz51YI8YRFM4MiGSL8RygUCY5DzqQAKCRBM4MiGSL8RylarEA CLloRoyGbQDhI44s5Mt5NVxpE+xCmY0ifkaJe0wDUNgS79Bp+dVNdEdcU3M6qNQTg6lFzH0ouldx9F FlJ21MvOd6GmeBcD7W0OZnBRDzqDZw2N/rPbUaRDuniwPLWKl3oNAqJeRHDpHpw7/4xiGWfw4Zw1W5 YZUqVZHcHLPz8IErV/rNZKVfBWXnY2xf3Om2RQasB6gayNDbRoGZlK0+a/9RZkMC8ooGn5L4qBzbjU /ocQXCYxN5nGrLAMOoTrR6qXdjlCz64dSk0MOkp4ehTU7kkYrsHuiUCsGfP7UYd+IrbClp4fBABkHi 7nr0Tik5z6oYV/GIQsBknJxYbNi/+1lgN+VhiEEqD0e1zh4p9O1VfLReL1pmDGaujIyX4qzJZtIRSj V/xu+ZrxEwrU/wfDRivyow+IkBsJIyCoRwUhWEnGcXziRYFUQjwfGKKVFV7wWyOaw3OhpvHIANb6xs Sv39EJ+6vaRmusCpZLMytWquXwiqhPqjByLYRl3f7E4K5iCa6YmPPgRXijTCqLPVTZgv73CcTgSknl qLmX3zdOrPnKK51CYyYgWNu3hVryBJULssS7EVNmZd7kIxF+NQK9ZgxzXUqLz7DYEe5mzs5t9nFvo/ Hay8hB3UVVq3Vy3PFa+O+WjlHIL/mp9ROWxm02irTp47RcZIp+lmDbukg+kg== X-Developer-Key: i=memxor@gmail.com; a=openpgp; fpr=4BBE2A7E06ECF9D5823C61114CE0C88648BF11CA Precedence: bulk List-ID: X-Mailing-List: bpf@vger.kernel.org X-Patchwork-Delegate: bpf@iogearbox.net The original support for bpf_user_ringbuf_drain callbacks simply short-circuited checks for the dynptr state, allowing users to pass PTR_TO_DYNPTR (now CONST_PTR_TO_DYNPTR) to helpers that initialize a dynptr. This bug would have also surfaced with other dynptr helpers in the future that changed dynptr view or modified it in some way. Include test cases for all cases, i.e. both bpf_dynptr_from_mem and bpf_ringbuf_reserve_dynptr, and ensure verifier rejects both of them. Without the fix, both of these programs load and pass verification. While at it, remove sys_nanosleep target from failure cases' SEC definition, as there is no such tracepoint. Acked-by: David Vernet Acked-by: Joanne Koong Signed-off-by: Kumar Kartikeya Dwivedi --- .../selftests/bpf/prog_tests/user_ringbuf.c | 2 + .../selftests/bpf/progs/user_ringbuf_fail.c | 51 ++++++++++++++++--- 2 files changed, 45 insertions(+), 8 deletions(-) diff --git a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c index aefa0a474e58..dae68de285b9 100644 --- a/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c +++ b/tools/testing/selftests/bpf/prog_tests/user_ringbuf.c @@ -676,6 +676,8 @@ static struct { {"user_ringbuf_callback_discard_dynptr", "cannot release unowned const bpf_dynptr"}, {"user_ringbuf_callback_submit_dynptr", "cannot release unowned const bpf_dynptr"}, {"user_ringbuf_callback_invalid_return", "At callback return the register R0 has value"}, + {"user_ringbuf_callback_reinit_dynptr_mem", "Dynptr has to be an uninitialized dynptr"}, + {"user_ringbuf_callback_reinit_dynptr_ringbuf", "Dynptr has to be an uninitialized dynptr"}, }; #define SUCCESS_TEST(_func) { _func, #_func } diff --git a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c index 82aba4529aa9..f3201dc69a60 100644 --- a/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c +++ b/tools/testing/selftests/bpf/progs/user_ringbuf_fail.c @@ -18,6 +18,13 @@ struct { __uint(type, BPF_MAP_TYPE_USER_RINGBUF); } user_ringbuf SEC(".maps"); +struct { + __uint(type, BPF_MAP_TYPE_RINGBUF); + __uint(max_entries, 2); +} ringbuf SEC(".maps"); + +static int map_value; + static long bad_access1(struct bpf_dynptr *dynptr, void *context) { @@ -32,7 +39,7 @@ bad_access1(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to read before the pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_bad_access1(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, bad_access1, NULL, 0); @@ -54,7 +61,7 @@ bad_access2(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to read past the end of the pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_bad_access2(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, bad_access2, NULL, 0); @@ -73,7 +80,7 @@ write_forbidden(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to write to that pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_write_forbidden(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, write_forbidden, NULL, 0); @@ -92,7 +99,7 @@ null_context_write(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to write to that pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_null_context_write(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, null_context_write, NULL, 0); @@ -113,7 +120,7 @@ null_context_read(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to write to that pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_null_context_read(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, null_context_read, NULL, 0); @@ -132,7 +139,7 @@ try_discard_dynptr(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to read past the end of the pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_discard_dynptr(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, try_discard_dynptr, NULL, 0); @@ -151,7 +158,7 @@ try_submit_dynptr(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to read past the end of the pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_submit_dynptr(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, try_submit_dynptr, NULL, 0); @@ -168,10 +175,38 @@ invalid_drain_callback_return(struct bpf_dynptr *dynptr, void *context) /* A callback that accesses a dynptr in a bpf_user_ringbuf_drain callback should * not be able to write to that pointer. */ -SEC("?raw_tp/sys_nanosleep") +SEC("?raw_tp/") int user_ringbuf_callback_invalid_return(void *ctx) { bpf_user_ringbuf_drain(&user_ringbuf, invalid_drain_callback_return, NULL, 0); return 0; } + +static long +try_reinit_dynptr_mem(struct bpf_dynptr *dynptr, void *context) +{ + bpf_dynptr_from_mem(&map_value, 4, 0, dynptr); + return 0; +} + +static long +try_reinit_dynptr_ringbuf(struct bpf_dynptr *dynptr, void *context) +{ + bpf_ringbuf_reserve_dynptr(&ringbuf, 8, 0, dynptr); + return 0; +} + +SEC("?raw_tp/") +int user_ringbuf_callback_reinit_dynptr_mem(void *ctx) +{ + bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_mem, NULL, 0); + return 0; +} + +SEC("?raw_tp/") +int user_ringbuf_callback_reinit_dynptr_ringbuf(void *ctx) +{ + bpf_user_ringbuf_drain(&user_ringbuf, try_reinit_dynptr_ringbuf, NULL, 0); + return 0; +}