Fwd: Windows shmem fix: makes shared memory fully ASLR and DEP compliant (ea. cache zone, limit zone, etc.)

Maxim Dounin mdounin at mdounin.ru
Mon Apr 27 01:25:30 UTC 2015


Hello!

On Fri, Apr 24, 2015 at 01:21:41AM +0200, Sergey Brester wrote:

> Hello, 
> 
> > There are lots of style problems which need cleanup.
> 
> The newer, "nginx-style" compliant version of changeset (shmem
> fix2.patch) was already posted to nginx-devel (Thx, Filipe DA SILVA).
> You will find it also on under
> https://github.com/sebres/nginx/commit/e7c149f1ad76b9d850fb59ecc479d4a658c13e04
> [4]. 

It still needs cleanup, but I don't think it worth detailed 
comments as there are more important things to be addressed.

> > Such things belong to ngx_win32_init.c. It may be also good 
> > enough to use ngx_pagesize, which is already set there.
> 
>  Agree, but some (little) things can reside in same module, where these
> are used (noted as todo). 
> 
> > This probably should be somewhere at ngx_win32_config.h.
> 
> Imho, belong definitelly in shmem module. But don't want to fight about
> it :) 

The question here is a balance between code locality and 
visibility of platform-dependent constants for modifications.  E.g., 
we've recently added 64-bit constants into ngx_win32_config.h 
to allow compilation of 64-bit windows binaries - and I'm 
perfectly sure shmem code would have been forgotten assuming it 
used just magic addresses as in your patch.

May be some constants with appropriate comments at the top of the 
ngx_shmem.c file will be a good compromise solution.

> > It might be better idea to save the address at the end of the 
> > memory block allocated...
> 
> And for example stop to work of other new worker (when unfortunately
> overwriting of it occurs). That brings at once many problem and
> restrictions later (for example more difficult implementing of shared
> area resizing, etc). And for what? To save 2 line code for decrement
> addr pointer in ngx_shm_free? 

It won't be overwritten as long as you allocate additional bytes 
for it (as of now) and shm->size is not incremented.  And yes, 
saving 2 lines of code is important - especially if it happens 
because the whole approach becomes simplier.

Anyway, I think that we should try to use the address which is 
already present, see other comments.  This will also avoid 
unneeded restrictions on mappings where pointers are not stored.

> > Have you tried using an arbitrary addresses (with subsequent 
> > remap using proper base address for already existing mappings)? 
> > Does is work noticeably worse?
> 
> I've fought against ASLR since 2008 resp. x64 was released and it was
> essential. This specific solution is extensively tested and runs in
> production also without any problems (not only in nginx). 

So the answer is "no", right?  Either way, I tried it myself, and 
it seems to be noticeably worse - in my tests in most cases it 
can't use the address from the master process in workers due to 
conflicts with other allocations.  When combined with base address 
selection, it seems to be much more stable.

Below is a patch which takes into account above comments, and 
also fixes a problem observed with your patch on configuration 
reloads, e.g., when something like:

    proxy_cache_path cache1 keys_zone=cache1:10m;

is changed to

    proxy_cache_path cache0 keys_zone=cache0:10m;
    proxy_cache_path cache1 keys_zone=cache1:10m;

(i.e., when mappings are created in master and worker processes in 
a different order, resulting in conflicts between a mapping we are 
trying to create at some base address with a mapping already 
remapped to this address).

Review, comments and testing appreciated.

# HG changeset patch
# User Maxim Dounin <mdounin at mdounin.ru>
# Date 1430097438 -10800
#      Mon Apr 27 04:17:18 2015 +0300
# Node ID 89cd9c63c58e5d7c474d317f83584779e2342ee3
# Parent  859ce1c41f642c39d2db9741bdd305f3ee6507f5
Win32: shared memory base addresses and remapping.

Two mechanisms are implemented to make it possible to store pointers
in shared memory on Windows, in particular on Windows Vista and later
versions with ASLR:

- The ngx_shm_remap() function added to allow remapping of a shared memory
  zone to the address originally used for it in the master process.  While
  important, it doesn't solve the problem by itself as in many cases it's
  not possible to use the address because of conflicts with other
  allocations.

- We now create mappings at the same address in all processes by starting
  mappings at predefined addresses normally unused by newborn processes.

These two mechanisms combined allow to use shared memory on Windows
almost without problems, including reloads.

Based on the patch by Sergey Brester:
http://mailman.nginx.org/pipermail/nginx-devel/2015-April/006836.html

diff -r 859ce1c41f64 -r 89cd9c63c58e src/core/ngx_cycle.c
--- a/src/core/ngx_cycle.c	Mon Apr 27 03:44:30 2015 +0300
+++ b/src/core/ngx_cycle.c	Mon Apr 27 04:17:18 2015 +0300
@@ -863,6 +863,22 @@ ngx_init_zone_pool(ngx_cycle_t *cycle, n
             return NGX_OK;
         }
 
+#if (NGX_WIN32)
+
+        /* remap at the required address */
+
+        if (ngx_shm_remap(&zn->shm, sp->addr) != NGX_OK) {
+            return NGX_ERROR;
+        }
+
+        sp = (ngx_slab_pool_t *) zn->shm.addr;
+
+        if (sp == sp->addr) {
+            return NGX_OK;
+        }
+
+#endif
+
         ngx_log_error(NGX_LOG_EMERG, cycle->log, 0,
                       "shared zone \"%V\" has no equal addresses: %p vs %p",
                       &zn->shm.name, sp->addr, sp);
