From 9192d20f02592fae65494aa8d60c3bb504a56550 Mon Sep 17 00:00:00 2001 From: Willy Tarreau Date: Thu, 8 Dec 2022 17:47:59 +0100 Subject: [PATCH] MINOR: pools: make DEBUG_UAF a runtime setting Since the massive pools cleanup that happened in 2.6, the pools architecture was made quite more hierarchical and many alternate code blocks could be moved to runtime flags set by -dM. One of them had not been converted by then, DEBUG_UAF. It's not much more difficult actually, since it only acts on a pair of functions indirection on the slow path (OS-level allocator) and a default setting for the cache activation. This patch adds the "uaf" setting to the options permitted in -dM so that it now becomes possible to set or unset UAF at boot time without recompiling. This is particularly convenient, because every 3 months on average, developers ask a user to recompile haproxy with DEBUG_UAF to understand a bug. Now it will not be needed anymore, instead the user will only have to disable pools and enable uaf using -dMuaf. Note that -dMuaf only disables previously enabled pools, but it remains possible to re-enable caching by specifying the cache after, like -dMuaf,cache. A few tests with this mode show that it can be an interesting combination which catches significantly less UAF but will do so with much less overhead, so it might be compatible with some high-traffic deployments. The change is very small and isolated. It could be helpful to backport this at least to 2.7 once confirmed not to cause build issues on exotic systems, and even to 2.6 a bit later as this has proven to be useful over time, and could be even more if it did not require a rebuild. If a backport is desired, the following patches are needed as well: CLEANUP: pools: move the write before free to the uaf-only function CLEANUP: pool: only include pool-os from pool.c not pool.h REORG: pool: move all the OS specific code to pool-os.h CLEANUP: pools: get rid of CONFIG_HAP_POOLS DEBUG: pool: show a few examples in -dMhelp --- doc/internals/api/pools.txt | 11 +++++++---- include/haproxy/pool-t.h | 1 + src/pool.c | 30 ++++++++++++++++++++---------- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/doc/internals/api/pools.txt b/doc/internals/api/pools.txt index 315d5c4b23..4023dc3168 100644 --- a/doc/internals/api/pools.txt +++ b/doc/internals/api/pools.txt @@ -75,9 +75,10 @@ The pools architecture is selected at build time. The main options are: accesses. Released objects are instantly freed using munmap() so that any immediate subsequent access to the memory area crashes the process if the area had not been reallocated yet. This mode can be enabled at build time - by setting DEBUG_UAF. It tends to consume a lot of memory and not to scale - at all with concurrent calls, that tends to make the system stall. The - watchdog may even trigger on some slow allocations. + by setting DEBUG_UAF, or at run time by disabling pools and enabling UAF + with "-dMuaf". It tends to consume a lot of memory and not to scale at all + with concurrent calls, that tends to make the system stall. The watchdog + may even trigger on some slow allocations. There are no more provisions for running with a shared pool but no thread-local cache: the shared pool's main goal is to compensate for the expensive calls to @@ -511,7 +512,9 @@ DEBUG_UAF through mmap() and munmap(). The memory usage significantly inflates and the performance degrades, but this allows to detect a lot of use-after-free conditions by crashing the program at the first abnormal - access. This should not be used in production. + access. This should not be used in production. It corresponds to + boot-time options "-dMuaf". Caching is disabled but may be re-enabled + using "-dMcache". DEBUG_POOL_INTEGRITY When enabled, objects picked from the cache are checked for corruption diff --git a/include/haproxy/pool-t.h b/include/haproxy/pool-t.h index 460173c6fb..523bbaf9af 100644 --- a/include/haproxy/pool-t.h +++ b/include/haproxy/pool-t.h @@ -50,6 +50,7 @@ #define POOL_DBG_CALLER 0x00000040 // trace last caller's location #define POOL_DBG_TAG 0x00000080 // place a tag at the end of the area #define POOL_DBG_POISON 0x00000100 // poison memory area on pool_alloc() +#define POOL_DBG_UAF 0x00000200 // enable use-after-free protection /* This is the head of a thread-local cache */ diff --git a/src/pool.c b/src/pool.c index ba5fe51652..e225d2144d 100644 --- a/src/pool.c +++ b/src/pool.c @@ -60,6 +60,9 @@ uint pool_debugging __read_mostly = /* set of POOL_DBG_* flags */ #endif #if defined(DEBUG_MEMORY_POOLS) POOL_DBG_TAG | +#endif +#if defined(DEBUG_UAF) + POOL_DBG_UAF | #endif 0; @@ -79,6 +82,7 @@ static const struct { { POOL_DBG_CALLER, "caller", "no-caller", "save caller information in cache" }, { POOL_DBG_TAG, "tag", "no-tag", "add tag at end of allocated objects" }, { POOL_DBG_POISON, "poison", "no-poison", "poison newly allocated objects" }, + { POOL_DBG_UAF, "uaf", "no-uaf", "enable use-after-free checks (slow)" }, { 0 /* end */ } }; @@ -336,11 +340,11 @@ void *pool_get_from_os(struct pool_head *pool) { if (!pool->limit || pool->allocated < pool->limit) { void *ptr; -#ifdef DEBUG_UAF - ptr = pool_alloc_area_uaf(pool->alloc_sz); -#else - ptr = pool_alloc_area(pool->alloc_sz); -#endif + + if (pool_debugging & POOL_DBG_UAF) + ptr = pool_alloc_area_uaf(pool->alloc_sz); + else + ptr = pool_alloc_area(pool->alloc_sz); if (ptr) { _HA_ATOMIC_INC(&pool->allocated); return ptr; @@ -357,11 +361,10 @@ void *pool_get_from_os(struct pool_head *pool) */ void pool_put_to_os(struct pool_head *pool, void *ptr) { -#ifdef DEBUG_UAF - pool_free_area_uaf(ptr, pool->alloc_sz); -#else - pool_free_area(ptr, pool->alloc_sz); -#endif + if (pool_debugging & POOL_DBG_UAF) + pool_free_area_uaf(ptr, pool->alloc_sz); + else + pool_free_area(ptr, pool->alloc_sz); _HA_ATOMIC_DEC(&pool->allocated); } @@ -1061,6 +1064,8 @@ int pool_parse_debugging(const char *str, char **err) " Detect out-of-bound corruptions: -dMno-merge,tag\n" " Detect post-free cache corruptions: -dMno-merge,cold-first,integrity,caller\n" " Detect all cache corruptions: -dMno-merge,cold-first,integrity,tag,caller\n" + " Detect UAF (disables cache, very slow): -dMuaf\n" + " Detect post-cache UAF: -dMuaf,cache,no-merge,cold-first,integrity,tag,caller\n" " Detect post-free cache corruptions: -dMno-merge,cold-first,integrity,caller\n", *err); return -1; @@ -1069,6 +1074,11 @@ int pool_parse_debugging(const char *str, char **err) for (v = 0; dbg_options[v].flg; v++) { if (isteq(feat, ist(dbg_options[v].set))) { new_dbg |= dbg_options[v].flg; + /* UAF implicitly disables caching, but it's + * still possible to forcefully re-enable it. + */ + if (dbg_options[v].flg == POOL_DBG_UAF) + new_dbg |= POOL_DBG_NO_CACHE; break; } else if (isteq(feat, ist(dbg_options[v].clr))) {