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

Thu Apr 23 23:21:41 UTC 2015

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

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

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

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

Regards,
sebres. 

Am 22.04.2015 18:29, schrieb Maxim Dounin: 

> Hello!
> 
> On Wed, Apr 22, 2015 at 09:45:45AM +0200, Sergey Brester wrote:
> 
>> enclosed you will find an attached changeset, that fixes a ASLR/DEP problem on windows platforms (example Win 7/2008 x64). To find shared addr offset with ASLR, we have successful used the same resp. similar solution on various open source projects (ex.: postgresql etc.). Also nginx with suggested patch works fine over 5 months in production on several machines (Win-2k8-R2).
> 
> Interesting, thanks, though the patch certainly needs some 
> cleanup. See below for some comments.
> 
>> BTW(1): I have interest to fix a problem with multiple workers under windows (ex.: http://forum.nginx.org/read.php?2,188714,188714#msg-188714 [1] [1]). @Maxim Dounin: can you possibly give me more information, what you mean here, resp. what it currently depends on (see http://forum.nginx.org/read.php?2,188714,212568#msg-212568 [2] [2]).
> 
> The most obvious problem with multiple workers on Windows is 
> listening sockets. As of now, if you start multiple workers on 
> windows, nginx will open multiple listening sockets - one in each 
> worker process. These are independant sockets, each opened with 
> SO_REUSEADDR, and only one of these sockets will be able to accept 
> connections.
> 
> Possible solution to the problem would be to pass listening sockets 
> from a master process to worker processes via handle inheritance 
> as available in Windows:
> 
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463(v=vs.85).aspx [3]
> 
> [...]
> 
>> # HG changeset patch # User Serg G. Brester (sebres) <serg.brester at sebres.de> # Date 1429643760 -7200 # Tue Apr 21 21:16:00 2015 +0200 # Branch sb-mod # Node ID f759f7084204bb1f3c2b0a8023a8f03a81da9a0b # Parent 1bdfceda86a99a4dc99934181d2f9e2632003ca8 Windows shmem fix: makes shared memory fully ASLR and DEP compliant (ea. cache zone, limit zone, etc.) diff -r 1bdfceda86a9 -r f759f7084204 src/os/win32/ngx_shmem.c --- a/src/os/win32/ngx_shmem.c Mon Apr 20 17:36:51 2015 +0300 +++ b/src/os/win32/ngx_shmem.c Tue Apr 21 21:16:00 2015 +0200 @@ -9,11 +9,30 @@ #include <ngx_core.h> +static volatile size_t g_addrbaseoffs = 0; + +static volatile int g_SystemInfo_init = 0; +SYSTEM_INFO g_SystemInfo; + +int ngx_shm_init_once() { + if (g_SystemInfo_init) + return 1; + g_SystemInfo_init = 1; + GetSystemInfo(&g_SystemInfo); + return 1; +}
> 
> Such things belong to ngx_win32_init.c. It may be also good 
> enough to use ngx_pagesize, which is already set there.
> 
>> + +/* ------------------- */ +
> 
> There are lots of style problems which need cleanup.
> 
>> ngx_int_t ngx_shm_alloc(ngx_shm_t *shm) { u_char *name; uint64_t size; + u_char *addr; + u_char *addrbase; + + ngx_shm_init_once(); name = ngx_alloc(shm->name.len + 2 + NGX_INT32_LEN, shm->log); if (name == NULL) { @@ -26,6 +45,9 @@ size = shm->size; + // increase for base address, will be saved inside shared mem : + size += sizeof(addr); +
> 
> As of now, uses of ngx_shm_alloc() already contain address in the 
> shared memory region where addresses are important. It may worth 
> to try to use it, if possible (may be with some additional call to 
> redo the mapping with the address specified).
> 
>> shm->handle = CreateFileMapping(INVALID_HANDLE_VALUE, NULL, PAGE_READWRITE, (u_long) (size >> 32), (u_long) (size & 0xffffffff), @@ -34,7 +56,7 @@ if (shm->handle == NULL) { ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno, "CreateFileMapping(%uz, %s) failed", - shm->size, name); + size, name); ngx_free(name); return NGX_ERROR; @@ -42,14 +64,79 @@ ngx_free(name); + shm->exists = 0;
> 
> This looks like an unneeded change. The exists flag is set to 0 
> in ngx_shared_memory_add(), when ngx_shm_t is allocated and 
> initialized.
> 
>> if (ngx_errno == ERROR_ALREADY_EXISTS) { shm->exists = 1; } - shm->addr = MapViewOfFile(shm->handle, FILE_MAP_WRITE, 0, 0, 0); + /* + // Because of Win x64 since Vista/Win7 (always ASLR in kernel32), and nginx uses pointer + // inside this shared areas, we should use the same address for shared memory (Windows IPC) + + // Now we set preferred base to a hardcoded address that newborn processes never seem to be using (in available versions of Windows). + // The addresses was selected somewhat randomly in order to minimize the probability that some other library doing something similar + // conflicts with us. That is, using conspicuous addresses like 0x20000000 might not be good if someone else does it. + */ + #ifdef _WIN64 + /* + * There is typically a giant hole (almost 8TB): + * 00000000 7fff0000 + * ... + * 000007f6 8e8b0000 + */ + addrbase = (u_char*)0x0000047047e00000ULL; + #else + /* + * This is more dicey. However, even with ASLR there still + * seems to be a big hole: + *
10000000 + * ... + * 70000000 + */ + addrbase = (u_char*)0x2efe0000; + #endif
> 
> This probably should be somewhere at ngx_win32_config.h.
> 
>> - if (shm->addr != NULL) { + // add offset (corresponding all used shared) to preferred base: + addrbase += g_addrbaseoffs; + + addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, addrbase);
> 
> Have you tried using an arbitrary addresses (with subsequent 
> remap using proper base address for already existing mappings)? 
> Does is work noticeably worse?
> 
>> + + ngx_log_debug4(NGX_LOG_DEBUG_CORE, shm->log, 0, "map shared "%V" -> %p (base %p), size: %uz", &shm->name, addr, addrbase, size); + + if (addr != NULL) { + + // get allocated address if already exists: + if (shm->exists) { + // get base and realoc using it if different: + addrbase = *(u_char **)addr; + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared "%V" -> %p -> %p", &shm->name, addr, addrbase); + if (addrbase != addr) { + // free: + if (UnmapViewOfFile(addr) == 0) { + ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno, + "UnmapViewOfFile(%p) of file mapping "%V" failed", + addr, &shm->name); + } + // allocate again using base address: + addr = MapViewOfFileEx(shm->handle, FILE_MAP_WRITE, 0, 0, 0, + addrbase // lpBaseAddress + ); + } + } else { + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared "%V" -> %p (base %p)", &shm->name, addr, addrbase); + // save first allocated address as base for next caller : + *(u_char **)addr = addr;
> 
> It might be better idea to save the address at the end of the 
> memory block allocated, this will at least allow to avoid address 
> manipulation here and in ngx_shm_free(). Or use the address 
> stored in the memory block itself, see above.
> 
>> + } + + if (addr != NULL) { + // increase base offset (use proper alignment, obtain it from dwAllocationGranularity): + g_addrbaseoffs += (size + g_SystemInfo.dwAllocationGranularity) & (0xffffffff & ~(g_SystemInfo.dwAllocationGranularity-1)); + //ngx_log_debug2(NGX_LOG_DEBUG_CORE, shm->log, 0, "offset %ui, granularity %ui", g_addrbaseoffs, g_SystemInfo.dwAllocationGranularity); + shm->addr = addr + sizeof(addr); + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared alloc "%V" -> %p = %p", &shm->name, addr, shm->addr); return NGX_OK; + } + } ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno, @@ -65,14 +152,17 @@ return NGX_ERROR; } - void
> 
> Unrelated change (and a style bug).
> 
>> ngx_shm_free(ngx_shm_t *shm) { - if (UnmapViewOfFile(shm->addr) == 0) { + u_char *addr; + + addr = shm->addr - sizeof(addr); + ngx_log_debug3(NGX_LOG_DEBUG_CORE, shm->log, 0, "shared free "%V" -> %p = %p", &shm->name, addr, shm->addr); + if (UnmapViewOfFile(addr) == 0) { ngx_log_error(NGX_LOG_ALERT, shm->log, ngx_errno, "UnmapViewOfFile(%p) of file mapping "%V" failed", - shm->addr, &shm->name); + addr, &shm->name); } if (CloseHandle(shm->handle) == 0) {
> 
> See above, this change shouldn't be needed.

Links:
------
[1] http://forum.nginx.org/read.php?2,188714,188714#msg-188714
[2] http://forum.nginx.org/read.php?2,188714,212568#msg-212568
[3]
http://msdn.microsoft.com/en-us/library/windows/desktop/ms683463(v=vs.85).aspx
[4]
https://github.com/sebres/nginx/commit/e7c149f1ad76b9d850fb59ecc479d4a658c13e04
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.nginx.org/pipermail/nginx-devel/attachments/20150424/36240c47/attachment-0001.html>