Message ID | 20200422173630.8351-1-luke.r.nels@gmail.com |
---|---|
State | Accepted |
Delegated to: | BPF Maintainers |
Headers | show |
Series | [bpf,v2,1/2] bpf, x86_32: Fix incorrect encoding in BPF_LDX zero-extension | expand |
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>
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>
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
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: