Re: [PATCH bpf-next v2 1/7] bpf: implement lookup-free direct value access

[Andrii Nakryiko <andrii.nakryiko@gmail.com>]

On Mon, Mar 4, 2019 at 7:59 AM Daniel Borkmann <daniel@iogearbox.net> wrote:
>
> On 03/04/2019 07:03 AM, Andrii Nakryiko wrote:
> > On Thu, Feb 28, 2019 at 3:31 PM Daniel Borkmann <daniel@iogearbox.net> wrote:
> [...]
> >> @@ -6664,8 +6669,10 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env)
> >>                 }
> >>
> >>                 if (insn[0].code == (BPF_LD | BPF_IMM | BPF_DW)) {
> >> +                       struct bpf_insn_aux_data *aux;
> >>                         struct bpf_map *map;
> >>                         struct fd f;
> >> +                       u64 addr;
> >>
> >>                         if (i == insn_cnt - 1 || insn[1].code != 0 ||
> >>                             insn[1].dst_reg != 0 || insn[1].src_reg != 0 ||
> >
> > Next line after this one rejects ldimm64 instructions with off != 0.
> > This check needs to be changed, depending on whether src_reg ==
> > BPF_PSEUDO_MAP_VALUE, right?
>
> Yes, that's correct, I already have that changed in my local branch for
> supporting non-zero off.
>
> > This is also to the previously discussed question of not enforcing
> > offset (imm=0 in 2nd part of insn) for BPF_PSEUDO_MAP_FD. Seems like
> > verifier *does* enforce that (not that I'm advocating for re-using
> > BPF_PSEUDO_MAP_FD, just stumbled on this bit when going through
> > verifier code).
>
> Not really, lets test:

Ah, sorry, my bad. That code tests .off, not .imm, so yeah, any imm
would be accepted.

>
> [...]
>         .insns = {
>         BPF_MOV64_IMM(BPF_REG_0, 0),
>         BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, BPF_REG_1,
>                      BPF_PSEUDO_MAP_FD, 0, 0),
>         BPF_RAW_INSN(0, 0, 0, 0, 0xfefefe),
>         BPF_EXIT_INSN(),
>         },
> [...]
>
> #545/p test14 ld_imm64: reject 2nd imm != 0 FAIL
> Unexpected success to load!
> 0: (b7) r0 = 0
> 1: (18) r1 = 0xffff97e612486400
> 3: (95) exit
> processed 3 insns (limit 131072), stack depth 0
> Summary: 0 PASSED, 0 SKIPPED, 2 FAILED
>
> So I still think it would be worth doing something like the below
> for bpf:

Yep, lgtm.

>
> From 290f739ae6bab7b0709d327855a1812f9022beed Mon Sep 17 00:00:00 2001
> From: Daniel Borkmann <daniel@iogearbox.net>
> Date: Mon, 4 Mar 2019 14:22:41 +0000
> Subject: [PATCH bpf] bpf: fix replace_map_fd_with_map_ptr wrt ldimm64 wrt second imm field
>
> Non-zero imm value in the second part of the ldimm64 instruction for
> BPF_PSEUDO_MAP_FD is invalid, and thus must be rejected. The map fd
> only ever sits in the first instructions' imm field. None of the BPF
> loaders known to us are using it, so risk of regression is minimal.
> For clarity and consistency, the few insn->{src_reg,imm} occurences
> are rewritten into insn[0].{src_reg,imm}. Add a test case to the BPF
> selftest suite as well.
>
> Fixes: 0246e64d9a5f ("bpf: handle pseudo BPF_LD_IMM64 insn")
> Signed-off-by: Daniel Borkmann <daniel@iogearbox.net>
> ---
>  kernel/bpf/verifier.c                           | 10 +++++-----
>  tools/testing/selftests/bpf/verifier/ld_imm64.c | 17 +++++++++++++++--
>  2 files changed, 20 insertions(+), 7 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0e4edd7e3c5f..c8d2a948db37 100644
> --- a/kernel/bpf/verifier.c
> +++ b/kernel/bpf/verifier.c
> @@ -6678,17 +6678,17 @@ static int replace_map_fd_with_map_ptr(struct bpf_verifier_env *env)
>                                 /* valid generic load 64-bit imm */
>                                 goto next_insn;
>
> -                       if (insn->src_reg != BPF_PSEUDO_MAP_FD) {
> -                               verbose(env,
> -                                       "unrecognized bpf_ld_imm64 insn\n");
> +                       if (insn[0].src_reg != BPF_PSEUDO_MAP_FD ||
> +                           insn[1].imm != 0) {
> +                               verbose(env, "unrecognized bpf_ld_imm64 insn\n");
>                                 return -EINVAL;
>                         }
>
> -                       f = fdget(insn->imm);
> +                       f = fdget(insn[0].imm);
>                         map = __bpf_map_get(f);
>                         if (IS_ERR(map)) {
>                                 verbose(env, "fd %d is not pointing to valid bpf_map\n",
> -                                       insn->imm);
> +                                       insn[0].imm);
>                                 return PTR_ERR(map);
>                         }
>
> diff --git a/tools/testing/selftests/bpf/verifier/ld_imm64.c b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> index 28b8c805a293..4a1ff4560a8a 100644
> --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c
> +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> @@ -121,8 +121,8 @@
>         "test12 ld_imm64",
>         .insns = {
>         BPF_MOV64_IMM(BPF_REG_1, 0),
> -       BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 1),
> -       BPF_RAW_INSN(0, 0, 0, 0, 1),
> +       BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, 0, BPF_REG_1, 0, 999),
> +       BPF_RAW_INSN(0, 0, 0, 0, 0),
>         BPF_EXIT_INSN(),
>         },
>         .errstr = "not pointing to valid bpf_map",
> @@ -139,3 +139,16 @@
>         .errstr = "invalid bpf_ld_imm64 insn",
>         .result = REJECT,
>  },
> +{
> +       "test14 ld_imm64: reject 2nd imm != 0",
> +       .insns = {
> +       BPF_MOV64_IMM(BPF_REG_0, 0),
> +       BPF_RAW_INSN(BPF_LD | BPF_IMM | BPF_DW, BPF_REG_1,
> +                    BPF_PSEUDO_MAP_FD, 0, 0),
> +       BPF_RAW_INSN(0, 0, 0, 0, 0xfefefe),
> +       BPF_EXIT_INSN(),
> +       },
> +       .fixup_map_hash_48b = { 1 },
> +       .errstr = "unrecognized bpf_ld_imm64 insn",
> +       .result = REJECT,
> +},
> --
> 2.17.1
>