diff mbox series

[bpf-next,v3,1/3] bpf: Mark raw_tp arguments with PTR_MAYBE_NULL

Message ID 20241104171959.2938862-2-memxor@gmail.com (mailing list archive)
State Accepted
Commit cb4158ce8ec8a5bb528cc1693356a5eb8058094d
Delegated to: BPF
Headers show
Series Handle possible NULL trusted raw_tp arguments | expand

Checks

Context Check Description
bpf/vmtest-bpf-next-PR success PR summary
netdev/series_format success Posting correctly formatted
netdev/tree_selection success Clearly marked for bpf-next, async
netdev/ynl success Generated files up to date; no warnings/errors; no diff in generated;
netdev/fixes_present success Fixes tag not required for -next series
netdev/header_inline success No static functions without inline keyword in header files
netdev/build_32bit success Errors and warnings before: 204 this patch: 204
netdev/build_tools success Errors and warnings before: 0 (+0) this patch: 0 (+0)
netdev/cc_maintainers fail 1 blamed authors not CCed: void@manifault.com; 12 maintainers not CCed: mykolal@fb.com kpsingh@kernel.org martin.lau@linux.dev lulie@linux.alibaba.com shuah@kernel.org sdf@fomichev.me linux-kselftest@vger.kernel.org void@manifault.com song@kernel.org john.fastabend@gmail.com haoluo@google.com yonghong.song@linux.dev
netdev/build_clang success Errors and warnings before: 253 this patch: 253
netdev/verify_signedoff success Signed-off-by tag matches author and committer
netdev/deprecated_api success None detected
netdev/check_selftest success No net selftest shell script
netdev/verify_fixes success Fixes tag looks correct
netdev/build_allmodconfig_warn success Errors and warnings before: 6965 this patch: 6965
netdev/checkpatch fail ERROR: open brace '{' following function definitions go on the next line WARNING: line length of 86 exceeds 80 columns WARNING: line length of 90 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_clang_rust success No Rust files in patch. Skipping build
netdev/kdoc success Errors and warnings before: 6 this patch: 6
netdev/source_inline success Was 0 now: 0
bpf/vmtest-bpf-next-VM_Test-0 success Logs for Lint
bpf/vmtest-bpf-next-VM_Test-2 success Logs for Unittests
bpf/vmtest-bpf-next-VM_Test-5 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-9 success Logs for aarch64-gcc / test (test_verifier, false, 360) / test_verifier on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-10 success Logs for aarch64-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-12 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-33 success Logs for x86_64-llvm-17 / veristat
bpf/vmtest-bpf-next-VM_Test-19 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-8 success Logs for aarch64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-28 success Logs for x86_64-llvm-17 / build-release / build for x86_64 with llvm-17-O2
bpf/vmtest-bpf-next-VM_Test-17 success Logs for set-matrix
bpf/vmtest-bpf-next-VM_Test-27 success Logs for x86_64-llvm-17 / build / build for x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-7 success Logs for aarch64-gcc / test (test_progs, false, 360) / test_progs on aarch64 with gcc
bpf/vmtest-bpf-next-VM_Test-18 success Logs for x86_64-gcc / build / build for x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-16 success Logs for s390x-gcc / veristat
bpf/vmtest-bpf-next-VM_Test-11 success Logs for s390x-gcc / build / build for s390x with gcc
bpf/vmtest-bpf-next-VM_Test-34 success Logs for x86_64-llvm-18 / build / build for x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-35 success Logs for x86_64-llvm-18 / build-release / build for x86_64 with llvm-18-O2
bpf/vmtest-bpf-next-VM_Test-15 success Logs for s390x-gcc / test (test_verifier, false, 360) / test_verifier on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-13 success Logs for s390x-gcc / test (test_progs, false, 360) / test_progs on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-41 success Logs for x86_64-llvm-18 / veristat
bpf/vmtest-bpf-next-VM_Test-14 success Logs for s390x-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on s390x with gcc
bpf/vmtest-bpf-next-VM_Test-32 success Logs for x86_64-llvm-17 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-25 success Logs for x86_64-gcc / test (test_verifier, false, 360) / test_verifier on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-20 success Logs for x86_64-gcc / test (test_maps, false, 360) / test_maps on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-31 success Logs for x86_64-llvm-17 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-23 success Logs for x86_64-gcc / test (test_progs_no_alu32_parallel, true, 30) / test_progs_no_alu32_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-24 success Logs for x86_64-gcc / test (test_progs_parallel, true, 30) / test_progs_parallel on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-26 success Logs for x86_64-gcc / veristat / veristat on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-29 success Logs for x86_64-llvm-17 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-22 success Logs for x86_64-gcc / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-36 success Logs for x86_64-llvm-18 / test (test_maps, false, 360) / test_maps on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-21 success Logs for x86_64-gcc / test (test_progs, false, 360) / test_progs on x86_64 with gcc
bpf/vmtest-bpf-next-VM_Test-30 success Logs for x86_64-llvm-17 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-17
bpf/vmtest-bpf-next-VM_Test-37 success Logs for x86_64-llvm-18 / test (test_progs, false, 360) / test_progs on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-38 success Logs for x86_64-llvm-18 / test (test_progs_cpuv4, false, 360) / test_progs_cpuv4 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-39 success Logs for x86_64-llvm-18 / test (test_progs_no_alu32, false, 360) / test_progs_no_alu32 on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-40 success Logs for x86_64-llvm-18 / test (test_verifier, false, 360) / test_verifier on x86_64 with llvm-18
bpf/vmtest-bpf-next-VM_Test-3 success Logs for s390x-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-6 success Logs for x86_64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-1 success Logs for aarch64-gcc / build-release
bpf/vmtest-bpf-next-VM_Test-4 success Logs for set-matrix

