Implementation of Binary::extend may have problem.

[Clcanny]

Describe the bug

In brief, I think the following statements have problem:

// src/ELF/Binary.cpp
Section& Binary::extend(const Section& section, uint64_t size) {
  // ...
  uint64_t from_address = section_to_extend->virtual_address() + size;
  this->datahandler_->make_hole(
    	section_to_extend->offset() + section_to_extend->size(),
    	size);
  this->shift_symbols(from_address, shift);
}

They should be:

// src/ELF/Binary.cpp
Section& Binary::extend(const Section& section, uint64_t size) {
  // ...
  uint64_t from_address = section_to_extend->virtual_address() + section_to_extend->virtual_size();
  this->datahandler_->make_hole(
    	section_to_extend->offset() + section_to_extend->size(),
    	size);
  this->shift_symbols(from_address, shift);
}

Source file

// main.cpp
int main() {}

// extend.cpp
// g++ -std=c++11 -O0 -ggdb extend.cpp -lLIEF -o extend
#include <LIEF/ELF.hpp>
#include <memory>
int main() {
    std::unique_ptr<LIEF::ELF::Binary> binary{LIEF::ELF::Parser::parse("main")};
    binary->extend(binary->get_section(".text"), 64);
    binary->write("main-extended");
}

Take a look at .text section

# objdump -d -j .text main | grep ">:" -A1 | grep -v "\-\-"
0000000000000530 <_start>:
 530:	31 ed                	xor    %ebp,%ebp
0000000000000560 <deregister_tm_clones>:
 560:	48 8d 3d c1 0a 20 00 	lea    0x200ac1(%rip),%rdi        # 201028 <__TMC_END__>
00000000000005a0 <register_tm_clones>:
 5a0:	48 8d 3d 81 0a 20 00 	lea    0x200a81(%rip),%rdi        # 201028 <__TMC_END__>
00000000000005f0 <__do_global_dtors_aux>:
 5f0:	80 3d 31 0a 20 00 00 	cmpb   $0x0,0x200a31(%rip)        # 201028 <__TMC_END__>
0000000000000630 <frame_dummy>:
 630:	48 8d 3d e1 07 20 00 	lea    0x2007e1(%rip),%rdi        # 200e18 <__JCR_END__>
0000000000000660 <main>:
 660:	55                   	push   %rbp
0000000000000670 <__libc_csu_init>:
 670:	41 57                	push   %r15
00000000000006e0 <__libc_csu_fini>:
 6e0:	f3 c3                	repz retq

objdump -d -j .text main-extended | grep ">:" -A1 | grep -v "\-\-"
0000000000000530 <_start>:
 530:	31 ed                	xor    %ebp,%ebp
0000000000000560 <deregister_tm_clones>:
 560:	48 8d 3d c1 0a 20 00 	lea    0x200ac1(%rip),%rdi        # 201028 <_Jv_RegisterClasses>
00000000000005e0 <register_tm_clones>:
 5e0:	5d                   	pop    %rbp
0000000000000630 <__do_global_dtors_aux>:
 630:	48 8d 3d e1 07 20 00 	lea    0x2007e1(%rip),%rdi        # 200e18 <__FRAME_END__+0x2005a0>
0000000000000670 <frame_dummy>:
 670:	41 57                	push   %r15
00000000000006a0 <main>:
 6a0:	ff 48 85             	decl   -0x7b(%rax)
00000000000006b0 <__libc_csu_init>:
 6b0:	4c 89 ea             	mov    %r13,%rdx
0000000000000720 <__libc_csu_fini>:

After extending .text section, functions in .text section are wrong. Actually this is cause by wrong records in .symtab section.

Take register_tm_clones and __do_global_dtors_aux as examples:

# objdump -d -j .text main-extended | grep "5a0"
 5a0:	48 8d 3d 81 0a 20 00 	lea    0x200a81(%rip),%rdi        # 201028 <_Jv_RegisterClasses>
# objdump -d -j .text main-extended | grep "5f0"
0000000000000630 <__do_global_dtors_aux>:
 5f0:	80 3d 31 0a 20 00 00 	cmpb   $0x0,0x200a31(%rip)        # 201028 <_Jv_RegisterClasses>

• Start address of register_tm_clones in main-extended isn't 0x5e0, it is 0x5a0 (just as start address of register_tm_clones in main).
• Start address of __do_global_dtors_aux in main-extended isn't 0x630, it is 0x5f0 (just as start address of __do_global_dtors_aux in main).

Root cause

# readelf --symbols main | grep register_tm_clones
    35: 00000000000005a0     0 FUNC    LOCAL  DEFAULT   13 register_tm_clones
