Skip to contentSkip to conversation

bpf: fix bpf_tail_call() x64 JIT

PATCH In review Alexei Starovoitov 3 participants 3 messages Oct 3, 2017

Subsystems (6)
Networking Network drivers x86 BPF +2 more subsystems
lore
Share & tools
lore

Alexei Starovoitov

author
#0 lore raw

PATCH 1/1 bpf: fix bpf_tail_call() x64 JIT

- bpf prog_array just like all other types of bpf array accepts 32-bit index. Clarify that in the comment.
- fix x64 JIT of bpf_tail_call which was incorrectly loading 8 instead of 4 bytes
- tighten corresponding check in the interpreter to stay consistent

The JIT bug can be triggered after introduction of BPF_F_NUMA_NODE flag in commit 96eabe7a40aa in 4.14. Before that the map_flags would stay zero and though JIT code is wrong it will check bounds correctly.
Hence two fixes tags. All other JITs don't have this problem.

Signed-off-by Alexei Starovoitov <[email protected]>
Fixes 96eabe7a40aa ("bpf: Allow selecting numa node during map creation")
Fixes b52f00e6a715 ("x86: bpf_jit: implement bpf_tail_call() helper")
Acked-by Daniel Borkmann <[email protected]>
Acked-by Martin KaFai Lau <[email protected]>

---
Backport to stable would be nice, but not strictly necessary.

 arch/x86/net/bpf_jit_comp.c | 4 ++--
 include/uapi/linux/bpf.h    | 2 +-
 kernel/bpf/core.c           | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)
arch/x86/net/bpf_jit_comp.c +2 -2
index 8c9573660d51..0554e8aef4d5 100644
@@ -284,9 +284,9 @@ static void emit_bpf_tail_call(u8 **pprog)
 	/* if (index >= array->map.max_entries)
 	 *   goto out;
 	 */
-	EMIT4(0x48, 0x8B, 0x46,                   /* mov rax, qword ptr [rsi + 16] */
+	EMIT2(0x89, 0xD2);                        /* mov edx, edx */
+	EMIT3(0x39, 0x56,                         /* cmp dword ptr [rsi + 16], edx */
 	      offsetof(struct bpf_array, map.max_entries));
-	EMIT3(0x48, 0x39, 0xD0);                  /* cmp rax, rdx */
 #define OFFSET1 43 /* number of bytes to jump */
 	EMIT2(X86_JBE, OFFSET1);                  /* jbe out */
 	label1 = cnt;
include/uapi/linux/bpf.h +1 -1
index 43ab5c402f98..f90860d1f897 100644
@@ -312,7 +312,7 @@ union bpf_attr {
  *     jump into another BPF program
  *     @ctx: context pointer passed to next program
  *     @prog_array_map: pointer to map which type is BPF_MAP_TYPE_PROG_ARRAY
- *     @index: index inside array that selects specific program to run
+ *     @index: 32-bit index inside array that selects specific program to run
  *     Return: 0 on success or negative error
  *
  * int bpf_clone_redirect(skb, ifindex, flags)
kernel/bpf/core.c +1 -1
index 917cc04a0a94..7b62df86be1d 100644
@@ -1022,7 +1022,7 @@ static unsigned int ___bpf_prog_run(u64 *regs, const struct bpf_insn *insn,
 		struct bpf_map *map = (struct bpf_map *) (unsigned long) BPF_R2;
 		struct bpf_array *array = container_of(map, struct bpf_array, map);
 		struct bpf_prog *prog;
-		u64 index = BPF_R3;
+		u32 index = BPF_R3;
 
 		if (unlikely(index >= array->map.max_entries))
 			goto out;
Signature · 1 line
2.9.5
Reviewed-by on v1 1/1
#1 lore raw
Quoted 10 lines · Alexei Starovoitov

- bpf prog_array just like all other types of bpf array accepts 32-bit index. Clarify that in the comment.
- fix x64 JIT of bpf_tail_call which was incorrectly loading 8 instead of 4 bytes
- tighten corresponding check in the interpreter to stay consistent

The JIT bug can be triggered after introduction of BPF_F_NUMA_NODE flag in commit 96eabe7a40aa in 4.14. Before that the map_flags would stay zero and though JIT code is wrong it will check bounds correctly.
Hence two fixes tags. All other JITs don't have this problem.

Quoted 7 lines
Signed-off-by Alexei Starovoitov <[email protected]>
Fixes 96eabe7a40aa ("bpf: Allow selecting numa node during map creation")
Fixes b52f00e6a715 ("x86: bpf_jit: implement bpf_tail_call() helper")
Acked-by Daniel Borkmann <[email protected]>
Acked-by Martin KaFai Lau <[email protected]>

---
Backport to stable would be nice, but not strictly necessary.

Reviewed-by Eric Dumazet <[email protected]>

Thanks !

maintainer
#2 lore raw
In reply to Alexei Starovoitov

From: Alexei Starovoitov <[email protected]>
Date: Tue, 3 Oct 2017 15:37:20 -0700

Quoted 10 lines

- bpf prog_array just like all other types of bpf array accepts 32-bit index. Clarify that in the comment.
- fix x64 JIT of bpf_tail_call which was incorrectly loading 8 instead of 4 bytes
- tighten corresponding check in the interpreter to stay consistent

The JIT bug can be triggered after introduction of BPF_F_NUMA_NODE flag in commit 96eabe7a40aa in 4.14. Before that the map_flags would stay zero and though JIT code is wrong it will check bounds correctly.
Hence two fixes tags. All other JITs don't have this problem.

Quoted 7 lines
Signed-off-by Alexei Starovoitov <[email protected]>
Fixes 96eabe7a40aa ("bpf: Allow selecting numa node during map creation")
Fixes b52f00e6a715 ("x86: bpf_jit: implement bpf_tail_call() helper")
Acked-by Daniel Borkmann <[email protected]>
Acked-by Martin KaFai Lau <[email protected]>

---
Backport to stable would be nice, but not strictly necessary.

Applied and queued up for -stable, thanks.