On Thursday, June 20, 2019 at 1:15:29 PM UTC-4, Nadav Har'El wrote:
>
> On Thu, Jun 20, 2019 at 3:21 PM Waldemar Kozaczuk <jwkoz...@gmail.com
> <javascript:>> wrote:
>
>> This patch provides necessary changes to OSv dynamic linker
>> to allow running non-PIEs (= Position Dependant Executables)
>> as long as they do not collide in virtual memory with kernel.
>>
>
> Thanks. Nice progress in making OSv run these standard executables!
> I have some minor comments and requests below.
>
>
>> Fixes #190
>>
>> Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com <javascript:>>
>> ---
>> Makefile | 3 ++-
>> core/elf.cc | 35 ++++++++++++++++++++++++-----------
>> include/osv/elf.hh | 1 +
>> modules/tests/Makefile | 8 ++++++--
>> tests/tst-non-pie.cc | 6 ++++++
>> 5 files changed, 39 insertions(+), 14 deletions(-)
>> create mode 100644 tests/tst-non-pie.cc
>>
>> diff --git a/Makefile b/Makefile
>> index 3c5a2077..440c1636 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -312,7 +312,8 @@ gcc-sysroot = $(if $(CROSS_PREFIX), --sysroot
>> external/$(arch)/gcc.bin) \
>> # To add something that will *not* be part of the main kernel, you can
>> do:
>> #
>> # mydir/*.o EXTRA_FLAGS = <MY_STUFF>
>> -EXTRA_FLAGS = -D__OSV_CORE__ -DOSV_KERNEL_BASE=$(kernel_base)
>> -DOSV_KERNEL_VM_SHIFT=$(kernel_vm_shift)
>> -DOSV_LZKERNEL_BASE=$(lzkernel_base)
>> +EXTRA_FLAGS = -D__OSV_CORE__ -DOSV_KERNEL_BASE=$(kernel_base)
>> -DOSV_KERNEL_VM_BASE=$(kernel_vm_base) \
>> + -DOSV_KERNEL_VM_SHIFT=$(kernel_vm_shift)
>> -DOSV_LZKERNEL_BASE=$(lzkernel_base)
>> EXTRA_LIBS =
>> COMMON = $(autodepend) -g -Wall -Wno-pointer-arith $(CFLAGS_WERROR)
>> -Wformat=0 -Wno-format-security \
>> -D __BSD_VISIBLE=1 -U _FORTIFY_SOURCE -fno-stack-protector
>> $(INCLUDES) \
>> diff --git a/core/elf.cc b/core/elf.cc
>> index 477a0177..c5d8beaf 100644
>> --- a/core/elf.cc
>> +++ b/core/elf.cc
>> @@ -34,6 +34,9 @@ TRACEPOINT(trace_elf_unload, "%s", const char *);
>> TRACEPOINT(trace_elf_lookup, "%s", const char *);
>> TRACEPOINT(trace_elf_lookup_addr, "%p", const void *);
>>
>> +extern void* elf_start;
>> +extern size_t elf_size;
>>
>
> Eventually we should have those in a header file... We're doing this
> explicit "extern" thing from too many places already.
> But not important now.
>
> +
>> using namespace boost::range;
>>
>> namespace {
>> @@ -267,15 +270,6 @@ void file::load_elf_header()
>> || _ehdr.e_ident[EI_OSABI] == 0)) {
>> throw osv::invalid_elf_error("bad os abi");
>> }
>> - // We currently only support running ET_DYN objects (shared library
>> or
>> - // position-independent executable). In the future we can add
>> support for
>> - // ET_EXEC (ordinary, position-dependent executables) but it will
>> require
>> - // loading them at their specified address and moving the kernel out
>> of
>> - // their way.
>> - if (_ehdr.e_type != ET_DYN) {
>> - throw osv::invalid_elf_error(
>> - "bad executable type (only shared-object or PIE
>> supported)");
>> - }
>>
>
> I think that we should leave the test but allow either ET_DYN or ET_EXEC,
> but nothing else.
> E.g., we don't accept silly things like ".o" files or core files.
> I don't expect anybody to try this, but since we already have this code,
> we can fix it instead of remove it.
>
Makes sense. Relatedly is there a way to detect if it is a statically
linked executable. Look for missing INTERP segment?
A actually tried to run the golang example with this patch applied built
without -pie flag (the default) and it becomes statically linked:
ELF 64-bit LSB executable, x86-64, version 1 (SYSV), statically linked, Go
BuildID=DW67KbYc5IVwTIMqoOk6/rOXOWO8FcCqwLa8h8mh8/D2KlTg3IeNi2cqc9tqbm/
IUNwPuHLnppbny4n9Nfv, not stripped
And I get this exception:
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>
We should at least catch that we do not support this type of executable
I know we have a problem of missing sbrk syscall per
https://github.com/cloudius-systems/osv/issues/212. But golang does not
seem to be using it - how does it allocate memory?, maybe through mmap. I
see it is using clone() which we do not implement.
>
> }
>>
>> void file::read(Elf64_Off offset, void* data, size_t size)
>> @@ -297,17 +291,35 @@ void* align(void* addr, ulong align, ulong offset)
>>
>> }
>>
>> +static bool intersects_with_kernel(Elf64_Addr elf_addr)
>> +{
>> + void *addr = reinterpret_cast<void*>(elf_addr);
>> + return addr >= elf_start && addr < elf_start + elf_size;
>> +}
>> +
>> void object::set_base(void* base)
>> {
>> auto p = std::min_element(_phdrs.begin(), _phdrs.end(),
>> [](Elf64_Phdr a, Elf64_Phdr b)
>> { return a.p_type == PT_LOAD
>> && a.p_vaddr < b.p_vaddr; });
>> - _base = align(base, p->p_align, p->p_vaddr & (p->p_align - 1)) -
>> p->p_vaddr;
>> auto q = std::min_element(_phdrs.begin(), _phdrs.end(),
>> [](Elf64_Phdr a, Elf64_Phdr b)
>> { return a.p_type == PT_LOAD
>> && a.p_vaddr > b.p_vaddr; });
>> +
>> + if (is_non_pie_executable() && base != ((void*)OSV_KERNEL_VM_BASE)) {
>>
>
> What is this "base == OSV_KERNEL_VM_BASE case", and why does it fall back
> to the "else" below?
>
This is an ugly way to filter out our kernel itself which is an executable
as well (set_base is called on loader.elf as well at some point).
>
> + // Verify non-PIE executable does not collide with the kernel
>> + if (intersects_with_kernel(p->p_vaddr) ||
>> intersects_with_kernel(q->p_vaddr + q->p_memsz)) {
>> + abort("Non-PIE executable [%s] collides with kernel: [%p-%p]
>> !\n",
>> + pathname().c_str(), p->p_vaddr, q->p_vaddr +
>> q->p_memsz);
>> + }
>> + // The base for non-PIEs ( = Position Dependant Executables)
>> should be set to 0
>> + _base = 0x0;
>>
>
> Why? What is its value before you set it here?
>
Are you saying this value would be 0 already?
>
>
>> + } else {
>> + _base = align(base, p->p_align, p->p_vaddr & (p->p_align - 1)) -
>> p->p_vaddr;
>> + }
>> +
>> _end = _base + q->p_vaddr + q->p_memsz;
>> }
>>
>> @@ -1223,7 +1235,8 @@ program::load_object(std::string name,
>> std::vector<std::string> extra_path,
>> _modules_rcu.assign(new_modules.release());
>> osv::rcu_dispose(old_modules);
>> ef->load_segments();
>> - _next_alloc = ef->end();
>> + if (!ef->is_non_pie_executable())
>> + _next_alloc = ef->end();
>>
> add_debugger_obj(ef.get());
>> loaded_objects.push_back(ef);
>> ef->load_needed(loaded_objects);
>> diff --git a/include/osv/elf.hh b/include/osv/elf.hh
>> index 894ea237..775d8c8d 100644
>> --- a/include/osv/elf.hh
>> +++ b/include/osv/elf.hh
>> @@ -365,6 +365,7 @@ public:
>> void init_static_tls();
>> size_t initial_tls_size() { return _initial_tls_size; }
>> void* initial_tls() { return _initial_tls.get(); }
>> + bool is_non_pie_executable() { return _ehdr.e_type == ET_EXEC; }
>> std::vector<ptrdiff_t>& initial_tls_offsets() { return
>> _initial_tls_offsets; }
>> protected:
>> virtual void load_segment(const Elf64_Phdr& segment) = 0;
>> diff --git a/modules/tests/Makefile b/modules/tests/Makefile
>> index b0bfb031..d8ea6049 100644
>> --- a/modules/tests/Makefile
>> +++ b/modules/tests/Makefile
>> @@ -67,7 +67,11 @@ $(out)/tests/tst-getopt-pie.o:
>> $(src)/tests/tst-getopt.cc
>> $(makedir)
>> $(call quiet, $(CXX) $(CXXFLAGS) -c -o $@ $<, CXX $*.cc)
>> $(out)/tests/tst-getopt-pie.so: $(out)/tests/tst-getopt-pie.o
>> - $(call quiet, $(CXX) $(CXXFLAGS) -pie -o $@ $< $(LIBS), LD $*.so)
>> + $(call quiet, $(CXX) $(CXXFLAGS) -pie -o $@ $< $(LIBS), LD $@)
>> +
>> +$(out)/tests/tst-non-pie.so: CXXFLAGS:=$(subst -fPIC,-no-pie,$(CXXFLAGS))
>> +$(out)/tests/tst-non-pie.so: $(src)/tests/tst-non-pie.cc
>> + $(call quiet, $(CXX) $(CXXFLAGS) -o $@ $< $(LIBS), LD $@)
>>
>> # The rofs test image mounts /tmp as ramfs and 4 tests that exercise
>> file system
>> # fail due to some unresolved bugs or other shortcomings of the ramfs
>> implementation
>> @@ -124,7 +128,7 @@ tests := tst-pthread.so misc-ramdisk.so tst-vblk.so
>> tst-bsd-evh.so \
>> tst-ttyname.so tst-pthread-barrier.so tst-feexcept.so tst-math.so
>> \
>> tst-sigaltstack.so tst-fread.so tst-tcp-cork.so tst-tcp-v6.so \
>> tst-calloc.so tst-crypt.so tst-non-fpic.so tst-small-malloc.so \
>> - tst-mmx-fpu.so tst-getopt.so tst-getopt-pie.so
>> + tst-mmx-fpu.so tst-getopt.so tst-getopt-pie.so tst-non-pie.so
>> # libstatic-thread-variable.so tst-static-thread-variable.so \
>>
>> tests += testrunner.so
>> diff --git a/tests/tst-non-pie.cc b/tests/tst-non-pie.cc
>> new file mode 100644
>> index 00000000..be1cf2bf
>> --- /dev/null
>> +++ b/tests/tst-non-pie.cc
>> @@ -0,0 +1,6 @@
>> +#include <stdio.h>
>> +
>> +int main(){
>> + printf("Hello from C code\n");
>> + return 0;
>> +}
>> --
>> 2.20.1
>>
>> --
>> You received this message because you are subscribed to the Google Groups
>> "OSv Development" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to osv...@googlegroups.com <javascript:>.
>> To view this discussion on the web visit
>> https://groups.google.com/d/msgid/osv-dev/20190620122108.3549-1-jwkozaczuk%40gmail.com
>> .
>> For more options, visit https://groups.google.com/d/optout.
>>
>
--
You received this message because you are subscribed to the Google Groups "OSv
Development" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to osv-dev+unsubscr...@googlegroups.com.
To view this discussion on the web visit
https://groups.google.com/d/msgid/osv-dev/8312547e-9aec-4790-bfac-56dd4f0bae66%40googlegroups.com.
For more options, visit https://groups.google.com/d/optout.
