On Mon, Mar 4, 2019 at 12:09 PM Daniel Borkmann <daniel@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
Acked-by: Song Liu <songliubraving@xxxxxx>
> ---
> [ 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
>
