* [PATCH bpf 2/2] bpf, selftests: Add test for BPF_STX BPF_B storing R10
2020-04-18 23:26 [PATCH bpf 1/2] bpf, x86: Fix encoding for lower 8-bit registers in BPF_STX BPF_B Luke Nelson
@ 2020-04-18 23:26 ` Luke Nelson
2020-04-21 2:46 ` [PATCH bpf 1/2] bpf, x86: Fix encoding for lower 8-bit registers in BPF_STX BPF_B Alexei Starovoitov
1 sibling, 0 replies; 3+ messages in thread
From: Luke Nelson @ 2020-04-18 23:26 UTC (permalink / raw)
To: bpf
Cc: Luke Nelson, Xi Wang, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Alexei Starovoitov, Daniel Borkmann,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
x86, Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
John Fastabend, KP Singh, Shuah Khan, netdev, linux-kernel,
linux-kselftest
This patch adds a test to test_verifier that writes the lower 8 bits of
R10 (aka FP) using BPF_B to an array map and reads the result back. The
expected behavior is that the result should be the same as first copying
R10 to R9, and then storing / loading the lower 8 bits of R9.
This test catches a bug that was present in the x86-64 JIT that caused
an incorrect encoding for BPF_STX BPF_B when the source operand is R10.
Signed-off-by: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
---
.../selftests/bpf/verifier/stack_ptr.c | 40 +++++++++++++++++++
1 file changed, 40 insertions(+)
diff --git a/tools/testing/selftests/bpf/verifier/stack_ptr.c b/tools/testing/selftests/bpf/verifier/stack_ptr.c
index 7276620ef242..8bfeb77c60bd 100644
--- a/tools/testing/selftests/bpf/verifier/stack_ptr.c
+++ b/tools/testing/selftests/bpf/verifier/stack_ptr.c
@@ -315,3 +315,43 @@
},
.result = ACCEPT,
},
+{
+ "store PTR_TO_STACK in R10 to array map using BPF_B",
+ .insns = {
+ /* Load pointer to map. */
+ BPF_MOV64_REG(BPF_REG_2, BPF_REG_10),
+ BPF_ALU64_IMM(BPF_ADD, BPF_REG_2, -8),
+ BPF_ST_MEM(BPF_DW, BPF_REG_2, 0, 0),
+ BPF_LD_MAP_FD(BPF_REG_1, 0),
+ BPF_EMIT_CALL(BPF_FUNC_map_lookup_elem),
+ BPF_JMP_IMM(BPF_JNE, BPF_REG_0, 0, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 2),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_REG(BPF_REG_1, BPF_REG_0),
+ /* Copy R10 to R9. */
+ BPF_MOV64_REG(BPF_REG_9, BPF_REG_10),
+ /* Pollute other registers with unaligned values. */
+ BPF_MOV64_IMM(BPF_REG_2, -1),
+ BPF_MOV64_IMM(BPF_REG_3, -1),
+ BPF_MOV64_IMM(BPF_REG_4, -1),
+ BPF_MOV64_IMM(BPF_REG_5, -1),
+ BPF_MOV64_IMM(BPF_REG_6, -1),
+ BPF_MOV64_IMM(BPF_REG_7, -1),
+ BPF_MOV64_IMM(BPF_REG_8, -1),
+ /* Store both R9 and R10 with BPF_B and read back. */
+ BPF_STX_MEM(BPF_B, BPF_REG_1, BPF_REG_10, 0),
+ BPF_LDX_MEM(BPF_B, BPF_REG_2, BPF_REG_1, 0),
+ BPF_STX_MEM(BPF_B, BPF_REG_1, BPF_REG_9, 0),
+ BPF_LDX_MEM(BPF_B, BPF_REG_3, BPF_REG_1, 0),
+ /* Should read back as same value. */
+ BPF_JMP_REG(BPF_JEQ, BPF_REG_2, BPF_REG_3, 2),
+ BPF_MOV64_IMM(BPF_REG_0, 1),
+ BPF_EXIT_INSN(),
+ BPF_MOV64_IMM(BPF_REG_0, 42),
+ BPF_EXIT_INSN(),
+ },
+ .fixup_map_array_48b = { 3 },
+ .result = ACCEPT,
+ .retval = 42,
+ .prog_type = BPF_PROG_TYPE_SCHED_CLS,
+},
--
2.17.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH bpf 1/2] bpf, x86: Fix encoding for lower 8-bit registers in BPF_STX BPF_B
2020-04-18 23:26 [PATCH bpf 1/2] bpf, x86: Fix encoding for lower 8-bit registers in BPF_STX BPF_B Luke Nelson
2020-04-18 23:26 ` [PATCH bpf 2/2] bpf, selftests: Add test for BPF_STX BPF_B storing R10 Luke Nelson
@ 2020-04-21 2:46 ` Alexei Starovoitov
1 sibling, 0 replies; 3+ messages in thread
From: Alexei Starovoitov @ 2020-04-21 2:46 UTC (permalink / raw)
To: Luke Nelson
Cc: bpf, Luke Nelson, Xi Wang, David S. Miller, Alexey Kuznetsov,
Hideaki YOSHIFUJI, Alexei Starovoitov, Daniel Borkmann,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin,
X86 ML, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, Shuah Khan,
Network Development, LKML, open list:KERNEL SELFTEST FRAMEWORK
On Sat, Apr 18, 2020 at 4:27 PM Luke Nelson <lukenels@cs.washington.edu> wrote:
>
> This patch fixes an encoding bug in emit_stx for BPF_B when the source
> register is BPF_REG_FP.
>
> The current implementation for BPF_STX BPF_B in emit_stx saves one REX
> byte when the operands can be encoded using Mod-R/M alone. The lower 8
> bits of registers %rax, %rbx, %rcx, and %rdx can be accessed without using
> a REX prefix via %al, %bl, %cl, and %dl, respectively. Other registers,
> (e.g., %rsi, %rdi, %rbp, %rsp) require a REX prefix to use their 8-bit
> equivalents (%sil, %dil, %bpl, %spl).
>
> The current code checks if the source for BPF_STX BPF_B is BPF_REG_1
> or BPF_REG_2 (which map to %rdi and %rsi), in which case it emits the
> required REX prefix. However, it misses the case when the source is
> BPF_REG_FP (mapped to %rbp).
>
> The result is that BPF_STX BPF_B with BPF_REG_FP as the source operand
> will read from register %ch instead of the correct %bpl. This patch fixes
> the problem by fixing and refactoring the check on which registers need
> the extra REX byte. Since no BPF registers map to %rsp, there is no need
> to handle %spl.
>
> Fixes: 622582786c9e0 ("net: filter: x86: internal BPF JIT")
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
Applied. Thanks for the fix.
It's questionable whether the verifier should have allowed such insn
in the first place, but JIT fix is good regardless.
^ permalink raw reply [flat|nested] 3+ messages in thread