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

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: <3311ac2f-2947-62d7-2be8-dcb4b23c6b5b@iogearbox.net>
Date:   Mon, 4 Mar 2019 16:59:17 +0100
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Andrii Nakryiko <andrii.nakryiko@...il.com>
Cc:     Alexei Starovoitov <ast@...com>, bpf@...r.kernel.org,
        Networking <netdev@...r.kernel.org>,
        Joe Stringer <joe@...d.net.nz>,
        john fastabend <john.fastabend@...il.com>, tgraf@...g.ch,
        Yonghong Song <yhs@...com>, Andrii Nakryiko <andriin@...com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        lmb@...udflare.com
Subject: Re: [PATCH bpf-next v2 1/7] bpf: implement lookup-free direct value
 access

On 03/04/2019 07:03 AM, Andrii Nakryiko wrote:
> On Thu, Feb 28, 2019 at 3:31 PM Daniel Borkmann <daniel@...earbox.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:

[...]
        .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:

>From 290f739ae6bab7b0709d327855a1812f9022beed Mon Sep 17 00:00:00 2001
From: Daniel Borkmann <daniel@...earbox.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@...earbox.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