Commit Message

Kumar Kartikeya Dwivedi Nov. 4, 2024, 5:19 p.m. UTC 
Arguments to a raw tracepoint are tagged as trusted, which carries the
semantics that the pointer will be non-NULL.  However, in certain cases,
a raw tracepoint argument may end up being NULL. More context about this
issue is available in [0].

Thus, there is a discrepancy between the reality, that raw_tp arguments
can actually be NULL, and the verifier's knowledge, that they are never
NULL, causing explicit NULL checks to be deleted, and accesses to such
pointers potentially crashing the kernel.

To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special
case the dereference and pointer arithmetic to permit it, and allow
passing them into helpers/kfuncs; these exceptions are made for raw_tp
programs only. Ensure that we don't do this when ref_obj_id > 0, as in
that case this is an acquired object and doesn't need such adjustment.

The reason we do mask_raw_tp_trusted_reg logic is because other will
recheck in places whether the register is a trusted_reg, and then
consider our register as untrusted when detecting the presence of the
PTR_MAYBE_NULL flag.

To allow safe dereference, we enable PROBE_MEM marking when we see loads
into trusted pointers with PTR_MAYBE_NULL.

While trusted raw_tp arguments can also be passed into helpers or kfuncs
where such broken assumption may cause issues, a future patch set will
tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can
already be passed into helpers and causes similar problems. Thus, they
are left alone for now.

It is possible that these checks also permit passing non-raw_tp args
that are trusted PTR_TO_BTF_ID with null marking. In such a case,
allowing dereference when pointer is NULL expands allowed behavior, so
won't regress existing programs, and the case of passing these into
helpers is the same as above and will be dealt with later.

Also update the failure case in tp_btf_nullable selftest to capture the
new behavior, as the verifier will no longer cause an error when
directly dereference a raw tracepoint argument marked as __nullable.

  [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb

Reviewed-by: Jiri Olsa <jolsa@kernel.org>
Reported-by: Juri Lelli <juri.lelli@redhat.com>
Tested-by: Juri Lelli <juri.lelli@redhat.com>
Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")
Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
---
 include/linux/bpf.h                           |  6 ++
 kernel/bpf/btf.c                              |  5 +-
 kernel/bpf/verifier.c                         | 79 +++++++++++++++++--
 .../bpf/progs/test_tp_btf_nullable.c          |  6 +-
 4 files changed, 87 insertions(+), 9 deletions(-)

Comments

Andrii Nakryiko Nov. 6, 2024, 10:32 p.m. UTC | #1 
On Mon, Nov 4, 2024 at 9:20 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> Arguments to a raw tracepoint are tagged as trusted, which carries the
> semantics that the pointer will be non-NULL.  However, in certain cases,
> a raw tracepoint argument may end up being NULL. More context about this
> issue is available in [0].
>
> Thus, there is a discrepancy between the reality, that raw_tp arguments
> can actually be NULL, and the verifier's knowledge, that they are never
> NULL, causing explicit NULL checks to be deleted, and accesses to such
> pointers potentially crashing the kernel.
>
> To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special
> case the dereference and pointer arithmetic to permit it, and allow
> passing them into helpers/kfuncs; these exceptions are made for raw_tp
> programs only. Ensure that we don't do this when ref_obj_id > 0, as in
> that case this is an acquired object and doesn't need such adjustment.
>
> The reason we do mask_raw_tp_trusted_reg logic is because other will
> recheck in places whether the register is a trusted_reg, and then
> consider our register as untrusted when detecting the presence of the
> PTR_MAYBE_NULL flag.
>
> To allow safe dereference, we enable PROBE_MEM marking when we see loads
> into trusted pointers with PTR_MAYBE_NULL.
>
> While trusted raw_tp arguments can also be passed into helpers or kfuncs
> where such broken assumption may cause issues, a future patch set will
> tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can
> already be passed into helpers and causes similar problems. Thus, they
> are left alone for now.
>
> It is possible that these checks also permit passing non-raw_tp args
> that are trusted PTR_TO_BTF_ID with null marking. In such a case,
> allowing dereference when pointer is NULL expands allowed behavior, so
> won't regress existing programs, and the case of passing these into
> helpers is the same as above and will be dealt with later.
>
> Also update the failure case in tp_btf_nullable selftest to capture the
> new behavior, as the verifier will no longer cause an error when
> directly dereference a raw tracepoint argument marked as __nullable.
>
>   [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb
>
> Reviewed-by: Jiri Olsa <jolsa@kernel.org>
> Reported-by: Juri Lelli <juri.lelli@redhat.com>
> Tested-by: Juri Lelli <juri.lelli@redhat.com>
> Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")
> Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> ---
>  include/linux/bpf.h                           |  6 ++
>  kernel/bpf/btf.c                              |  5 +-
>  kernel/bpf/verifier.c                         | 79 +++++++++++++++++--
>  .../bpf/progs/test_tp_btf_nullable.c          |  6 +-
>  4 files changed, 87 insertions(+), 9 deletions(-)
>

[...]

> @@ -12065,12 +12109,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>                         return -EINVAL;
>                 }
>
> +               mask = mask_raw_tp_reg(env, reg);
>                 if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) &&
>                     (register_is_null(reg) || type_may_be_null(reg->type)) &&
>                         !is_kfunc_arg_nullable(meta->btf, &args[i])) {
>                         verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i);
> +                       unmask_raw_tp_reg(reg, mask);

