* [PATCH bpf v2 2/2] bpf, x86_32: Fix clobbering of dst for BPF_JSET
2020-04-22 17:36 [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension Luke Nelson
@ 2020-04-22 17:36 ` Luke Nelson
2020-04-23 4:10 ` Wang YanQing
2020-04-23 4:53 ` [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension Wang YanQing
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Luke Nelson @ 2020-04-22 17:36 UTC (permalink / raw)
To: bpf
Cc: Brian Gerst, Luke Nelson, Xi Wang, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Wang YanQing,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, x86,
H. Peter Anvin, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
John Fastabend, KP Singh, netdev, linux-kernel
The current JIT clobbers the destination register for BPF_JSET BPF_X
and BPF_K by using "and" and "or" instructions. This is fine when the
destination register is a temporary loaded from a register stored on
the stack but not otherwise.
This patch fixes the problem (for both BPF_K and BPF_X) by always loading
the destination register into temporaries since BPF_JSET should not
modify the destination register.
This bug may not be currently triggerable as BPF_REG_AX is the only
register not stored on the stack and the verifier uses it in a limited
way.
Fixes: 03f5781be2c7b ("bpf, x86_32: add eBPF JIT compiler for ia32")
Signed-off-by: Xi Wang <xi.wang@gmail.com>
Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
---
v1 -> v2: No changes.
---
arch/x86/net/bpf_jit_comp32.c | 22 ++++++++++++++++++----
1 file changed, 18 insertions(+), 4 deletions(-)
diff --git a/arch/x86/net/bpf_jit_comp32.c b/arch/x86/net/bpf_jit_comp32.c
index cc9ad3892ea6..ba7d9ccfc662 100644
--- a/arch/x86/net/bpf_jit_comp32.c
+++ b/arch/x86/net/bpf_jit_comp32.c
@@ -2015,8 +2015,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
case BPF_JMP | BPF_JSET | BPF_X:
case BPF_JMP32 | BPF_JSET | BPF_X: {
bool is_jmp64 = BPF_CLASS(insn->code) == BPF_JMP;
- u8 dreg_lo = dstk ? IA32_EAX : dst_lo;
- u8 dreg_hi = dstk ? IA32_EDX : dst_hi;
+ u8 dreg_lo = IA32_EAX;
+ u8 dreg_hi = IA32_EDX;
u8 sreg_lo = sstk ? IA32_ECX : src_lo;
u8 sreg_hi = sstk ? IA32_EBX : src_hi;
@@ -2028,6 +2028,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
add_2reg(0x40, IA32_EBP,
IA32_EDX),
STACK_VAR(dst_hi));
+ } else {
+ /* mov dreg_lo,dst_lo */
+ EMIT2(0x89, add_2reg(0xC0, dreg_lo, dst_lo));
+ if (is_jmp64)
+ /* mov dreg_hi,dst_hi */
+ EMIT2(0x89,
+ add_2reg(0xC0, dreg_hi, dst_hi));
}
if (sstk) {
@@ -2052,8 +2059,8 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
case BPF_JMP | BPF_JSET | BPF_K:
case BPF_JMP32 | BPF_JSET | BPF_K: {
bool is_jmp64 = BPF_CLASS(insn->code) == BPF_JMP;
- u8 dreg_lo = dstk ? IA32_EAX : dst_lo;
- u8 dreg_hi = dstk ? IA32_EDX : dst_hi;
+ u8 dreg_lo = IA32_EAX;
+ u8 dreg_hi = IA32_EDX;
u8 sreg_lo = IA32_ECX;
u8 sreg_hi = IA32_EBX;
u32 hi;
@@ -2066,6 +2073,13 @@ static int do_jit(struct bpf_prog *bpf_prog, int *addrs, u8 *image,
add_2reg(0x40, IA32_EBP,
IA32_EDX),
STACK_VAR(dst_hi));
+ } else {
+ /* mov dreg_lo,dst_lo */
+ EMIT2(0x89, add_2reg(0xC0, dreg_lo, dst_lo));
+ if (is_jmp64)
+ /* mov dreg_hi,dst_hi */
+ EMIT2(0x89,
+ add_2reg(0xC0, dreg_hi, dst_hi));
}
/* mov ecx,imm32 */
--
2.17.1
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH bpf v2 2/2] bpf, x86_32: Fix clobbering of dst for BPF_JSET
2020-04-22 17:36 ` [PATCH bpf v2 2/2] bpf, x86_32: Fix clobbering of dst for BPF_JSET Luke Nelson
@ 2020-04-23 4:10 ` Wang YanQing
0 siblings, 0 replies; 6+ messages in thread
From: Wang YanQing @ 2020-04-23 4:10 UTC (permalink / raw)
To: Luke Nelson
Cc: bpf, Brian Gerst, Luke Nelson, Xi Wang, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
linux-kernel
On Wed, Apr 22, 2020 at 10:36:30AM -0700, Luke Nelson wrote:
> The current JIT clobbers the destination register for BPF_JSET BPF_X
> and BPF_K by using "and" and "or" instructions. This is fine when the
> destination register is a temporary loaded from a register stored on
> the stack but not otherwise.
>
> This patch fixes the problem (for both BPF_K and BPF_X) by always loading
> the destination register into temporaries since BPF_JSET should not
> modify the destination register.
>
> This bug may not be currently triggerable as BPF_REG_AX is the only
> register not stored on the stack and the verifier uses it in a limited
> way.
>
> Fixes: 03f5781be2c7b ("bpf, x86_32: add eBPF JIT compiler for ia32")
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
Acked-by: Wang YanQing <udknight@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension
2020-04-22 17:36 [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension Luke Nelson
2020-04-22 17:36 ` [PATCH bpf v2 2/2] bpf, x86_32: Fix clobbering of dst for BPF_JSET Luke Nelson
@ 2020-04-23 4:53 ` Wang YanQing
2020-04-23 6:08 ` hpa
2020-04-25 0:15 ` Alexei Starovoitov
3 siblings, 0 replies; 6+ messages in thread
From: Wang YanQing @ 2020-04-23 4:53 UTC (permalink / raw)
To: Luke Nelson
Cc: bpf, Brian Gerst, Luke Nelson, Xi Wang, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, H. Peter Anvin,
Alexei Starovoitov, Daniel Borkmann, Martin KaFai Lau, Song Liu,
Yonghong Song, Andrii Nakryiko, John Fastabend, KP Singh, netdev,
linux-kernel
On Wed, Apr 22, 2020 at 10:36:29AM -0700, Luke Nelson wrote:
> The current JIT uses the following sequence to zero-extend into the
> upper 32 bits of the destination register for BPF_LDX BPF_{B,H,W},
> when the destination register is not on the stack:
>
> EMIT3(0xC7, add_1reg(0xC0, dst_hi), 0);
>
> The problem is that C7 /0 encodes a MOV instruction that requires a 4-byte
> immediate; the current code emits only 1 byte of the immediate. This
> means that the first 3 bytes of the next instruction will be treated as
> the rest of the immediate, breaking the stream of instructions.
>
> This patch fixes the problem by instead emitting "xor dst_hi,dst_hi"
> to clear the upper 32 bits. This fixes the problem and is more efficient
> than using MOV to load a zero immediate.
>
> This bug may not be currently triggerable as BPF_REG_AX is the only
> register not stored on the stack and the verifier uses it in a limited
> way, and the verifier implements a zero-extension optimization. But the
> JIT should avoid emitting incorrect encodings regardless.
>
> Fixes: 03f5781be2c7b ("bpf, x86_32: add eBPF JIT compiler for ia32")
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
Acked-by: Wang YanQing <udknight@gmail.com>
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension
2020-04-22 17:36 [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension Luke Nelson
2020-04-22 17:36 ` [PATCH bpf v2 2/2] bpf, x86_32: Fix clobbering of dst for BPF_JSET Luke Nelson
2020-04-23 4:53 ` [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension Wang YanQing
@ 2020-04-23 6:08 ` hpa
2020-04-25 0:15 ` Alexei Starovoitov
3 siblings, 0 replies; 6+ messages in thread
From: hpa @ 2020-04-23 6:08 UTC (permalink / raw)
To: Luke Nelson, bpf
Cc: Brian Gerst, Luke Nelson, Xi Wang, Wang YanQing, David S. Miller,
Alexey Kuznetsov, Hideaki YOSHIFUJI, Thomas Gleixner,
Ingo Molnar, Borislav Petkov, x86, Alexei Starovoitov,
Daniel Borkmann, Martin KaFai Lau, Song Liu, Yonghong Song,
Andrii Nakryiko, John Fastabend, KP Singh, netdev, linux-kernel
On April 22, 2020 10:36:29 AM PDT, Luke Nelson <lukenels@cs.washington.edu> wrote:
>The current JIT uses the following sequence to zero-extend into the
>upper 32 bits of the destination register for BPF_LDX BPF_{B,H,W},
>when the destination register is not on the stack:
>
> EMIT3(0xC7, add_1reg(0xC0, dst_hi), 0);
>
>The problem is that C7 /0 encodes a MOV instruction that requires a
>4-byte
>immediate; the current code emits only 1 byte of the immediate. This
>means that the first 3 bytes of the next instruction will be treated as
>the rest of the immediate, breaking the stream of instructions.
>
>This patch fixes the problem by instead emitting "xor dst_hi,dst_hi"
>to clear the upper 32 bits. This fixes the problem and is more
>efficient
>than using MOV to load a zero immediate.
>
>This bug may not be currently triggerable as BPF_REG_AX is the only
>register not stored on the stack and the verifier uses it in a limited
>way, and the verifier implements a zero-extension optimization. But the
>JIT should avoid emitting incorrect encodings regardless.
>
>Fixes: 03f5781be2c7b ("bpf, x86_32: add eBPF JIT compiler for ia32")
>Signed-off-by: Xi Wang <xi.wang@gmail.com>
>Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
>---
>v1 -> v2: Updated commit message to better reflect the bug.
> (H. Peter Anvin and Brian Gerst)
>---
> arch/x86/net/bpf_jit_comp32.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/arch/x86/net/bpf_jit_comp32.c
>b/arch/x86/net/bpf_jit_comp32.c
>index 4d2a7a764602..cc9ad3892ea6 100644
>--- a/arch/x86/net/bpf_jit_comp32.c
>+++ b/arch/x86/net/bpf_jit_comp32.c
>@@ -1854,7 +1854,9 @@ static int do_jit(struct bpf_prog *bpf_prog, int
>*addrs, u8 *image,
> STACK_VAR(dst_hi));
> EMIT(0x0, 4);
> } else {
>- EMIT3(0xC7, add_1reg(0xC0, dst_hi), 0);
>+ /* xor dst_hi,dst_hi */
>+ EMIT2(0x33,
>+ add_2reg(0xC0, dst_hi, dst_hi));
> }
> break;
> case BPF_DW:
Reviewed-by: H. Peter Anvin (Intel) <hpa@zytor.com>
--
Sent from my Android device with K-9 Mail. Please excuse my brevity.
^ permalink raw reply [flat|nested] 6+ messages in thread* Re: [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension
2020-04-22 17:36 [PATCH bpf v2 1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension Luke Nelson
` (2 preceding siblings ...)
2020-04-23 6:08 ` hpa
@ 2020-04-25 0:15 ` Alexei Starovoitov
3 siblings, 0 replies; 6+ messages in thread
From: Alexei Starovoitov @ 2020-04-25 0:15 UTC (permalink / raw)
To: Luke Nelson
Cc: bpf, Brian Gerst, Luke Nelson, Xi Wang, Wang YanQing,
David S. Miller, Alexey Kuznetsov, Hideaki YOSHIFUJI,
Thomas Gleixner, Ingo Molnar, Borislav Petkov, X86 ML,
H. Peter Anvin, Alexei Starovoitov, Daniel Borkmann,
Martin KaFai Lau, Song Liu, Yonghong Song, Andrii Nakryiko,
John Fastabend, KP Singh, Network Development, LKML
On Wed, Apr 22, 2020 at 10:36 AM Luke Nelson <lukenels@cs.washington.edu> wrote:
>
> The current JIT uses the following sequence to zero-extend into the
> upper 32 bits of the destination register for BPF_LDX BPF_{B,H,W},
> when the destination register is not on the stack:
>
> EMIT3(0xC7, add_1reg(0xC0, dst_hi), 0);
>
> The problem is that C7 /0 encodes a MOV instruction that requires a 4-byte
> immediate; the current code emits only 1 byte of the immediate. This
> means that the first 3 bytes of the next instruction will be treated as
> the rest of the immediate, breaking the stream of instructions.
>
> This patch fixes the problem by instead emitting "xor dst_hi,dst_hi"
> to clear the upper 32 bits. This fixes the problem and is more efficient
> than using MOV to load a zero immediate.
>
> This bug may not be currently triggerable as BPF_REG_AX is the only
> register not stored on the stack and the verifier uses it in a limited
> way, and the verifier implements a zero-extension optimization. But the
> JIT should avoid emitting incorrect encodings regardless.
>
> Fixes: 03f5781be2c7b ("bpf, x86_32: add eBPF JIT compiler for ia32")
> Signed-off-by: Xi Wang <xi.wang@gmail.com>
> Signed-off-by: Luke Nelson <luke.r.nels@gmail.com>
Applied. Thanks
^ permalink raw reply [flat|nested] 6+ messages in thread