netdev - Re: [PATCH bpf] bpf: fix replace_map_fd_with_map

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAPhsuW6yiUu6WSkC5Xz+Lks+mq7Af-xtqVe-K7oO=bdPUJA8gA@mail.gmail.com>
Date:   Mon, 4 Mar 2019 15:01:24 -0800
From:   Song Liu <liu.song.a23@...il.com>
To:     Daniel Borkmann <daniel@...earbox.net>
Cc:     Alexei Starovoitov <ast@...nel.org>, bpf <bpf@...r.kernel.org>,
        Networking <netdev@...r.kernel.org>
Subject: Re: [PATCH bpf] bpf: fix replace_map_fd_with_map_ptr's ldimm64 second
 imm field

On Mon, Mar 4, 2019 at 12:09 PM Daniel Borkmann <daniel@...earbox.net> wrote:
>
> 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} occurrences
> 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@...earbox.net>

Acked-by: Song Liu <songliubraving@...com>

> ---
>  [ Needs to wait until bpf tree has everything fast-forwarded from
>    Linus' tree. ]
>
>  kernel/bpf/verifier.c                           | 10 +++++-----
>  tools/testing/selftests/bpf/verifier/ld_imm64.c | 15 ++++++++++++++-
>  2 files changed, 19 insertions(+), 6 deletions(-)
>
> diff --git a/kernel/bpf/verifier.c b/kernel/bpf/verifier.c
> index 0e4edd7..c8d2a94 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 28b8c80..3856dba 100644
> --- a/tools/testing/selftests/bpf/verifier/ld_imm64.c
> +++ b/tools/testing/selftests/bpf/verifier/ld_imm64.c
> @@ -122,7 +122,7 @@
>         .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(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.9.5
>