Kumar,

Do we really need this unmask? We are already erroring out, restoring
reg->type is probably not very important at this point?

>                         return -EACCES;
>                 }
> +               unmask_raw_tp_reg(reg, mask);
>
>                 if (reg->ref_obj_id) {
>                         if (is_kfunc_release(meta) && meta->ref_obj_id) {
> @@ -12128,16 +12175,24 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>                         if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
>                                 break;
>
> +                       /* Allow passing maybe NULL raw_tp arguments to
> +                        * kfuncs for compatibility. Don't apply this to
> +                        * arguments with ref_obj_id > 0.
> +                        */
> +                       mask = mask_raw_tp_reg(env, reg);
>                         if (!is_trusted_reg(reg)) {
>                                 if (!is_kfunc_rcu(meta)) {
>                                         verbose(env, "R%d must be referenced or trusted\n", regno);
> +                                       unmask_raw_tp_reg(reg, mask);

same as above, do we care about unmasking in this situation? and the
one immediately below?

>                                         return -EINVAL;
>                                 }
>                                 if (!is_rcu_reg(reg)) {
>                                         verbose(env, "R%d must be a rcu pointer\n", regno);
> +                                       unmask_raw_tp_reg(reg, mask);
>                                         return -EINVAL;
>                                 }
>                         }
> +                       unmask_raw_tp_reg(reg, mask);
>                         fallthrough;
>                 case KF_ARG_PTR_TO_CTX:
>                 case KF_ARG_PTR_TO_DYNPTR:
> @@ -12160,7 +12215,9 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>
>                 if (is_kfunc_release(meta) && reg->ref_obj_id)
>                         arg_type |= OBJ_RELEASE;
> +               mask = mask_raw_tp_reg(env, reg);
>                 ret = check_func_arg_reg_off(env, reg, regno, arg_type);
> +               unmask_raw_tp_reg(reg, mask);
>                 if (ret < 0)
>                         return ret;
>
> @@ -12337,6 +12394,7 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>                         ref_tname = btf_name_by_offset(btf, ref_t->name_off);
>                         fallthrough;
>                 case KF_ARG_PTR_TO_BTF_ID:
> +                       mask = mask_raw_tp_reg(env, reg);
>                         /* Only base_type is checked, further checks are done here */
>                         if ((base_type(reg->type) != PTR_TO_BTF_ID ||
>                              (bpf_type_has_unsafe_modifiers(reg->type) && !is_rcu_reg(reg))) &&
> @@ -12345,9 +12403,11 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
>                                 verbose(env, "expected %s or socket\n",
>                                         reg_type_str(env, base_type(reg->type) |
>                                                           (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS)));
> +                               unmask_raw_tp_reg(reg, mask);

ditto

>                                 return -EINVAL;
>                         }
>                         ret = process_kf_arg_ptr_to_btf_id(env, reg, ref_t, ref_tname, ref_id, meta, i);
> +                       unmask_raw_tp_reg(reg, mask);
>                         if (ret < 0)
>                                 return ret;
>                         break;
> @@ -13320,7 +13380,7 @@ static int sanitize_check_bounds(struct bpf_verifier_env *env,
>   */
>  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
>                                    struct bpf_insn *insn,
> -                                  const struct bpf_reg_state *ptr_reg,
> +                                  struct bpf_reg_state *ptr_reg,
>                                    const struct bpf_reg_state *off_reg)
>  {
>         struct bpf_verifier_state *vstate = env->cur_state;
> @@ -13334,6 +13394,7 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
>         struct bpf_sanitize_info info = {};
>         u8 opcode = BPF_OP(insn->code);
>         u32 dst = insn->dst_reg;
> +       bool mask;
>         int ret;
>
>         dst_reg = &regs[dst];
> @@ -13360,11 +13421,14 @@ static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
>                 return -EACCES;
>         }
>
> +       mask = mask_raw_tp_reg(env, ptr_reg);
>         if (ptr_reg->type & PTR_MAYBE_NULL) {
>                 verbose(env, "R%d pointer arithmetic on %s prohibited, null-check it first\n",
>                         dst, reg_type_str(env, ptr_reg->type));
> +               unmask_raw_tp_reg(ptr_reg, mask);

ditto

>                 return -EACCES;
>         }
> +       unmask_raw_tp_reg(ptr_reg, mask);
>
>         switch (base_type(ptr_reg->type)) {
>         case PTR_TO_CTX:
> @@ -19866,6 +19930,7 @@ static int convert_ctx_accesses(struct bpf_verifier_env *env)
>                  * for this case.
>                  */
>                 case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
> +               case PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL:
>                         if (type == BPF_READ) {
>                                 if (BPF_MODE(insn->code) == BPF_MEM)
>                                         insn->code = BPF_LDX | BPF_PROBE_MEM |
> diff --git a/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c b/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c
> index bba3e37f749b..5aaf2b065f86 100644
> --- a/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c
> +++ b/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c
> @@ -7,7 +7,11 @@
>  #include "bpf_misc.h"
>
>  SEC("tp_btf/bpf_testmod_test_nullable_bare")
> -__failure __msg("R1 invalid mem access 'trusted_ptr_or_null_'")
> +/* This used to be a failure test, but raw_tp nullable arguments can now
> + * directly be dereferenced, whether they have nullable annotation or not,
> + * and don't need to be explicitly checked.
> + */
> +__success
>  int BPF_PROG(handle_tp_btf_nullable_bare1, struct bpf_testmod_test_read_ctx *nullable_ctx)
>  {
>         return nullable_ctx->len;
> --
> 2.43.5
>
Kumar Kartikeya Dwivedi Nov. 6, 2024, 10:58 p.m. UTC | #2 
On Wed, 6 Nov 2024 at 16:32, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
>
> On Mon, Nov 4, 2024 at 9:20 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> >
> > Arguments to a raw tracepoint are tagged as trusted, which carries the
> > semantics that the pointer will be non-NULL.  However, in certain cases,
> > a raw tracepoint argument may end up being NULL. More context about this
> > issue is available in [0].
> >
> > Thus, there is a discrepancy between the reality, that raw_tp arguments
> > can actually be NULL, and the verifier's knowledge, that they are never
> > NULL, causing explicit NULL checks to be deleted, and accesses to such
> > pointers potentially crashing the kernel.
> >
> > To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special
> > case the dereference and pointer arithmetic to permit it, and allow
> > passing them into helpers/kfuncs; these exceptions are made for raw_tp
> > programs only. Ensure that we don't do this when ref_obj_id > 0, as in
> > that case this is an acquired object and doesn't need such adjustment.
> >
> > The reason we do mask_raw_tp_trusted_reg logic is because other will
> > recheck in places whether the register is a trusted_reg, and then
> > consider our register as untrusted when detecting the presence of the
> > PTR_MAYBE_NULL flag.
> >
> > To allow safe dereference, we enable PROBE_MEM marking when we see loads
> > into trusted pointers with PTR_MAYBE_NULL.
> >
> > While trusted raw_tp arguments can also be passed into helpers or kfuncs
> > where such broken assumption may cause issues, a future patch set will
> > tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can
> > already be passed into helpers and causes similar problems. Thus, they
> > are left alone for now.
> >
> > It is possible that these checks also permit passing non-raw_tp args
> > that are trusted PTR_TO_BTF_ID with null marking. In such a case,
> > allowing dereference when pointer is NULL expands allowed behavior, so
> > won't regress existing programs, and the case of passing these into
> > helpers is the same as above and will be dealt with later.
> >
> > Also update the failure case in tp_btf_nullable selftest to capture the
> > new behavior, as the verifier will no longer cause an error when
> > directly dereference a raw tracepoint argument marked as __nullable.
> >
> >   [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb
> >
> > Reviewed-by: Jiri Olsa <jolsa@kernel.org>
> > Reported-by: Juri Lelli <juri.lelli@redhat.com>
> > Tested-by: Juri Lelli <juri.lelli@redhat.com>
> > Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")
> > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > ---
> >  include/linux/bpf.h                           |  6 ++
> >  kernel/bpf/btf.c                              |  5 +-
> >  kernel/bpf/verifier.c                         | 79 +++++++++++++++++--
> >  .../bpf/progs/test_tp_btf_nullable.c          |  6 +-
> >  4 files changed, 87 insertions(+), 9 deletions(-)
> >
>
> [...]
>
> > @@ -12065,12 +12109,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> >                         return -EINVAL;
> >                 }
> >
> > +               mask = mask_raw_tp_reg(env, reg);
> >                 if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) &&
> >                     (register_is_null(reg) || type_may_be_null(reg->type)) &&
> >                         !is_kfunc_arg_nullable(meta->btf, &args[i])) {
> >                         verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i);
> > +                       unmask_raw_tp_reg(reg, mask);
>
> Kumar,
>
> Do we really need this unmask? We are already erroring out, restoring
> reg->type is probably not very important at this point?
>

Hello Andrii,
The reason I undid the masking was to ensure if the register type is
printed for some reason,
it stays consistent and the masking isn't visible, but I guess the
verifier state is printed _before_ an instruction is symbolically
executed so it's not helping with anything.

I can send a follow up to remove the additional unmasking steps.

> [...]
Alexei Starovoitov Nov. 8, 2024, 5:12 a.m. UTC | #3 
On Wed, Nov 6, 2024 at 2:58 PM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
>
> On Wed, 6 Nov 2024 at 16:32, Andrii Nakryiko <andrii.nakryiko@gmail.com> wrote:
> >
> > On Mon, Nov 4, 2024 at 9:20 AM Kumar Kartikeya Dwivedi <memxor@gmail.com> wrote:
> > >
> > > Arguments to a raw tracepoint are tagged as trusted, which carries the
> > > semantics that the pointer will be non-NULL.  However, in certain cases,
> > > a raw tracepoint argument may end up being NULL. More context about this
> > > issue is available in [0].
> > >
> > > Thus, there is a discrepancy between the reality, that raw_tp arguments
> > > can actually be NULL, and the verifier's knowledge, that they are never
> > > NULL, causing explicit NULL checks to be deleted, and accesses to such
> > > pointers potentially crashing the kernel.
> > >
> > > To fix this, mark raw_tp arguments as PTR_MAYBE_NULL, and then special
> > > case the dereference and pointer arithmetic to permit it, and allow
> > > passing them into helpers/kfuncs; these exceptions are made for raw_tp
> > > programs only. Ensure that we don't do this when ref_obj_id > 0, as in
> > > that case this is an acquired object and doesn't need such adjustment.
> > >
> > > The reason we do mask_raw_tp_trusted_reg logic is because other will
> > > recheck in places whether the register is a trusted_reg, and then
> > > consider our register as untrusted when detecting the presence of the
> > > PTR_MAYBE_NULL flag.
> > >
> > > To allow safe dereference, we enable PROBE_MEM marking when we see loads
> > > into trusted pointers with PTR_MAYBE_NULL.
> > >
> > > While trusted raw_tp arguments can also be passed into helpers or kfuncs
> > > where such broken assumption may cause issues, a future patch set will
> > > tackle their case separately, as PTR_TO_BTF_ID (without PTR_TRUSTED) can
> > > already be passed into helpers and causes similar problems. Thus, they
> > > are left alone for now.
> > >
> > > It is possible that these checks also permit passing non-raw_tp args
> > > that are trusted PTR_TO_BTF_ID with null marking. In such a case,
> > > allowing dereference when pointer is NULL expands allowed behavior, so
> > > won't regress existing programs, and the case of passing these into
> > > helpers is the same as above and will be dealt with later.
> > >
> > > Also update the failure case in tp_btf_nullable selftest to capture the
> > > new behavior, as the verifier will no longer cause an error when
> > > directly dereference a raw tracepoint argument marked as __nullable.
> > >
> > >   [0]: https://lore.kernel.org/bpf/ZrCZS6nisraEqehw@jlelli-thinkpadt14gen4.remote.csb
> > >
> > > Reviewed-by: Jiri Olsa <jolsa@kernel.org>
> > > Reported-by: Juri Lelli <juri.lelli@redhat.com>
> > > Tested-by: Juri Lelli <juri.lelli@redhat.com>
> > > Fixes: 3f00c5239344 ("bpf: Allow trusted pointers to be passed to KF_TRUSTED_ARGS kfuncs")
> > > Signed-off-by: Kumar Kartikeya Dwivedi <memxor@gmail.com>
> > > ---
> > >  include/linux/bpf.h                           |  6 ++
> > >  kernel/bpf/btf.c                              |  5 +-
> > >  kernel/bpf/verifier.c                         | 79 +++++++++++++++++--
> > >  .../bpf/progs/test_tp_btf_nullable.c          |  6 +-
> > >  4 files changed, 87 insertions(+), 9 deletions(-)
> > >
> >
> > [...]
> >
> > > @@ -12065,12 +12109,15 @@ static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
> > >                         return -EINVAL;
> > >                 }
> > >
> > > +               mask = mask_raw_tp_reg(env, reg);
> > >                 if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) &&
> > >                     (register_is_null(reg) || type_may_be_null(reg->type)) &&
> > >                         !is_kfunc_arg_nullable(meta->btf, &args[i])) {
> > >                         verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i);
> > > +                       unmask_raw_tp_reg(reg, mask);
> >
> > Kumar,
> >
> > Do we really need this unmask? We are already erroring out, restoring
> > reg->type is probably not very important at this point?
> >
>
> Hello Andrii,
> The reason I undid the masking was to ensure if the register type is
> printed for some reason,
> it stays consistent and the masking isn't visible, but I guess the
> verifier state is printed _before_ an instruction is symbolically
> executed so it's not helping with anything.
>
> I can send a follow up to remove the additional unmasking steps.

