Delay processing PT_NOTE segments until all PT_LOAD segments are mmapped

[wkozaczuk]

Some ELF executable (see regular Golang one) are constructed such that PT_NOTE segment comes before PT_LOAD as you see below per readelf:

Elf file type is EXEC (Executable file)
Entry point 0x44f2a0
There are 7 program headers, starting at offset 64

Program Headers:
  Type           Offset             VirtAddr           PhysAddr
                 FileSiz            MemSiz              Flags  Align
  PHDR           0x0000000000000040 0x0000000000400040 0x0000000000400040
                 0x0000000000000188 0x0000000000000188  R      0x1000
  NOTE           0x0000000000000f9c 0x0000000000400f9c 0x0000000000400f9c
                 0x0000000000000064 0x0000000000000064  R      0x4
  LOAD           0x0000000000000000 0x0000000000400000 0x0000000000400000
                 0x00000000000821d5 0x00000000000821d5  R E    0x1000
  LOAD           0x0000000000083000 0x0000000000483000 0x0000000000483000
                 0x00000000000905dd 0x00000000000905dd  R      0x1000
  LOAD           0x0000000000114000 0x0000000000514000 0x0000000000514000
                 0x00000000000136c0 0x00000000000323f8  RW     0x1000
  GNU_STACK      0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000  RW     0x8
  LOOS+0x5041580 0x0000000000000000 0x0000000000000000 0x0000000000000000
                 0x0000000000000000 0x0000000000000000         0x8

OSv linker (see object::load_segments()) processes segments in the order as they come. And when PT_NOTE segment comes before PT_LOAD we get page fault in the ELF note constructor looking like this:

OSv v0.53.0-36-g99804a6b
eth0: 192.168.122.15
Booted up in 138.79 ms
page fault outside application, addr: 0x0000000000400000
[registers]
RIP: 0x0000000040359a57 <elf::Elf64_Note::Elf64_Note(void*, char*)+39>
RFL: 0x0000000000010206  CS:  0x0000000000000008  SS:  0x0000000000000010
RAX: 0x0000000000000004  RBX: 0x00002000000ff4f0  RCX: 0x0000000000000000  RDX: 0x0000000000400fa8
RSI: 0x0000000000400f9c  RDI: 0x00002000000ff4f0  RBP: 0x00002000000ff4a0  R8:  0x00000000409025c0
R9:  0x00000000409025c0  R10: 0xffffa00000e54a90  R11: 0x0000000000000001  R12: 0x0000000000400f9c
R13: 0x00000000409025c0  R14: 0xffffa00000e54a90  R15: 0x0000000000000001  RSP: 0x00002000000ff460
Aborted

[backtrace]
0x0000000040349602 <???+1077188098>
0x000000004034ae99 <mmu::vm_fault(unsigned long, exception_frame*)+393>
0x00000000403a70fd <page_fault+125>
0x00000000403a5f76 <???+1077567350>
0x000000004035d82e <elf::object::load_segments()+606>
0x00000000403608f1 <elf::program::load_object(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, std::vector<std::shared_ptr<elf::object>, std::allocator<std::shared_ptr<elf::object> > >&)+1441>
0x0000000040361222 <elf::program::get_library(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > >, bool)+322>
0x000000004042f21c <osv::application::application(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool, std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::function<0x000000004042f64b <osv::application::run(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&, bool, std::unordered_map<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::hash<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::equal_to<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >, std::allocator<std::pair<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > > const*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::function<void ()>0x000000004042f87b <osv::application::run(std::vector<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, std::allocator<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > > const&)+91>
0x000000004022df55 <do_main_thread(void*)+2501>
0x000000004045d5b5 <???+1078318517>
0x0000000040400126 <thread_main_c+38>
0x00000000403a6ef2 <???+1077571314>

Possibly the right fix would either require delaying processing PT_NOTE after all PT_LOAD segments are processed or making all PT_LOAD processed first which all boils down to the similar type of a solution.

[nyh]

Good catch.
The smallest patch is probably to move the PT_NOTE handling to a separate loop right after the loop it's now in (with a comment saying it needs to be after the LOAD).
But perhaps a "cleaner" solution is to change load_segments() to do only what it's name says - call load_segment() on the segments listed as PT_LOAD. Just like unload_segments() only unloads these segments and does nothing else. We could then have a separate function to look for PT_TLS and set the TLS variables, or to look for the dynamic table, or check if there is a PT_INTERP, and a separate function to read notes (maybe not each of these needs to be a separate function, but it can be), and we can call these in the order we want. We already have other speciaized functions that only look for specific headers: e.g., fix_permissions() only looks for PT_GNU_RELRO headers. make_text_writeable() only looks for PT_LOAD,