diff -r 859ce1c41f64 -r 89cd9c63c58e src/os/win32/ngx_shmem.c
--- a/src/os/win32/ngx_shmem.c	Mon Apr 27 03:44:30 2015 +0300
+++ b/src/os/win32/ngx_shmem.c	Mon Apr 27 04:17:18 2015 +0300
@@ -9,11 +9,44 @@
 #include <ngx_core.h>
 
 
+/*
+ * Base addresses selected by system for shared memory mappings are likely
+ * to be different on Windows Vista and later versions due to address space
+ * layout randomization.  This is however incompatible with storing absolute
+ * addresses within the shared memory.
+ *
+ * To make it possible to store absolute addresses we create mappings
+ * at the same address in all processes by starting mappings at predefined
+ * addresses.  The addresses was selected somewhat randomly in order to
+ * minimize the probability that some other library doing something similar 
+ * conflicts with us.  The addresses are from the following typically free
+ * blocks:
+ *
+ * - 0x10000000 .. 0x70000000 (about 1.8 GB in total) on 32-bit platforms
+ * - 0x000000007fff0000 .. 0x000007f68e8b0000 (about 8 TB) on 64-bit platforms
+ *
+ * Additionally, we allow to change the mapping address once it was detected
+ * to be different from one originally used.  This is needed to support
+ * reconfiguration.
+ */
+
+
+#ifdef _WIN64
+#define NGX_SHMEM_BASE  0x0000047047e00000
+#else
+#define NGX_SHMEM_BASE  0x2efe0000
+#endif
+
+
+ngx_uint_t  ngx_allocation_granularity;
+
+
 ngx_int_t
 ngx_shm_alloc(ngx_shm_t *shm)
 {
-    u_char    *name;
-    uint64_t   size;
+    u_char         *name;
+    uint64_t        size;
+    static u_char  *base = (u_char *) NGX_SHMEM_BASE;
 
     name = ngx_alloc(shm->name.len + 2 + NGX_INT32_LEN, shm->log);
     if (name == NULL) {
@@ -46,6 +79,27 @@ ngx_shm_alloc(ngx_shm_t *shm)
         shm->exists = 1;
     }
 
+    shm->addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, base);
+
+    if (shm->addr != NULL) {
+        base += ngx_align(size, ngx_allocation_granularity);
+        return NGX_OK;
+    }
+
+    ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, ngx_errno,
+                   "MapViewOfFileEx(%uz, %p) of file mapping \"%V\" failed, "
+                   "retry without a base address",
+                   shm->size, base, &shm->name);
+
+    /*
+     * Order of shared memory zones may be different in the master process
+     * and worker processes after reconfiguration.  As a result, the above
+     * may fail due to a conflict with a previously created mapping remapped
+     * to a different address.  Additionally, there may be a conflict with
+     * some other uses of the memory.  In this case we retry without a base
+     * address to let the system assign the address itself.
+     */
+
     shm->addr = MapViewOfFile(shm->handle, FILE_MAP_WRITE, 0, 0, 0);
 
     if (shm->addr != NULL) {
@@ -66,6 +120,30 @@ ngx_shm_alloc(ngx_shm_t *shm)
 }
 
 
+ngx_int_t
+ngx_shm_remap(ngx_shm_t *shm, u_char *addr)
+{
+    if (UnmapViewOfFile(shm->addr) == 0) {
+        ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
+                      "UnmapViewOfFile(%p) of file mapping \"%V\" failed",
+                      shm->addr, &shm->name);
+        return NGX_ERROR;
+    }
+
+    shm->addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, addr);
+
+    if (shm->addr != NULL) {
+        return NGX_OK;
+    }
+
+    ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno,
+                  "MapViewOfFileEx(%uz, %p) of file mapping \"%V\" failed",
+                  shm->size, addr, &shm->name);
+
+    return NGX_ERROR;
+}
+
+
 void
 ngx_shm_free(ngx_shm_t *shm)
 {
diff -r 859ce1c41f64 -r 89cd9c63c58e src/os/win32/ngx_shmem.h
--- a/src/os/win32/ngx_shmem.h	Mon Apr 27 03:44:30 2015 +0300
+++ b/src/os/win32/ngx_shmem.h	Mon Apr 27 04:17:18 2015 +0300
@@ -24,7 +24,10 @@ typedef struct {
 
 
 ngx_int_t ngx_shm_alloc(ngx_shm_t *shm);
+ngx_int_t ngx_shm_remap(ngx_shm_t *shm, u_char *addr);
 void ngx_shm_free(ngx_shm_t *shm);
 
+extern ngx_uint_t  ngx_allocation_granularity;
+
 
 #endif /* _NGX_SHMEM_H_INCLUDED_ */
diff -r 859ce1c41f64 -r 89cd9c63c58e src/os/win32/ngx_win32_init.c
--- a/src/os/win32/ngx_win32_init.c	Mon Apr 27 03:44:30 2015 +0300
+++ b/src/os/win32/ngx_win32_init.c	Mon Apr 27 04:17:18 2015 +0300
@@ -118,6 +118,7 @@ ngx_os_init(ngx_log_t *log)
 
     GetSystemInfo(&si);
     ngx_pagesize = si.dwPageSize;
+    ngx_allocation_granularity = si.dwAllocationGranularity;
     ngx_ncpu = si.dwNumberOfProcessors;
     ngx_cacheline_size = NGX_CPU_CACHE_LINE;
 

-- 
Maxim Dounin
http://nginx.org/



More information about the nginx-devel mailing list