Add an ELF loader that can load and run an executable in the sandbox

elf_loader.cc

+// Copyright (c) 2011 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include <elf.h>
+#include <fcntl.h>
+#include <stdio.h>
+#include <string.h>
+#include <sys/mman.h>
+#include <unistd.h>
+
+#include "sandbox_impl.h"
+#include "syscall_entrypoint.h"
+#include "tls_setup.h"
+
+
+#if defined(__x86_64__)
+# define ElfW(name) Elf64_##name
+# define ElfW_ELFCLASS ELFCLASS64
+#elif defined(__i386__)
+# define ElfW(name) Elf32_##name
+# define ElfW_ELFCLASS ELFCLASS32
+#else
+# error "Unsupported target platform"

[Markus (顧孟勤), 2011/08/15 22:05:23]

I keep making this same mistake all the time. There shouldn't be any quotes
around the string in an "#error" statement. Yes, it is confusing...

+#endif
+
+
+uintptr_t PageSizeRoundDown(uintptr_t val) {

[Markus (顧孟勤), 2011/08/15 22:05:23]

Are you deliberately not making these functions "static"? It might in fact make
sense to make them globally available, but in that case I wonder whether this is
the best place to put them.

+  return val & ~(getpagesize() - 1);

[Markus (顧孟勤), 2011/08/15 22:05:23]

You can also write:

 val & -getpagesize()

Not sure which one is more readable. Ignore this comment, if you think your
statement works better.

And of course, throughout the rest of the code, we just assume that the
user-visible pagesize is always 4096 on the platforms we care about. The people
on the kernel mailing list seem divided on whether that is correct or not.

I remember Linus saying that this will never change, but others claim that he
his since softened his stance on this question.

+}
+
+uintptr_t PageSizeRoundUp(uintptr_t val) {
+  return (val + getpagesize() - 1) & ~(getpagesize() - 1);

[Markus (顧孟勤), 2011/08/15 22:05:23]

Maybe:

  return PageSizeRoundDown(val + getpagesize() - 1);

The compiler should optimize both versions to do the same.

+}
+
+static ElfW(auxv_t) *FindAuxv(int argc, char **argv) {
+  char **ptr = argv + argc + 1;
+  // Skip over envp.
+  while (*ptr != NULL)
+    ptr++;

[Markus (顧孟勤), 2011/08/15 22:05:23]

I'd prefer "{ }" around this statement. But maybe, that's just Google3-habits
failing to die :-)

Been burned by not having braces one too many times.

Alternatively, you could always write:

  while (*ptr++) { }

You wouldn't need the extra increment outside of the block. But that might be
too condensed and hard to read.

