[PATCH V2] Allow running non-PIE executables that do not collide with kernel

12 views

Skip to first unread message

Waldemar Kozaczuk

unread,

Jun 20, 2019, 8:21:23 AM6/20/19

to osv...@googlegroups.com, Waldemar Kozaczuk

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.

Fixes #190

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
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;
+
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)");
- }
}

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)) {
+ // 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;
+ } 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

Nadav Har'El

unread,

Jun 20, 2019, 1:15:29 PM6/20/19

to Waldemar Kozaczuk, Osv Dev

On Thu, Jun 20, 2019 at 3:21 PM Waldemar Kozaczuk <jwkoz...@gmail.com> 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.

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.

}

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?

+ // 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?

--
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+u...@googlegroups.com.
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.

Waldek Kozaczuk

unread,

Jun 20, 2019, 2:33:29 PM6/20/19

to OSv Development

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?

To unsubscribe from this group and stop receiving emails from it, send an email to osv...@googlegroups.com.

Nadav Har'El

unread,

Jun 20, 2019, 4:32:07 PM6/20/19

to Waldek Kozaczuk, OSv Development

On Thu, Jun 20, 2019 at 9:33 PM Waldek Kozaczuk <jwkoz...@gmail.com> wrote:

On Thursday, June 20, 2019 at 1:15:29 PM UTC-4, Nadav Har'El wrote:
- // 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?

Yes, I think this is exactly the distinction - the presence or lack of a PT_INTERP section. By the way, we already put this into a flag

called _is_executable but that information may not be available at this point (I don't remember how the code is organized)

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>

Strange place to fail, but since we will have so many other problems with static executables (we have an issue about it), and they are so out of fashion, I'm not worried that we don't support them.

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

Indeed. We should. Can you please add a check for it too?

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.

+

+ 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).

Anyway, is this really the best way to check if we're the kernel? We also have is_core() to recognize if this is the kernel, no?

Because if we're loading a real executable which really had its base the same as the kernel's, we *shouldn't* just

say it's ok, we should continue with the following case and discover that it collides with the kernel.

If this base!= is the only solution, please at least add a comment saying this. Thanks.

+ // 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?

I don't know. I didn't check. But if you know you need _base = 0, then fine.

To unsubscribe from this group and stop receiving emails from it, send an email to osv-dev+u...@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.

Waldemar Kozaczuk

unread,

Jun 21, 2019, 12:10:05 AM6/21/19

to osv...@googlegroups.com, Waldemar Kozaczuk

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.

Fixes #190

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>
---
Makefile | 3 ++-

core/elf.cc | 45 +++++++++++++++++++++++++++++++-----------

include/osv/elf.hh | 1 +
modules/tests/Makefile | 8 ++++++--
tests/tst-non-pie.cc | 6 ++++++

5 files changed, 49 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..42fe07d9 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;

+
using namespace boost::range;

namespace {

@@ -224,7 +227,6 @@ memory_image::memory_image(program& prog, void* base)
auto p = static_cast<Elf64_Phdr*>(base + _ehdr.e_phoff);
assert(_ehdr.e_phentsize == sizeof(*p));
_phdrs.assign(p, p + _ehdr.e_phnum);
- set_base(base);
}

void memory_image::load_segment(const Elf64_Phdr& phdr)
@@ -267,14 +269,9 @@ 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) {

+ if (!(_ehdr.e_type == ET_DYN || _ehdr.e_type == ET_EXEC)) {

throw osv::invalid_elf_error(
- "bad executable type (only shared-object or PIE supported)");

+ "bad executable type (only shared-object, PIE or non-PIE executable supported)");
}
}

@@ -297,17 +294,37 @@ 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_core() && is_non_pie_executable()) {

+ // 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);
+ }

+ // Override the passed in value as the base for non-PIEs (Position Dependant Executables)
+ // needs to be set to 0 because all the addresses in it are absolute

+ _base = 0x0;
+ } else {

+ // Otherwise for kernel, PIEs and shared libraries set the base as requested by caller

+ _base = align(base, p->p_align, p->p_vaddr & (p->p_align - 1)) - p->p_vaddr;
+ }
+
_end = _base + q->p_vaddr + q->p_memsz;
}

@@ -447,6 +464,9 @@ void object::load_segments()
abort("Unknown p_type in executable %s: %d\n", pathname(), phdr.p_type);
}
}
+ if (!is_core() && _ehdr.e_type == ET_EXEC && !_is_executable) {
+ abort("Statically linked executables are not supported!\n");
+ }
// As explained in issue #352, we currently don't correctly support TLS
// used in PIEs.
if (_is_executable && _tls_segment) {
@@ -1099,7 +1119,9 @@ void create_main_program()
program::program(void* addr)
: _next_alloc(addr)
{
- _core = std::make_shared<memory_image>(*this, (void*)(ELF_IMAGE_START + OSV_KERNEL_VM_SHIFT));
+ void *program_base = (void*)(ELF_IMAGE_START + OSV_KERNEL_VM_SHIFT);
+ _core = std::make_shared<memory_image>(*this, program_base);
+ _core->set_base(program_base);
assert(_core->module_index() == core_module_index);
_core->load_segments();
set_search_path({"/", "/usr/lib"});
@@ -1223,7 +1245,8 @@ program::load_object(std::string name, std::vector<std::string> extra_path,

Waldek Kozaczuk

unread,

Jun 21, 2019, 12:16:02 AM6/21/19

to OSv Development

I have sent a new patch.

I think I have found new bug unrelated to my patch. When PT_NOTE comes before PT_LOAD then our elf::load_segments() will try to access data located in the memory in PT_LOAD segments before they have been mmapped() by load_segment(). We should delay processing PT_NOTE until all PT_LOAD are loaded.

Commit Bot

unread,

Jun 23, 2019, 6:38:12 AM6/23/19

to osv...@googlegroups.com, Waldemar Kozaczuk

From: Waldemar Kozaczuk <jwkoz...@gmail.com>
Committer: Nadav Har'El <n...@scylladb.com>
Branch: master

Allow running non-PIE executables that do not collide with kernel

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.

Fixes #190

Signed-off-by: Waldemar Kozaczuk <jwkoz...@gmail.com>

Message-Id: <20190621040947.2...@gmail.com>

---
diff --git a/Makefile b/Makefile

--- 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

--- 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