search

iovisor / ubpf

Userspace eBPF VM
Apache License 2.0
814 stars 132 forks source link

Emit ModRM Correctly When Register R12 Is Used #534

Closed hawkinsw closed 1 month ago

hawkinsw commented 1 month ago

When register R12 is used in an instruction requiring a ModRM byte, the semantics and encoding of the instruction are unique. The emit_modrm_and_displacement function did not handle this uniqueness properly. This patch modifies the function to handle the use of R12 correctly.

Fixes #529

hawkinsw commented 1 month ago

I believe that this patch will handle R12 usage properly. I have manually inspected the code emitted by the JIT and everything looks good. I have made a local test that will stress the bug and everything works. I will add special unit tests for this case tomorrow.

@dtcccc If you had any interest, I would love to know whether you can confirm that this resolves the issue you are seeing.

coveralls commented 1 month ago

Coverage Status

coverage: 80.316% (+0.01%) from 80.303% when pulling 3ea1a8a99cc29ed8adc8698126022b99b8a825c5 on hawkinsw:r12_fix into 19cd22c6fbe2878898717308d5792d49bff34d3e on iovisor:main.

dtcccc commented 1 month ago

Hmm, I still found my prog crashed. After a short dig:

    r8 = r1
    r0 = *(u64 *)(r8 + 0)
    exit

the llvm-objdump:

bpf.o:  file format elf64-bpf

Disassembly of section .text:

0000000000000000 <func>:
       0:   bf 18 00 00 00 00 00 00 r8 = r1
       1:   79 80 00 00 00 00 00 00 r0 = *(u64 *)(r8 + 0)
       2:   95 00 00 00 00 00 00 00 exit

and jited code:

   0x7fdcde481000:      push   %rbp
   0x7fdcde481001:      push   %rbx
   0x7fdcde481002:      push   %r12
   0x7fdcde481004:      push   %r13
   0x7fdcde481006:      push   %r14
   0x7fdcde481008:      push   %r15
   0x7fdcde48100a:      mov    %rdi,%r11
   0x7fdcde48100d:      sub    $0x8,%rsp
   0x7fdcde481014:      mov    %rsp,%rbp
   0x7fdcde481017:      mov    %rsp,%r15
   0x7fdcde48101a:      sub    $0x200,%rsp
   0x7fdcde481021:      callq  0x7fdcde48102b
   0x7fdcde481026:      jmpq   0x7fdcde481048
   0x7fdcde48102b:      sub    $0x8,%rsp
   0x7fdcde481032:      movq   $0x20,(%rsp)
   0x7fdcde48103a:      mov    %rdi,%r13
=> 0x7fdcde48103d:      mov    0x8c48148(%rip),%rax        # 0x7fdce70c918c
   0x7fdcde481044:      add    %al,(%rax)
   0x7fdcde481046:      add    %al,%bl
   0x7fdcde481048:      mov    %rbp,%rsp
   0x7fdcde48104b:      add    $0x8,%rsp
   0x7fdcde481052:      pop    %r15
   0x7fdcde481054:      pop    %r14
   0x7fdcde481056:      pop    %r13
   0x7fdcde481058:      pop    %r12
   0x7fdcde48105a:      pop    %rbx
   0x7fdcde48105b:      pop    %rbp
   0x7fdcde48105c:      retq   
   0x7fdcde48105d:      callq  0x7fdcde481069
   0x7fdcde481062:      pause  
   0x7fdcde481064:      jmpq   0x7fdcde481000
   0x7fdcde481069:      mov    %rax,(%rsp)
   0x7fdcde48106d:      retq

It seems this is time to r8....

dtcccc commented 1 month ago

I reverted this commit and r8 is ok. It seems r8 is the regression of this commit.

hawkinsw commented 1 month ago

Hmm, I still found my prog crashed. After a short dig:

  r8 = r1
  r0 = *(u64 *)(r8 + 0)
  exit

the llvm-objdump:

bpf.o:    file format elf64-bpf

Disassembly of section .text:

0000000000000000 <func>:
       0: bf 18 00 00 00 00 00 00 r8 = r1
       1: 79 80 00 00 00 00 00 00 r0 = *(u64 *)(r8 + 0)
       2: 95 00 00 00 00 00 00 00 exit

and jited code:

   0x7fdcde481000:      push   %rbp
   0x7fdcde481001:      push   %rbx
   0x7fdcde481002:      push   %r12
   0x7fdcde481004:      push   %r13
   0x7fdcde481006:      push   %r14
   0x7fdcde481008:      push   %r15
   0x7fdcde48100a:      mov    %rdi,%r11
   0x7fdcde48100d:      sub    $0x8,%rsp
   0x7fdcde481014:      mov    %rsp,%rbp
   0x7fdcde481017:      mov    %rsp,%r15
   0x7fdcde48101a:      sub    $0x200,%rsp
   0x7fdcde481021:      callq  0x7fdcde48102b
   0x7fdcde481026:      jmpq   0x7fdcde481048
   0x7fdcde48102b:      sub    $0x8,%rsp
   0x7fdcde481032:      movq   $0x20,(%rsp)
   0x7fdcde48103a:      mov    %rdi,%r13
=> 0x7fdcde48103d:      mov    0x8c48148(%rip),%rax        # 0x7fdce70c918c
   0x7fdcde481044:      add    %al,(%rax)
   0x7fdcde481046:      add    %al,%bl
   0x7fdcde481048:      mov    %rbp,%rsp
   0x7fdcde48104b:      add    $0x8,%rsp
   0x7fdcde481052:      pop    %r15
   0x7fdcde481054:      pop    %r14
   0x7fdcde481056:      pop    %r13
   0x7fdcde481058:      pop    %r12
   0x7fdcde48105a:      pop    %rbx
   0x7fdcde48105b:      pop    %rbp
   0x7fdcde48105c:      retq   
   0x7fdcde48105d:      callq  0x7fdcde481069
   0x7fdcde481062:      pause  
   0x7fdcde481064:      jmpq   0x7fdcde481000
   0x7fdcde481069:      mov    %rax,(%rsp)
   0x7fdcde48106d:      retq

It seems this is time to r8....

@dtcccc Thank you! I think that the issue here is slightly different. We have a special case for a 0 offset and I bet that is emitting incorrect code. I am checking now and then I will send out another PR.

Thank you for the feedback! Will

hawkinsw commented 1 month ago

@dtcccc I think that the latest version should resolve your issue! Give it a test, if you can!!

dtcccc commented 1 month ago

@dtcccc I think that the latest version should resolve your issue! Give it a test, if you can!!

It works for me now. Thanks for your work!

hawkinsw commented 1 month ago

@dtcccc I think that the latest version should resolve your issue! Give it a test, if you can!!

It works for me now. Thanks for your work!

Great news! I was waiting all day to hear a confirmation! Thank you!

dtcccc commented 1 month ago

Great news! I was waiting all day to hear a confirmation! Thank you!

I was sleeping at that time :-)

hawkinsw commented 1 month ago

Great news! I was waiting all day to hear a confirmation! Thank you!

I was sleeping at that time :-)

There's absolutely no excuse for you sleeping while I was waiting :-) Thank you, again, for reporting the issue and being a great user.