# readelf --symbols main-extended | grep register_tm_clones
    35: 00000000000005e0     0 FUNC    LOCAL  DEFAULT   13 register_tm_clones

# readelf --symbols main | grep __do_global_dtors_aux
    36: 00000000000005f0     0 FUNC    LOCAL  DEFAULT   13 __do_global_dtors_aux
# readelf --symbols main-extended | grep __do_global_dtors_aux
    36: 0000000000000630     0 FUNC    LOCAL  DEFAULT   13 __do_global_dtors_aux

I think the root cause is the records in .symtab is wrong.

// src/ELF/Binary.cpp
Section& Binary::extend(const Section& section, uint64_t size) {
  // ...
  uint64_t from_address = section_to_extend->virtual_address() + size;
  this->datahandler_->make_hole(
    	section_to_extend->offset() + section_to_extend->size(),
    	size);
  this->shift_symbols(from_address, shift);
}

And I compare Binary::extend with Binary::extend_segment:

// src/ELF/Binary.tcc
uint64_t from_address = segment_to_extend->virtual_address() + segment_to_extend->virtual_size();
this->datahandler_->make_hole(segment_to_extend->file_offset() + segment_to_extend->physical_size(), size);
this->shift_symbols(from_address, shift);

Expected behavior

A clear and concise description of what you expected to happen.

In the above example, start addresses of functions shouldn't change.

And I think uint64_t from_address = section_to_extend->virtual_address() + size; need to be uint64_t from_address = section_to_extend->virtual_address() + section_to_extend->virtual_size();.

Environment (please complete the following information):

• Kernal version: 4.9.0-9-amd64
• System and Version : Debian GNU/Linux 9 (stretch)
• Target format: ELF
• LIEF commit version: b94900c

[Clcanny]

I can't find virtual_size method id Section, so I just use size as an alternative method temporarily. After this change, result match expectation.

uint64_t from_address = section_to_extend->virtual_address() + section_to_extend->size();

# objdump -d -j .text main | grep ">:" -A1 | grep -v "\-\-"
0000000000000530 <_start>:
 530:	31 ed                	xor    %ebp,%ebp
0000000000000560 <deregister_tm_clones>:
 560:	48 8d 3d c1 0a 20 00 	lea    0x200ac1(%rip),%rdi        # 201028 <__TMC_END__>
00000000000005a0 <register_tm_clones>:
 5a0:	48 8d 3d 81 0a 20 00 	lea    0x200a81(%rip),%rdi        # 201028 <__TMC_END__>
00000000000005f0 <__do_global_dtors_aux>:
 5f0:	80 3d 31 0a 20 00 00 	cmpb   $0x0,0x200a31(%rip)        # 201028 <__TMC_END__>
0000000000000630 <frame_dummy>:
 630:	48 8d 3d e1 07 20 00 	lea    0x2007e1(%rip),%rdi        # 200e18 <__JCR_END__>
0000000000000660 <main>:
 660:	55                   	push   %rbp
0000000000000670 <__libc_csu_init>:
 670:	41 57                	push   %r15
00000000000006e0 <__libc_csu_fini>:
 6e0:	f3 c3                	repz retq

# objdump -d -j .text main-extended | grep ">:" -A1 | grep -v "\-\-"
0000000000000530 <_start>:
 530:	31 ed                	xor    %ebp,%ebp
0000000000000560 <deregister_tm_clones>:
 560:	48 8d 3d c1 0a 20 00 	lea    0x200ac1(%rip),%rdi        # 201028 <_Jv_RegisterClasses>
00000000000005a0 <register_tm_clones>:
 5a0:	48 8d 3d 81 0a 20 00 	lea    0x200a81(%rip),%rdi        # 201028 <_Jv_RegisterClasses>
00000000000005f0 <__do_global_dtors_aux>:
 5f0:	80 3d 31 0a 20 00 00 	cmpb   $0x0,0x200a31(%rip)        # 201028 <_Jv_RegisterClasses>
0000000000000630 <frame_dummy>:
 630:	48 8d 3d e1 07 20 00 	lea    0x2007e1(%rip),%rdi        # 200e18 <__FRAME_END__+0x2005a0>
0000000000000660 <main>:
 660:	55                   	push   %rbp
0000000000000670 <__libc_csu_init>:
 670:	41 57                	push   %r15
00000000000006e0 <__libc_csu_fini>:
 6e0:	f3 c3                	repz retq

[romainthomas]

Hello @Clcanny
Thanks for this very clean report!

I can't find virtual_size method in Section

You are right and you can't find it because this field does not exist in the ELF format and you are also right about the fix.

Thank you!

[romainthomas]

Waiting for the CI on this branch fix/477