[LibOS/PAL] Memory corruption fixes

This commit contains the following memory-corruption fixes: - Add a separate debug_print_vma() function - Fix _DkGetAvailableUserAddressRange() returning incorrect memory range - Protect memory occupied by LibOS code from being overwritten - Check return values of init_brk*() calls - Add check for allocation failure in __bkeep_mmap() - Fix initial brk region allocation (previously could overwrite PAL data)
gramineproject · Jun 10, 2019 · 5a5b42f · 5a5b42f
1 parent 15ca4a9
commit 5a5b42f
Show file tree

Hide file tree

Showing 12 changed files with 104 additions and 56 deletions.
diff --git a/LibOS/shim/include/shim_internal.h b/LibOS/shim/include/shim_internal.h
@@ -745,6 +745,8 @@ extern const char ** initial_envp;
 void get_brk_region (void ** start, void ** end, void ** current);
 
 int reset_brk (void);
+struct shim_handle;
+int init_brk_from_executable (struct shim_handle * exec);
 int init_brk_region (void * brk_region);
 int init_heap (void);
 int init_internal_map (void);

diff --git a/LibOS/shim/src/bookkeep/shim_vma.c b/LibOS/shim/src/bookkeep/shim_vma.c
@@ -306,6 +306,14 @@ int init_vma (void)
     if (ret < 0)
         return ret;
 
+    /* Keep track of LibOS code itself so nothing overwrites it */
+    ret = __bkeep_preloaded(&__load_address,
+                            ALIGN_UP(&__load_address_end),
+                            PROT_READ, MAP_PRIVATE|MAP_ANONYMOUS|VMA_INTERNAL,
+                            "LibOS");
+    if (ret < 0)
+        return ret;
+
     /* Initialize the allocator */
 
     if (!(vma_mgr = create_mem_mgr(init_align_up(VMA_MGR_ALLOC)))) {
@@ -1252,33 +1260,38 @@ BEGIN_CP_FUNC(all_vmas)
 }
 END_CP_FUNC_NO_RS(all_vmas)
 
+void debug_print_vma (struct shim_vma *vma)
+{
+    const char * type = "", * name = "";
+
+    if (vma->file) {
+        if (!qstrempty(&vma->file->path)) {
+            type = " path=";
+            name = qstrgetstr(&vma->file->path);
+        } else if (!qstrempty(&vma->file->uri)) {
+            type = " uri=";
+            name = qstrgetstr(&vma->file->uri);
+        }
+    }
+
+    SYS_PRINTF("[%p-%p] prot=%08x flags=%08x%s%s offset=%ld%s%s%s%s\n",
+               vma->start, vma->end,
+               vma->prot,
+               vma->flags & ~(VMA_INTERNAL|VMA_UNMAPPED|VMA_TAINTED|VMA_CP),
+               type, name,
+               vma->offset,
+               vma->flags & VMA_INTERNAL ? " (internal)" : "",
+               vma->flags & VMA_UNMAPPED ? " (unmapped)" : "",
+               vma->comment[0] ? " comment=" : "",
+               vma->comment[0] ? vma->comment : "");
+}
+
 void debug_print_vma_list (void)
 {
     SYS_PRINTF("vma bookkeeping:\n");
 
     struct shim_vma * vma;
     LISTP_FOR_EACH_ENTRY(vma, &vma_list, list) {
-        const char * type = "", * name = "";
-
-        if (vma->file) {
-            if (!qstrempty(&vma->file->path)) {
-                type = " path=";
-                name = qstrgetstr(&vma->file->path);
-            } else if (!qstrempty(&vma->file->uri)) {
-                type = " uri=";
-                name = qstrgetstr(&vma->file->uri);
-            }
-        }
-
-        SYS_PRINTF("[%p-%p] prot=%08x flags=%08x%s%s offset=%ld%s%s%s%s\n",
-                   vma->start, vma->end,
-                   vma->prot,
-                   vma->flags & ~(VMA_INTERNAL|VMA_UNMAPPED|VMA_TAINTED|VMA_CP),
-                   type, name,
-                   vma->offset,
-                   vma->flags & VMA_INTERNAL ? " (internal)" : "",
-                   vma->flags & VMA_UNMAPPED ? " (unmapped)" : "",
-                   vma->comment[0] ? " comment=" : "",
-                   vma->comment[0] ? vma->comment : "");
+        debug_print_vma(vma);
     }
 }
diff --git a/LibOS/shim/src/elf/shim_rtld.c b/LibOS/shim/src/elf/shim_rtld.c
@@ -1481,8 +1481,6 @@ int init_internal_map (void)
     return 0;
 }
 