After sleeping on it I think I prefer to keep the code as-is.
Removing unmask() in few error path will make it asymmetrical
and harder to reason about.
Right now it's a straightforward hack mask and unmask.
Optional unmask just begs the question... why?
Maybe something will print regs in error path...
Maybe not now, but tomorrow.

So keep it as-is. No need for follow up.
diff mbox series

Patch

diff --git a/include/linux/bpf.h b/include/linux/bpf.h
index c3ba4d475174..1b84613b10ac 100644
--- a/include/linux/bpf.h
+++ b/include/linux/bpf.h
@@ -3495,4 +3495,10 @@  static inline bool bpf_is_subprog(const struct bpf_prog *prog)
 	return prog->aux->func_idx != 0;
 }
 
+static inline bool bpf_prog_is_raw_tp(const struct bpf_prog *prog)
+{
+	return prog->type == BPF_PROG_TYPE_TRACING &&
+	       prog->expected_attach_type == BPF_TRACE_RAW_TP;
+}
+
 #endif /* _LINUX_BPF_H */
diff --git a/kernel/bpf/btf.c b/kernel/bpf/btf.c
index ed3219da7181..e7a59e6462a9 100644
--- a/kernel/bpf/btf.c
+++ b/kernel/bpf/btf.c
@@ -6588,7 +6588,10 @@  bool btf_ctx_access(int off, int size, enum bpf_access_type type,
 	if (prog_args_trusted(prog))
 		info->reg_type |= PTR_TRUSTED;
 
-	if (btf_param_match_suffix(btf, &args[arg], "__nullable"))
+	/* Raw tracepoint arguments always get marked as maybe NULL */
+	if (bpf_prog_is_raw_tp(prog))
+		info->reg_type |= PTR_MAYBE_NULL;
+	else if (btf_param_match_suffix(btf, &args[arg], "__nullable"))
 		info->reg_type |= PTR_MAYBE_NULL;
 
 	if (tgt_prog) {
diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
index ba800c7611e3..7958d6ff6b73 100644
--- a/kernel/bpf/verifier.c
+++ b/kernel/bpf/verifier.c
@@ -418,6 +418,25 @@  static struct btf_record *reg_btf_record(const struct bpf_reg_state *reg)
 	return rec;
 }
 
+static bool mask_raw_tp_reg_cond(const struct bpf_verifier_env *env, struct bpf_reg_state *reg) {
+	return reg->type == (PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL) &&
+	       bpf_prog_is_raw_tp(env->prog) && !reg->ref_obj_id;
+}
+
+static bool mask_raw_tp_reg(const struct bpf_verifier_env *env, struct bpf_reg_state *reg)
+{
+	if (!mask_raw_tp_reg_cond(env, reg))
+		return false;
+	reg->type &= ~PTR_MAYBE_NULL;
+	return true;
+}
+
+static void unmask_raw_tp_reg(struct bpf_reg_state *reg, bool result)
+{
+	if (result)
+		reg->type |= PTR_MAYBE_NULL;
+}
+
 static bool subprog_is_global(const struct bpf_verifier_env *env, int subprog)
 {
 	struct bpf_func_info_aux *aux = env->prog->aux->func_info_aux;
@@ -6622,6 +6641,7 @@  static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 	const char *field_name = NULL;
 	enum bpf_type_flag flag = 0;
 	u32 btf_id = 0;
+	bool mask;
 	int ret;
 
 	if (!env->allow_ptr_leaks) {
@@ -6693,7 +6713,21 @@  static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 
 	if (ret < 0)
 		return ret;
-
+	/* For raw_tp progs, we allow dereference of PTR_MAYBE_NULL
+	 * trusted PTR_TO_BTF_ID, these are the ones that are possibly
+	 * arguments to the raw_tp. Since internal checks in for trusted
+	 * reg in check_ptr_to_btf_access would consider PTR_MAYBE_NULL
+	 * modifier as problematic, mask it out temporarily for the
+	 * check. Don't apply this to pointers with ref_obj_id > 0, as
+	 * those won't be raw_tp args.
+	 *
+	 * We may end up applying this relaxation to other trusted
+	 * PTR_TO_BTF_ID with maybe null flag, since we cannot
+	 * distinguish PTR_MAYBE_NULL tagged for arguments vs normal
+	 * tagging, but that should expand allowed behavior, and not
+	 * cause regression for existing behavior.
+	 */
+	mask = mask_raw_tp_reg(env, reg);
 	if (ret != PTR_TO_BTF_ID) {
 		/* just mark; */
 
@@ -6754,8 +6788,13 @@  static int check_ptr_to_btf_access(struct bpf_verifier_env *env,
 		clear_trusted_flags(&flag);
 	}
 
-	if (atype == BPF_READ && value_regno >= 0)
+	if (atype == BPF_READ && value_regno >= 0) {
 		mark_btf_ld_reg(env, regs, value_regno, ret, reg->btf, btf_id, flag);
+		/* We've assigned a new type to regno, so don't undo masking. */
+		if (regno == value_regno)
+			mask = false;
+	}
+	unmask_raw_tp_reg(reg, mask);
 
 	return 0;
 }
@@ -7140,7 +7179,7 @@  static int check_mem_access(struct bpf_verifier_env *env, int insn_idx, u32 regn
 		if (!err && t == BPF_READ && value_regno >= 0)
 			mark_reg_unknown(env, regs, value_regno);
 	} else if (base_type(reg->type) == PTR_TO_BTF_ID &&
-		   !type_may_be_null(reg->type)) {
+		   (mask_raw_tp_reg_cond(env, reg) || !type_may_be_null(reg->type))) {
 		err = check_ptr_to_btf_access(env, regs, regno, off, size, t,
 					      value_regno);
 	} else if (reg->type == CONST_PTR_TO_MAP) {
@@ -8833,6 +8872,7 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	enum bpf_reg_type type = reg->type;
 	u32 *arg_btf_id = NULL;
 	int err = 0;
+	bool mask;
 
 	if (arg_type == ARG_DONTCARE)
 		return 0;
@@ -8873,11 +8913,11 @@  static int check_func_arg(struct bpf_verifier_env *env, u32 arg,
 	    base_type(arg_type) == ARG_PTR_TO_SPIN_LOCK)
 		arg_btf_id = fn->arg_btf_id[arg];
 
+	mask = mask_raw_tp_reg(env, reg);
 	err = check_reg_type(env, regno, arg_type, arg_btf_id, meta);
-	if (err)
-		return err;
 
-	err = check_func_arg_reg_off(env, reg, regno, arg_type);
+	err = err ?: check_func_arg_reg_off(env, reg, regno, arg_type);
+	unmask_raw_tp_reg(reg, mask);
 	if (err)
 		return err;
 
@@ -9672,14 +9712,17 @@  static int btf_check_func_arg_match(struct bpf_verifier_env *env, int subprog,
 				return ret;
 		} else if (base_type(arg->arg_type) == ARG_PTR_TO_BTF_ID) {
 			struct bpf_call_arg_meta meta;
+			bool mask;
 			int err;
 
 			if (register_is_null(reg) && type_may_be_null(arg->arg_type))
 				continue;
 
 			memset(&meta, 0, sizeof(meta)); /* leave func_id as zero */
+			mask = mask_raw_tp_reg(env, reg);
 			err = check_reg_type(env, regno, arg->arg_type, &arg->btf_id, &meta);
 			err = err ?: check_func_arg_reg_off(env, reg, regno, arg->arg_type);
+			unmask_raw_tp_reg(reg, mask);
 			if (err)
 				return err;
 		} else {
@@ -12007,6 +12050,7 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 		enum bpf_arg_type arg_type = ARG_DONTCARE;
 		u32 regno = i + 1, ref_id, type_size;
 		bool is_ret_buf_sz = false;
+		bool mask = false;
 		int kf_arg_type;
 
 		t = btf_type_skip_modifiers(btf, args[i].type, NULL);
@@ -12065,12 +12109,15 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 			return -EINVAL;
 		}
 
+		mask = mask_raw_tp_reg(env, reg);
 		if ((is_kfunc_trusted_args(meta) || is_kfunc_rcu(meta)) &&
 		    (register_is_null(reg) || type_may_be_null(reg->type)) &&
 			!is_kfunc_arg_nullable(meta->btf, &args[i])) {
 			verbose(env, "Possibly NULL pointer passed to trusted arg%d\n", i);
+			unmask_raw_tp_reg(reg, mask);
 			return -EACCES;
 		}
+		unmask_raw_tp_reg(reg, mask);
 
 		if (reg->ref_obj_id) {
 			if (is_kfunc_release(meta) && meta->ref_obj_id) {
@@ -12128,16 +12175,24 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 			if (!is_kfunc_trusted_args(meta) && !is_kfunc_rcu(meta))
 				break;
 
+			/* Allow passing maybe NULL raw_tp arguments to
+			 * kfuncs for compatibility. Don't apply this to
+			 * arguments with ref_obj_id > 0.
+			 */
+			mask = mask_raw_tp_reg(env, reg);
 			if (!is_trusted_reg(reg)) {
 				if (!is_kfunc_rcu(meta)) {
 					verbose(env, "R%d must be referenced or trusted\n", regno);
+					unmask_raw_tp_reg(reg, mask);
 					return -EINVAL;
 				}
 				if (!is_rcu_reg(reg)) {
 					verbose(env, "R%d must be a rcu pointer\n", regno);
+					unmask_raw_tp_reg(reg, mask);
 					return -EINVAL;
 				}
 			}
+			unmask_raw_tp_reg(reg, mask);
 			fallthrough;
 		case KF_ARG_PTR_TO_CTX:
 		case KF_ARG_PTR_TO_DYNPTR:
@@ -12160,7 +12215,9 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 
 		if (is_kfunc_release(meta) && reg->ref_obj_id)
 			arg_type |= OBJ_RELEASE;
+		mask = mask_raw_tp_reg(env, reg);
 		ret = check_func_arg_reg_off(env, reg, regno, arg_type);
+		unmask_raw_tp_reg(reg, mask);
 		if (ret < 0)
 			return ret;
 
@@ -12337,6 +12394,7 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 			ref_tname = btf_name_by_offset(btf, ref_t->name_off);
 			fallthrough;
 		case KF_ARG_PTR_TO_BTF_ID:
+			mask = mask_raw_tp_reg(env, reg);
 			/* Only base_type is checked, further checks are done here */
 			if ((base_type(reg->type) != PTR_TO_BTF_ID ||
 			     (bpf_type_has_unsafe_modifiers(reg->type) && !is_rcu_reg(reg))) &&
@@ -12345,9 +12403,11 @@  static int check_kfunc_args(struct bpf_verifier_env *env, struct bpf_kfunc_call_
 				verbose(env, "expected %s or socket\n",
 					reg_type_str(env, base_type(reg->type) |
 							  (type_flag(reg->type) & BPF_REG_TRUSTED_MODIFIERS)));
+				unmask_raw_tp_reg(reg, mask);
 				return -EINVAL;
 			}
 			ret = process_kf_arg_ptr_to_btf_id(env, reg, ref_t, ref_tname, ref_id, meta, i);
+			unmask_raw_tp_reg(reg, mask);
 			if (ret < 0)
 				return ret;
 			break;
@@ -13320,7 +13380,7 @@  static int sanitize_check_bounds(struct bpf_verifier_env *env,
  */
 static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 				   struct bpf_insn *insn,
-				   const struct bpf_reg_state *ptr_reg,
+				   struct bpf_reg_state *ptr_reg,
 				   const struct bpf_reg_state *off_reg)
 {
 	struct bpf_verifier_state *vstate = env->cur_state;
@@ -13334,6 +13394,7 @@  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 	struct bpf_sanitize_info info = {};
 	u8 opcode = BPF_OP(insn->code);
 	u32 dst = insn->dst_reg;
+	bool mask;
 	int ret;
 
 	dst_reg = &regs[dst];
@@ -13360,11 +13421,14 @@  static int adjust_ptr_min_max_vals(struct bpf_verifier_env *env,
 		return -EACCES;
 	}
 
+	mask = mask_raw_tp_reg(env, ptr_reg);
 	if (ptr_reg->type & PTR_MAYBE_NULL) {
 		verbose(env, "R%d pointer arithmetic on %s prohibited, null-check it first\n",
 			dst, reg_type_str(env, ptr_reg->type));
+		unmask_raw_tp_reg(ptr_reg, mask);
 		return -EACCES;
 	}
+	unmask_raw_tp_reg(ptr_reg, mask);
 
 	switch (base_type(ptr_reg->type)) {
 	case PTR_TO_CTX:
@@ -19866,6 +19930,7 @@  static int convert_ctx_accesses(struct bpf_verifier_env *env)
 		 * for this case.
 		 */
 		case PTR_TO_BTF_ID | MEM_ALLOC | PTR_UNTRUSTED:
+		case PTR_TO_BTF_ID | PTR_TRUSTED | PTR_MAYBE_NULL:
 			if (type == BPF_READ) {
 				if (BPF_MODE(insn->code) == BPF_MEM)
 					insn->code = BPF_LDX | BPF_PROBE_MEM |
diff --git a/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c b/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c
index bba3e37f749b..5aaf2b065f86 100644
--- a/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c
+++ b/tools/testing/selftests/bpf/progs/test_tp_btf_nullable.c
@@ -7,7 +7,11 @@ 
 #include "bpf_misc.h"
 
 SEC("tp_btf/bpf_testmod_test_nullable_bare")
-__failure __msg("R1 invalid mem access 'trusted_ptr_or_null_'")
+/* This used to be a failure test, but raw_tp nullable arguments can now
+ * directly be dereferenced, whether they have nullable annotation or not,
+ * and don't need to be explicitly checked.
+ */
+__success
 int BPF_PROG(handle_tp_btf_nullable_bare1, struct bpf_testmod_test_read_ctx *nullable_ctx)
 {
 	return nullable_ctx->len;