+  ptr++;
+  return (ElfW(auxv_t) *) ptr;
+}
+
+static void SetAuxvField(ElfW(auxv_t) *auxv, unsigned type, uintptr_t value) {
+  for (; auxv->a_type != AT_NULL; auxv++) {
+    if (auxv->a_type == type) {
+      auxv->a_un.a_val = value;
+      return;
+    }
+  }

[Markus (顧孟勤), 2011/08/15 22:05:23]

Is it OK, if we fail to find the matching type? Should we die() when this
happens?

+}
+
+static void JumpToElfEntryPoint(void *stack, void *entry_point,
+                                void *atexit_func) {

[Markus (顧孟勤), 2011/08/15 22:05:23]

You might consider declaring this function as __attribute__((noreturn))

+#if defined(__x86_64__)
+  asm("mov %0, %%rsp\n"
+      "jmp *%1\n"
+      // %edx is registered as an atexit handler if non-zero.
+      : : "r"(stack), "r"(entry_point), "d"(atexit_func));
+#elif defined(__i386__)
+  asm("mov %0, %%esp\n"
+      "jmp *%1\n"
+      // %rdx is registered as an atexit handler if non-zero.
+      : : "r"(stack), "r"(entry_point), "d"(atexit_func));
+#else
+# error "Unsupported target platform"
+#endif
+}
+
+static void *LoadElfObject(int fd, ElfW(auxv_t) *auxv) {
+  // Load and check headers.
+  ElfW(Ehdr) elf_header;
+  if (pread(fd, &elf_header, sizeof(elf_header), 0) != sizeof(elf_header)) {
+    Sandbox::die("Failed to read ELF header");
+  }
+  if (memcmp(elf_header.e_ident, ELFMAG, SELFMAG) != 0) {
+    Sandbox::die("Not an ELF file");
+  }
+  if (elf_header.e_ident[EI_CLASS] != ElfW_ELFCLASS) {
+    Sandbox::die("Unexpected ELFCLASS");
+  }
+  ssize_t phdrs_size = elf_header.e_phentsize * elf_header.e_phnum;

[Markus (顧孟勤), 2011/08/15 22:05:23]

Are we worried about malicious binaries overflowing the the phdrs_size variable?
Are we worried about e_phtentsize or e_phnum being negative? (more precisely,
they are currently unsigned, but they subsequently get used in situations where
they would be treated as signed)

+  char *phdrs = (char *) alloca(phdrs_size);

[Markus (顧孟勤), 2011/08/15 22:05:23]

You should check that the alloca() request succeeded.

+  if (pread(fd, phdrs, phdrs_size, elf_header.e_phoff) != phdrs_size) {
+    Sandbox::die("Failed to read ELF program headers");
+  }
+
+  // Scan program headers to find the overall size of the ELF object.
+  size_t load_size = 0;
+  char *required_start = 0;
+  for (int index = 0; index < elf_header.e_phnum; index++) {
+    ElfW(Phdr) *segment =
+      (ElfW(Phdr) *) (phdrs + elf_header.e_phentsize * index);
+    if (segment->p_type == PT_LOAD) {
+      size_t segment_end = PageSizeRoundUp(segment->p_vaddr + segment->p_memsz);
+      if (load_size < segment_end) {
+        load_size = segment_end;
+      }
+      if (elf_header.e_type == ET_EXEC) {

[Markus (顧孟勤), 2011/08/15 22:05:23]

I take it, we should see no or exactly one ET_EXEC header. Can we check this?
Maybe also add a comment explaining the two possible cases, as that makes the
next couple of lines more obvious.

+        required_start = (char *) PageSizeRoundDown(segment->p_vaddr);
+      }
+    }
+  }
+

[Markus (顧孟勤), 2011/08/15 22:05:23]

You might want to add a sanity check that "load_size" is positive and non-zero.

+  // Reserve address space.
+  // For executables that must be loaded at a fixed address, we could
+  // use MAP_FIXED, but if the address range is already occupied then
+  // that will clobber the existing mappings without warning, which is
+  // bad.  Instead, use an address hint and check that we got the
+  // expected address.
+  char *base_addr = (char *) mmap(required_start, load_size, PROT_NONE,
+                                  MAP_PRIVATE | MAP_ANONYMOUS, -1, 0);
+  if (base_addr == MAP_FAILED) {
+    Sandbox::die("Failed to reserve address space");
+  }
+  if (required_start != NULL && base_addr != required_start) {
+    Sandbox::die("Failed to reserve address space at fixed address");
+  }
+
+  char *load_offset = (char *) (base_addr - required_start);
+  char *entry_point = load_offset + elf_header.e_entry;
+  SetAuxvField(auxv, AT_ENTRY, (uintptr_t) entry_point);
+  SetAuxvField(auxv, AT_BASE, (uintptr_t) load_offset);
+  SetAuxvField(auxv, AT_PHNUM, elf_header.e_phnum);
+  SetAuxvField(auxv, AT_PHENT, elf_header.e_phentsize);
+  // Note that this assumes that the program headers are included in a
+  // PT_LOAD segment for which the file offsets matches the mapping
+  // offset, but Linux assumes this too when setting AT_PHDR.

[Markus (顧孟勤), 2011/08/15 22:05:23]

Any chance that we could actually test this assumption at run-time? I know, I'm
paranoid. Maybe, overly so. Tell me to shut up, if that's the case.

+  SetAuxvField(auxv, AT_PHDR, (uintptr_t) base_addr + elf_header.e_phoff);
+
+  for (int index = 0; index < elf_header.e_phnum; index++) {
+    ElfW(Phdr) *segment =
+      (ElfW(Phdr) *) (phdrs + elf_header.e_phentsize * index);
+    if (segment->p_type == PT_LOAD) {
+      uintptr_t segment_start = PageSizeRoundDown(segment->p_vaddr);
+      size_t segment_end = PageSizeRoundUp(segment->p_vaddr + segment->p_memsz);

[Markus (顧孟勤), 2011/08/15 22:05:23]

Again, do we worry about overflows?

+      int prot = 0;
+      if ((segment->p_flags & PF_R) != 0)
+        prot |= PROT_READ;
+      if ((segment->p_flags & PF_W) != 0)
+        prot |= PROT_WRITE;
+      if ((segment->p_flags & PF_X) != 0)
+        prot |= PROT_EXEC;
+      void *result = mmap(load_offset + segment_start,

[Markus (顧孟勤), 2011/08/15 22:05:23]

Don't we have the same problem as for the other call to mmap()? MAP_FIXED is
potentially going to overwrite things that shouldn't be clobbered.

+                          segment_end - segment_start,
+                          prot, MAP_PRIVATE | MAP_FIXED, fd,
+                          PageSizeRoundDown(segment->p_offset));
+      if (result == MAP_FAILED) {
+        Sandbox::die("Failed to map ELF segment");
+      }
+      // TODO(mseaborn): Support a BSS that goes beyond the file's extent.

[Markus (顧孟勤), 2011/08/15 22:05:23]

What happens if we encounter something like that? Is the loader going to crash?
The loaded application?

+      if ((segment->p_flags & PF_W) != 0) {
+        // Zero the BSS to the end of the page.
+        uintptr_t bss_start = segment->p_vaddr + segment->p_filesz;

[Markus (顧孟勤), 2011/08/15 22:05:23]

Do we need sanity checks that we don't zero out something that we shouldn't
zero?

+        memset(load_offset + bss_start, 0, segment_end - bss_start);
+      }
+    }
+  }
+  if (close(fd) != 0) {
+    Sandbox::die("close() failed");
+  }
+  return entry_point;
+}
+
+int main(int argc, char **argv) {
+  if (argc < 2) {
+    fprintf(stderr, "Usage: %s executable args...\n", argv[0]);
+    return 1;
+  }
+
+  const char *executable_filename = argv[1];
+  int executable_fd = open(executable_filename, O_RDONLY);
+  if (executable_fd < 0) {
+    fprintf(stderr, "Failed to open executable %s: %s\n",
+            executable_filename, strerror(errno));
+    return 1;
+  }
+
+  playground::g_policy.allow_file_namespace = true;

[Markus (顧孟勤), 2011/08/15 22:05:23]

We probably want a more fine-grained policy at some point. How difficult is is
to do what "ldd" does and find all the files that the program depends on?

+  playground::AddTlsSetupSyscall();
+  StartSeccompSandbox();
+
+  ElfW(auxv_t) *auxv = FindAuxv(argc, argv);
+  SetAuxvField(auxv, AT_SYSINFO, (uintptr_t) syscallEntryPointNoFrame);

[Markus (顧孟勤), 2011/08/15 22:05:23]

Does this work on both x86-32 and x86-64. I vaguely recall that glibc behaves
differently on these two platforms.

+  char **stack = argv;
+  *(long *) stack = argc - 1;

[Markus (顧孟勤), 2011/08/15 22:05:23]

Are there alignment requirements that we need to worry about? Do we have a
guarantee that argv is already aligned properly?

+  void *entry_point = LoadElfObject(executable_fd, auxv);
+  JumpToElfEntryPoint(stack, entry_point, 0);
+}