-int init_brk_from_executable (struct shim_handle * exec);
-
 int init_loader (void)
 {
     struct shim_thread * cur_thread = get_cur_thread();
@@ -1509,7 +1507,9 @@ int init_loader (void)
         exec_map = __search_map_by_handle(exec);
     }
 
-    init_brk_from_executable(exec);
+    ret = init_brk_from_executable(exec);
+    if (ret < 0)
+        goto out;
 
     if (!interp_map
         && __need_interp(exec_map)
@@ -1525,16 +1525,17 @@ int init_loader (void)
 int init_brk_from_executable (struct shim_handle * exec)
 {
     struct link_map * exec_map = __search_map_by_handle(exec);
+    int ret = 0;
 
     if (exec_map) {
         /*
          * Chia-Che 8/24/2017:
          * initialize brk region at the end of the executable data segment.
          */
-        init_brk_region((void *) ALIGN_UP(exec_map->l_map_end));
+        ret = init_brk_region((void *) ALIGN_UP(exec_map->l_map_end));
     }
 
-    return 0;
+    return ret;
 }
 
 int register_library (const char * name, unsigned long load_address)

diff --git a/LibOS/shim/src/sys/shim_brk.c b/LibOS/shim/src/sys/shim_brk.c
@@ -71,6 +71,8 @@ int init_brk_region (void * brk_region)
     }
 
     int flags = MAP_PRIVATE|MAP_ANONYMOUS;
+    bool brk_on_heap = true;
+    const int TRIES = 10;
 
     /*
      * Chia-Che 8/24/2017
@@ -80,24 +82,52 @@ int init_brk_region (void * brk_region)
      * should be within [exec-data-end, exec-data-end + 0x2000000)
      */
     if (brk_region) {
-        while (true) {
-            uint32_t rand;
-            int ret = DkRandomBitsRead(&rand, sizeof(rand));
-            if (ret < 0)
-                return -convert_pal_errno(-ret);
-            rand %= 0x2000000;
-            rand = ALIGN_UP(rand);
-
-            struct shim_vma_val vma;
-            if (lookup_overlap_vma(brk_region + rand, brk_max_size, &vma)
-                == -ENOENT) {
-                brk_region += rand;
-                break;
+        size_t max_brk = 0;
+        if (PAL_CB(user_address.end) >= PAL_CB(executable_range.end))
+            max_brk = PAL_CB(user_address.end) - PAL_CB(executable_range.end);
+
+        /* Check whether the brk region can potentially be located after exec at all. */
+        if (brk_max_size <= max_brk) {
+            int try;
+            for (try = TRIES; try > 0; try--) {
+                uint32_t rand = 0;
+#if ENABLE_ASLR == 1
+                int ret = DkRandomBitsRead(&rand, sizeof(rand));
+                if (ret < 0)
+                    return -convert_pal_errno(-ret);
+                rand %= MIN(0x2000000, PAL_CB(user_address.end) - brk_region - brk_max_size);
+                rand = ALIGN_DOWN(rand);
+
+                if (brk_region + rand + brk_max_size >= PAL_CB(user_address.end))
+                    continue;
+#else
+                /* Without randomization there is no point to retry here */
+                if (brk_region + rand + brk_max_size >= PAL_CB(user_address.end))
+                    break;
+#endif
+
+                struct shim_vma_val vma;
+                if (lookup_overlap_vma(brk_region + rand, brk_max_size, &vma) == -ENOENT) {
+                    /* Found a place for brk */
+                    brk_region += rand;
+                    brk_on_heap = false;
+                    break;
+                }
+#if !(ENABLE_ASLR == 1)
+                /* Without randomization, try memory directly after the overlapping block */
+                brk_region = vma.addr + vma.length;
+#endif
             }
-
-            brk_region = vma.addr + vma.length;
         }
+    }
 
+    if (brk_on_heap) {
+        brk_region = bkeep_unmapped_heap(brk_max_size, PROT_READ|PROT_WRITE,
+                                         flags|VMA_UNMAPPED, NULL, 0, "brk");
+        if (!brk_region) {
+            return -ENOMEM;
+        }
+    } else {
         /*
          * Create the bookkeeping before allocating the brk region.
          * The bookkeeping should never fail because we've already confirmed
@@ -106,11 +136,6 @@ int init_brk_region (void * brk_region)
         if (bkeep_mmap(brk_region, brk_max_size, PROT_READ|PROT_WRITE,
                        flags|VMA_UNMAPPED, NULL, 0, "brk") < 0)
             BUG();
-    } else {
-        brk_region = bkeep_unmapped_heap(brk_max_size, PROT_READ|PROT_WRITE,
-                                         flags|VMA_UNMAPPED, NULL, 0, "brk");
-        if (!brk_region)
-            return -ENOMEM;
     }
 
     void * end_brk_region = NULL;

diff --git a/LibOS/shim/src/sys/shim_exec.c b/LibOS/shim/src/sys/shim_exec.c
@@ -178,7 +178,10 @@ int shim_do_execve_rtld (struct shim_handle * hdl, const char ** argv,
     if ((ret = load_elf_object(cur_thread->exec, NULL, 0)) < 0)
         shim_terminate(ret);
 
-    init_brk_from_executable(cur_thread->exec);
+    ret = init_brk_from_executable(cur_thread->exec);
+    if (ret < 0)
+        return ret;
+
     load_elf_interp(cur_thread->exec);
 
     SAVE_PROFILE_INTERVAL(load_new_executable_for_exec);

diff --git a/Pal/lib/api.h b/Pal/lib/api.h
@@ -36,10 +36,16 @@ typedef ptrdiff_t ssize_t;
 #endif
 
 #ifndef MIN
-# define MIN(a, b) ((a) < (b) ? (a) : (b))
+#define MIN(a,b) \
+   ({ __typeof__(a) _a = (a); \
+      __typeof__(b) _b = (b); \
+      _a < _b ? _a : _b; })
 #endif
 #ifndef MAX
-# define MAX(a, b) ((a) > (b) ? (a) : (b))
+#define MAX(a,b) \
+   ({ __typeof__(a) _a = (a); \
+      __typeof__(b) _b = (b); \
+      _a > _b ? _a : _b; })
 #endif
 
 #define ALIGN_DOWN_PTR(ptr, size) \

diff --git a/Pal/src/host/FreeBSD/db_main.c b/Pal/src/host/FreeBSD/db_main.c
@@ -169,7 +169,7 @@ void _DkGetAvailableUserAddressRange (PAL_PTR * start, PAL_PTR * end)
         start_addr = (void *) ((unsigned long) start_addr << 1);
     }
 
-    *end   = (PAL_PTR) end_addr - USER_ADDRESS_RESERVED;
+    *end   = (PAL_PTR) end_addr;
     *start = (PAL_PTR) start_addr;
 }
 

diff --git a/Pal/src/host/FreeBSD/pal_freebsd_defs.h b/Pal/src/host/FreeBSD/pal_freebsd_defs.h
@@ -1,7 +1,6 @@
 #ifndef PAL_FREEBSD_DEFS_H
 #define PAL_FREEBSD_DEFS_H
 
-#define USER_ADDRESS_RESERVED   0x1000000
 #define USER_ADDRESS_LOWEST     0x10000
 #define USER_ADDRESS_HIGHEST	0x80000000
 

diff --git a/Pal/src/host/Linux-SGX/enclave_framework.c b/Pal/src/host/Linux-SGX/enclave_framework.c
@@ -332,7 +332,7 @@ int load_trusted_file (PAL_HANDLE file, sgx_stub_t ** stubptr,
          * store the data for checksum generation.
          */
 
-#define FILE_CHUNK_SIZE 1024
+#define FILE_CHUNK_SIZE 1024UL
 
         uint8_t small_chunk[FILE_CHUNK_SIZE]; /* Buffer for hashing */
         size_t chunk_offset = 0;

diff --git a/Pal/src/host/Linux-SGX/pal_linux_defs.h b/Pal/src/host/Linux-SGX/pal_linux_defs.h
@@ -12,7 +12,7 @@
 #define DEBUG_ECALL         0
 #define DEBUG_OCALL         0
 
-#define TRUSTED_STUB_SIZE   (PRESET_PAGESIZE * 4)
+#define TRUSTED_STUB_SIZE   (PRESET_PAGESIZE * 4UL)
 
 #define CACHE_FILE_STUBS    1
 

diff --git a/Pal/src/host/Linux/db_main.c b/Pal/src/host/Linux/db_main.c
@@ -172,7 +172,7 @@ void _DkGetAvailableUserAddressRange (PAL_PTR * start, PAL_PTR * end)
         start_addr = (void *) ((unsigned long) start_addr << 1);
     }
 
-    *end   = (PAL_PTR) end_addr - USER_ADDRESS_RESERVED;
+    *end   = (PAL_PTR) end_addr;
     *start = (PAL_PTR) start_addr;
 }
 

diff --git a/Pal/src/host/Linux/pal_linux_defs.h b/Pal/src/host/Linux/pal_linux_defs.h
@@ -1,7 +1,6 @@
 #ifndef PAL_LINUX_DEFS_H
 #define PAL_LINUX_DEFS_H
 
-#define USER_ADDRESS_RESERVED   0x100000000
 #define USER_ADDRESS_LOWEST     0x10000
 
 #define THREAD_STACK_SIZE       (PRESET_PAGESIZE * 2)