From a8d2054913c47f5cca48d97a60de8a69535745b7 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Fri, 13 May 2022 08:39:02 -0700 Subject: [PATCH 1/4] Patch eap.o memory leak WiFi Enterprise option can leak up to 3 allocations per connect/disconnect cycle: anonymous Identity, password, and some unidentified allocation. This solution patches eap.o from libwpa2 to call a special 2 part wrapper instead of vPortFree for cleanup. Corrected typos and adjusted tabs in script. Added script eval_fix_sdks.sh to aid in evaluating similarity between patch sections of .o files being patched across different SDKs. --- cores/esp8266/coredecls.h | 1 + cores/esp8266/heap.cpp | 8 +- cores/esp8266/wpa2_eap_patch.cpp | 120 ++++++++++++++++++++++ tools/sdk/lib/NONOSDK221/libwpa2.a | Bin 475218 -> 475218 bytes tools/sdk/lib/NONOSDK22x_190313/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_190703/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_191024/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_191105/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_191122/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK3V0/libwpa2.a | Bin 475358 -> 475358 bytes tools/sdk/lib/eval_fix_sdks.sh | 103 +++++++++++++++++++ tools/sdk/lib/fix_sdk_libs.sh | 80 +++++++++++++-- 12 files changed, 302 insertions(+), 10 deletions(-) create mode 100644 cores/esp8266/wpa2_eap_patch.cpp create mode 100755 tools/sdk/lib/eval_fix_sdks.sh diff --git a/cores/esp8266/coredecls.h b/cores/esp8266/coredecls.h index e06dc6029a..1e2776d71b 100644 --- a/cores/esp8266/coredecls.h +++ b/cores/esp8266/coredecls.h @@ -21,6 +21,7 @@ void esp_schedule(); void esp_yield(); void tune_timeshift64 (uint64_t now_us); void disable_extra4k_at_link_time (void) __attribute__((noinline)); +void enable_wifi_enterprise_patch(void) __attribute__((noinline)); bool sntp_set_timezone_in_seconds(int32_t timezone); void __disableWiFiAtBootTime (void) __attribute__((noinline)); void __real_system_restart_local() __attribute__((noreturn)); diff --git a/cores/esp8266/heap.cpp b/cores/esp8266/heap.cpp index 67f785c712..78011ed248 100644 --- a/cores/esp8266/heap.cpp +++ b/cores/esp8266/heap.cpp @@ -5,7 +5,13 @@ #include #include "umm_malloc/umm_malloc.h" -extern "C" size_t umm_umul_sat(const size_t a, const size_t b);; +extern "C" size_t umm_umul_sat(const size_t a, const size_t b); + +// z2EapFree: See wpa2_eap_patch.cpp for details +extern "C" void z2EapFree(void *ptr, const char* file, int line) __attribute__((weak, alias("vPortFree"), nothrow)); +// I don't understand all the compiler noise around this alias. +// Adding "__attribute__ ((nothrow))" seems to resolve the issue. +// This may be relevant: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81824 // Need FORCE_ALWAYS_INLINE to put HeapSelect class constructor/deconstructor in IRAM #define FORCE_ALWAYS_INLINE_HEAP_SELECT diff --git a/cores/esp8266/wpa2_eap_patch.cpp b/cores/esp8266/wpa2_eap_patch.cpp new file mode 100644 index 0000000000..95c74dca19 --- /dev/null +++ b/cores/esp8266/wpa2_eap_patch.cpp @@ -0,0 +1,120 @@ +/* + * To implement this patch, the SDK module `eap.o` from archive `libwpa2.a` must + * be patched to call `z2EapFree` instead of `vPortFree`. This limits extending + * the execution time of vPortFree to that module only. Not impacting other + * modules. + * + */ + +#include +#include +#include +#include "coredecls.h" +#if 0 +#include "esp8266_undocumented.h" +#define DEBUG_PRINTF ets_uart_printf +#else +#define DEBUG_PRINTF(...) +#endif + +extern "C" { + +// extern "C" void z2EapFree(void *ptr, const char* file, int line) __attribute__((weak, alias("vPortFree"))); + +/* + * Limited 2-part wrapper for vPortFree calls made in SDK module `eap.o` from + * archive `libwpa2.a`. + * + * vPortFree calls from eap.o are monitored for calls from line 799. This is + * the location of the memory leak. At entry register a12 contains the structure + * address which has the addresses of the allocations that will be leaked. + * + * Part 1 of this wrapper, z2EapFree, appends the value of register a12 as a + * 4th argument to part2 of this wrapper, patch_wpa2_eap_vPortFree_a12(). Which + * in turn checks and frees the additional allocations, that would have been + * lost. + * + * extern "C" z2EapFree(void*); + */ + +/* + * Part 1 of Limited vPortFree Wrapper + */ +asm( + // ".section .iram.text.z2EapFree,\"ax\",@progbits\n\t" + // Since all the possible callers in eap.o are in sections starting with + // .text and not .iram.text we should be safe putting these wrappers in .text. + ".section .text.z2EapFree,\"ax\",@progbits\n\t" + ".literal_position\n\t" + ".literal .patch_wpa2_eap_vPortFree_a12, patch_wpa2_eap_vPortFree_a12\n\t" + ".align 4\n\t" + ".global z2EapFree\n\t" + ".type z2EapFree, @function\n\t" + "\n" +"z2EapFree:\n\t" + "addi a1, a1, -16\n\t" + "s32i a0, a1, 0\n\t" + "mov a5, a12\n\t" + "l32r a0, .patch_wpa2_eap_vPortFree_a12\n\t" + "callx0 a0\n\t" + "l32i a0, a1, 0\n\t" + "addi a1, a1, 16\n\t" + "ret\n\t" + ".size z2EapFree, .-z2EapFree\n\t" +); + +/* + * While some insight can be gained from the ESP32 repo for this structure. + * It does not match exactly. This alternate structure focuses on correct offset + * rather than trying to exactly reconstruct the original labels. + */ +struct StateMachine { // size 200 bytes + void* beforeConfig[16]; + void* config[26]; + // 0 - mov a2, a12, 64 // username / Identity + // 1 - mov a2, a12, 68 + // 2 - mov a2, a12, 72 // anonymous Identity + // 3 - mov a2, a12, 76 + // 4 - mov a2, a12, 80 // password + // 21 - mov a2, a12, 148 // ?? + void* afterConfig[8]; +}; + +/* + * Part 2 of Limited vPortFree Wrapper + * + * Presently, all SDKs have the same memory leaks in the same module at the + * same line. + */ +void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a12) { + if (799 == line) { + struct StateMachine* sm = (struct StateMachine*)a12; + if (ptr == sm->config[0]) { + // Fix leaky frunction - eap.o only frees one out of 4 config items + // finish the other 3 first + vPortFree(sm->config[2], file, line); + vPortFree(sm->config[4], file, line); + vPortFree(sm->config[21], file, line); + // ptr is sm->config[0], let fall through handle it + } + DEBUG_PRINTF("\nz2EapFree/vPortFree patch working\n"); + } + vPortFree(ptr, file, line); +} + +}; + +/* + * This will minimize code space for non-wifi enterprise sketches which do not + * need the patch and disable_extra4k_at_link_time(). + */ +void enable_wifi_enterprise_patch(void) { + /* + * Calling this from setup or anywhere ensures that the patch code is + * included in the build. + * + * Also, WiFi Enterprise uses a lot of system stack space and may crash + * unless we: + */ + disable_extra4k_at_link_time(); +} diff --git a/tools/sdk/lib/NONOSDK221/libwpa2.a b/tools/sdk/lib/NONOSDK221/libwpa2.a index 1f7aabb6881af8cbac966e4042736747404cebf4..f2b2178fc036f8fc0a265767f196e8d66e95a318 100644 GIT binary patch delta 104 zcmccAAbY7nc7i0QnW>S9shNR=S)m-Go7*orCvd733% delta 82 zcmccAAbY7nc7o*e`|PaTjVfDJ813Vy_kU%U-`*6@_?w&Q0K@kBY{vglY-IuYMI~;N i@396?-+PRav;Dy_Mj&PaVrC#_0bS9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_190703/libwpa2.a b/tools/sdk/lib/NONOSDK22x_190703/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_191024/libwpa2.a b/tools/sdk/lib/NONOSDK22x_191024/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_191105/libwpa2.a b/tools/sdk/lib/NONOSDK22x_191105/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_191122/libwpa2.a b/tools/sdk/lib/NONOSDK22x_191122/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK3V0/libwpa2.a b/tools/sdk/lib/NONOSDK3V0/libwpa2.a index b868608a143d1eea9745d5044979f45bc8adf4d9..0826b45e7fb87b53d011d971fbc9dcedff052070 100644 GIT binary patch delta 99 zcmccDD0{C_c7i0QnW>S9shNR=S)V{D%*0h{e1)&Kwi diff --git a/tools/sdk/lib/eval_fix_sdks.sh b/tools/sdk/lib/eval_fix_sdks.sh new file mode 100755 index 0000000000..333f0ebbc6 --- /dev/null +++ b/tools/sdk/lib/eval_fix_sdks.sh @@ -0,0 +1,103 @@ +#!/bin/bash +# set -e + +add_path_ifexist() { + if [[ -d $1 ]]; then + export PATH=$( realpath $1 ):$PATH + return 0 + fi + return 1 +} + +if ! which xtensa-lx106-elf-ar | grep "tools/xtensa-lx106-elf/bin" >>/dev/null; then + add_path_ifexist "../../../xtensa-lx106-elf/bin" || add_path_ifexist "../../xtensa-lx106-elf/bin" +fi + + +list_sdks() { + cat <new.txt + if [[ -f old.txt ]]; then + echo "eap_peer_config_deinit: diff $prev_sdk $sdk" + diff old.txt new.txt + echo "" + fi + mv new.txt old.txt + prev_sdk=${sdk} + done + + unset prev_sdk + for sdk in `list_sdks`; do + unasm -j ".text.wpa2_sm_rx_eapol" ${sdk}/eap.o >new.txt + if [[ -f old2.txt ]]; then + echo "wpa2_sm_rx_eapol: diff $prev_sdk $sdk" + diff old2.txt new.txt + echo "" + fi + mv new.txt old2.txt + prev_sdk=${sdk} + done + + # Find offsets for patching vPortFree with z2EapFree + for sdk in `list_sdks`; do + echo -en "\n${sdk}/eap.o:\n " + grep --byte-offset --only-matching --text vPortFree ${sdk}/eap.o + done + + cleanup +} + + +patch_all() { + for sdk in `list_sdks`; do + pushd $sdk + ../fix_sdk_libs.sh + popd + done +} + +# analyze +patch_all +exit 0 diff --git a/tools/sdk/lib/fix_sdk_libs.sh b/tools/sdk/lib/fix_sdk_libs.sh index b6aea12752..c94e9492d0 100755 --- a/tools/sdk/lib/fix_sdk_libs.sh +++ b/tools/sdk/lib/fix_sdk_libs.sh @@ -1,36 +1,98 @@ #!/bin/bash set -e -export PATH=../../xtensa-lx106-elf/bin:$PATH +add_path_ifexist() { + if [[ -d $1 ]]; then + export PATH=$( realpath $1 ):$PATH + return 0 + fi + return 1 +} + +if ! which xtensa-lx106-elf-ar | grep "tools/xtensa-lx106-elf/bin" >>/dev/null; then + add_path_ifexist "../../../xtensa-lx106-elf/bin" || add_path_ifexist "../../xtensa-lx106-elf/bin" +fi +WORK_SPACE=${PWD} + VERSION=$(basename ${PWD}) addSymbol_system_func1() { - ADDRESS=$1 - xtensa-lx106-elf-objcopy --add-symbol system_func1=.irom0.text:${ADDRESS},function,global user_interface.o + if ! xtensa-lx106-elf-nm user_interface.o | grep -q " T system_func1"; then # Don't add symbol if it already exists + ADDRESS=$1 + xtensa-lx106-elf-objcopy --add-symbol system_func1=.irom0.text:${ADDRESS},function,global user_interface.o + fi } +patchFile() { + FILE=$1 + ADDRESS=$2 # DO NOT PASS AS HEX! + LENGTH=$3 # DO NOT PASS AS HEX! + EXPECTED=$4 + REPLACEWITH=$5 + if [[ "$(dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0)" = "$EXPECTED" ]]; then + echo "Patching $VERSION $1 ..." + echo $5 | base64 -d | dd of=$FILE bs=1 count=$LENGTH seek=$ADDRESS conv=notrunc + elif ! [[ "$(dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0)" = "$REPLACEWITH" ]]; then + echo "PATCH FAILED!" + echo "dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0" + dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | hexdump -C + dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0 + echo "" + exit 1 + fi +} + +# # xtensa-lx106-elf-ar x libwpa2.a eap.o +if [[ "--shell" == "$1" ]]; then + # need to poke around a bit + bash --rcfile <(echo ". ~/.bashrc; cd ${WORK_SPACE}") + exit 0 +fi + +if [[ ! -f libmain.a ]]; then + echo -e "\n\n*** Archive libmain.a is missing ***\n\n" + exit 1 +fi # Remove mem_manager.o from libmain.a to use custom heap implementation, # and time.o to fix redefinition of time-related functions: xtensa-lx106-elf-ar d libmain.a mem_manager.o xtensa-lx106-elf-ar d libmain.a time.o +# Patch WPA2-Enterprise double-free +xtensa-lx106-elf-ar x libwpa2.a eap.o +eapcs=$(sha256sum eap.o | awk '{print $1}') + # Rename `hostname` and `default_hostname` symbols: xtensa-lx106-elf-ar x libmain.a eagle_lwip_if.o user_interface.o -xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname user_interface.o -xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname eagle_lwip_if.o -xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname user_interface.o -xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname eagle_lwip_if.o +lwipcs=$(sha256sum eagle_lwip_if.o | awk '{print $1}') +uics=$(sha256sum user_interface.o | awk '{print $1}') +xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname user_interface.o +xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname eagle_lwip_if.o +xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname user_interface.o +xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname eagle_lwip_if.o + if [[ ${VERSION} == "NONOSDK221" ]]; then addSymbol_system_func1 "0x60" + patchFile "eap.o" "3055" "2" "wAA=" "8CA=" # WPA2-Enterprise patch which replaces a double-free with nop, see #8082 + patchFile "eap.o" "26352" "9" "dlBvcnRGcmVl" "ejJFYXBGcmVl" # special vPortFree to recover leaked memory elif [[ ${VERSION} == "NONOSDK22x"* ]]; then addSymbol_system_func1 "0x54" + patchFile "eap.o" "3059" "2" "wAA=" "8CA=" # WPA2-Enterprise patch which replaces a double-free with nop, see #8082 + patchFile "eap.o" "26356" "9" "dlBvcnRGcmVl" "ejJFYXBGcmVl" # special vPortFree to recover leaked memory elif [[ ${VERSION} == "NONOSDK3"* ]]; then addSymbol_system_func1 "0x60" + patchFile "eap.o" "3059" "2" "wAA=" "8CA=" # WPA2-Enterprise patch which replaces a double-free with nop, see #8082 + patchFile "eap.o" "26356" "9" "dlBvcnRGcmVl" "ejJFYXBGcmVl" # special vPortFree to recover leaked memory else echo "WARN: Unknown address for system_func1() called by system_restart_local()" fi -xtensa-lx106-elf-ar r libmain.a eagle_lwip_if.o user_interface.o -rm -f eagle_lwip_if.o user_interface.o +if [[ $(sha256sum eap.o | awk '{print $1}') != $eapcs ]]; then + xtensa-lx106-elf-ar r libwpa2.a eap.o +fi +if [[ $(sha256sum user_interface.o | awk '{print $1}') != $uics || $(sha256sum eagle_lwip_if.o | awk '{print $1}') != $lwipcs ]]; then + xtensa-lx106-elf-ar r libmain.a eagle_lwip_if.o user_interface.o +fi +rm -f eagle_lwip_if.o user_interface.o eap.o From a2d2aa2331c948702e116b8a267cd2ffdc2e7378 Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Sun, 15 May 2022 14:16:50 -0700 Subject: [PATCH 2/4] Add some dev debug code and improve comments --- cores/esp8266/wpa2_eap_patch.cpp | 53 +++++++++++++++++++++++++++----- tools/sdk/lib/eval_fix_sdks.sh | 18 +++++++++-- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/cores/esp8266/wpa2_eap_patch.cpp b/cores/esp8266/wpa2_eap_patch.cpp index 95c74dca19..4a49cb216c 100644 --- a/cores/esp8266/wpa2_eap_patch.cpp +++ b/cores/esp8266/wpa2_eap_patch.cpp @@ -10,7 +10,8 @@ #include #include #include "coredecls.h" -#if 0 + +#ifdef DEBUG_WPA2_EAP_PATCH #include "esp8266_undocumented.h" #define DEBUG_PRINTF ets_uart_printf #else @@ -67,16 +68,28 @@ asm( * While some insight can be gained from the ESP32 repo for this structure. * It does not match exactly. This alternate structure focuses on correct offset * rather than trying to exactly reconstruct the original labels. + * These offset were found in libwpa2.a:eap.o .text.eap_peer_config_init */ struct StateMachine { // size 200 bytes void* beforeConfig[16]; void* config[26]; - // 0 - mov a2, a12, 64 // username / Identity - // 1 - mov a2, a12, 68 - // 2 - mov a2, a12, 72 // anonymous Identity - // 3 - mov a2, a12, 76 - // 4 - mov a2, a12, 80 // password - // 21 - mov a2, a12, 148 // ?? + // 0 - s32i a2, a12, 64 // username / Identity + // 1 - s32i a2, a12, 68 // length + // 2 - s32i a2, a12, 72 // anonymous Identity + // 3 - s32i a2, a12, 76 + // 4 - s32i a2, a12, 80 // password + // 5 - s32i a2, a12, 84 + // + // "new password" - From wifi_station_set_enterprise_new_password(), we see + // global saved value .bss+32 and .bss+36 which are later used to populate + // ".config" in eap_peer_config_init(). I do not have an environment to + // exercise this parameter. In my tests, the "new password" element in the + // ".config" is never initialized. At the moment, I don't see any code that + // would free the allocation. + // allocated via pvPortZalloc from line 0x30f, 783 + // 21 - s32i a2, a12, 148 // new password + // 22 - s32i a2, a12, 152 + void* afterConfig[8]; }; @@ -88,6 +101,7 @@ struct StateMachine { // size 200 bytes */ void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a12) { if (799 == line) { + // This caller is eap_peer_config_deinit() struct StateMachine* sm = (struct StateMachine*)a12; if (ptr == sm->config[0]) { // Fix leaky frunction - eap.o only frees one out of 4 config items @@ -97,8 +111,31 @@ void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a vPortFree(sm->config[21], file, line); // ptr is sm->config[0], let fall through handle it } - DEBUG_PRINTF("\nz2EapFree/vPortFree patch working\n"); +#ifdef DEBUG_WPA2_EAP_PATCH + DEBUG_PRINTF("\nz2EapFree/vPortFree patch struct StateMachine * = %8p\n", a12); + DEBUG_PRINTF(" config[0] vPortFree(%8p, file, line);\n", ptr); + DEBUG_PRINTF(" config[2] vPortFree(%8p, file, line);\n", sm->config[2]); + DEBUG_PRINTF(" config[4] vPortFree(%8p, file, line);\n", sm->config[4]); + DEBUG_PRINTF(" config[21] vPortFree(%8p, file, line);\n", sm->config[21]); + if (a12) { + void** pw = (void**)a12; + DEBUG_PRINTF("\nhexdump struct StateMachine:\n"); + for (size_t i=0; i<200/4; i+=4) { + DEBUG_PRINTF("%03u: %8p %8p %8p %8p\n", i*4, pw[i], pw[i+1], pw[i+2], pw[i+3]); + } + } +#endif + } +#if 0 + // This is not needed because the call was NO-OPed in the library. This code + // snippit is just to show how a future memory free issue might be resolved. + else if (672 == line) { + // This caller is wpa2_sm_rx_eapol() + // 1st of a double free + // let the 2nd free handle it. + return; } +#endif vPortFree(ptr, file, line); } diff --git a/tools/sdk/lib/eval_fix_sdks.sh b/tools/sdk/lib/eval_fix_sdks.sh index 333f0ebbc6..217157d325 100755 --- a/tools/sdk/lib/eval_fix_sdks.sh +++ b/tools/sdk/lib/eval_fix_sdks.sh @@ -13,6 +13,15 @@ if ! which xtensa-lx106-elf-ar | grep "tools/xtensa-lx106-elf/bin" >>/dev/null; add_path_ifexist "../../../xtensa-lx106-elf/bin" || add_path_ifexist "../../xtensa-lx106-elf/bin" fi +help_msg() { + cat < Date: Fri, 13 May 2022 08:39:02 -0700 Subject: [PATCH 3/4] Patch eap.o memory leak WiFi Enterprise option can leak up to 3 allocations per connect/disconnect cycle: anonymous Identity, password, and some unidentified allocation. This solution patches eap.o from libwpa2 to call a special 2 part wrapper instead of vPortFree for cleanup. Corrected typos and adjusted tabs in script. Added script eval_fix_sdks.sh to aid in evaluating similarity between patch sections of .o files being patched across different SDKs. --- cores/esp8266/coredecls.h | 1 + cores/esp8266/heap.cpp | 8 +- cores/esp8266/wpa2_eap_patch.cpp | 120 ++++++++++++++++++++++ tools/sdk/lib/NONOSDK221/libwpa2.a | Bin 475218 -> 475218 bytes tools/sdk/lib/NONOSDK22x_190313/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_190703/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_191024/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_191105/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK22x_191122/libwpa2.a | Bin 475222 -> 475222 bytes tools/sdk/lib/NONOSDK3V0/libwpa2.a | Bin 475358 -> 475358 bytes tools/sdk/lib/eval_fix_sdks.sh | 103 +++++++++++++++++++ tools/sdk/lib/fix_sdk_libs.sh | 80 +++++++++++++-- 12 files changed, 302 insertions(+), 10 deletions(-) create mode 100644 cores/esp8266/wpa2_eap_patch.cpp create mode 100755 tools/sdk/lib/eval_fix_sdks.sh diff --git a/cores/esp8266/coredecls.h b/cores/esp8266/coredecls.h index e06dc6029a..1e2776d71b 100644 --- a/cores/esp8266/coredecls.h +++ b/cores/esp8266/coredecls.h @@ -21,6 +21,7 @@ void esp_schedule(); void esp_yield(); void tune_timeshift64 (uint64_t now_us); void disable_extra4k_at_link_time (void) __attribute__((noinline)); +void enable_wifi_enterprise_patch(void) __attribute__((noinline)); bool sntp_set_timezone_in_seconds(int32_t timezone); void __disableWiFiAtBootTime (void) __attribute__((noinline)); void __real_system_restart_local() __attribute__((noreturn)); diff --git a/cores/esp8266/heap.cpp b/cores/esp8266/heap.cpp index 67f785c712..78011ed248 100644 --- a/cores/esp8266/heap.cpp +++ b/cores/esp8266/heap.cpp @@ -5,7 +5,13 @@ #include #include "umm_malloc/umm_malloc.h" -extern "C" size_t umm_umul_sat(const size_t a, const size_t b);; +extern "C" size_t umm_umul_sat(const size_t a, const size_t b); + +// z2EapFree: See wpa2_eap_patch.cpp for details +extern "C" void z2EapFree(void *ptr, const char* file, int line) __attribute__((weak, alias("vPortFree"), nothrow)); +// I don't understand all the compiler noise around this alias. +// Adding "__attribute__ ((nothrow))" seems to resolve the issue. +// This may be relevant: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=81824 // Need FORCE_ALWAYS_INLINE to put HeapSelect class constructor/deconstructor in IRAM #define FORCE_ALWAYS_INLINE_HEAP_SELECT diff --git a/cores/esp8266/wpa2_eap_patch.cpp b/cores/esp8266/wpa2_eap_patch.cpp new file mode 100644 index 0000000000..95c74dca19 --- /dev/null +++ b/cores/esp8266/wpa2_eap_patch.cpp @@ -0,0 +1,120 @@ +/* + * To implement this patch, the SDK module `eap.o` from archive `libwpa2.a` must + * be patched to call `z2EapFree` instead of `vPortFree`. This limits extending + * the execution time of vPortFree to that module only. Not impacting other + * modules. + * + */ + +#include +#include +#include +#include "coredecls.h" +#if 0 +#include "esp8266_undocumented.h" +#define DEBUG_PRINTF ets_uart_printf +#else +#define DEBUG_PRINTF(...) +#endif + +extern "C" { + +// extern "C" void z2EapFree(void *ptr, const char* file, int line) __attribute__((weak, alias("vPortFree"))); + +/* + * Limited 2-part wrapper for vPortFree calls made in SDK module `eap.o` from + * archive `libwpa2.a`. + * + * vPortFree calls from eap.o are monitored for calls from line 799. This is + * the location of the memory leak. At entry register a12 contains the structure + * address which has the addresses of the allocations that will be leaked. + * + * Part 1 of this wrapper, z2EapFree, appends the value of register a12 as a + * 4th argument to part2 of this wrapper, patch_wpa2_eap_vPortFree_a12(). Which + * in turn checks and frees the additional allocations, that would have been + * lost. + * + * extern "C" z2EapFree(void*); + */ + +/* + * Part 1 of Limited vPortFree Wrapper + */ +asm( + // ".section .iram.text.z2EapFree,\"ax\",@progbits\n\t" + // Since all the possible callers in eap.o are in sections starting with + // .text and not .iram.text we should be safe putting these wrappers in .text. + ".section .text.z2EapFree,\"ax\",@progbits\n\t" + ".literal_position\n\t" + ".literal .patch_wpa2_eap_vPortFree_a12, patch_wpa2_eap_vPortFree_a12\n\t" + ".align 4\n\t" + ".global z2EapFree\n\t" + ".type z2EapFree, @function\n\t" + "\n" +"z2EapFree:\n\t" + "addi a1, a1, -16\n\t" + "s32i a0, a1, 0\n\t" + "mov a5, a12\n\t" + "l32r a0, .patch_wpa2_eap_vPortFree_a12\n\t" + "callx0 a0\n\t" + "l32i a0, a1, 0\n\t" + "addi a1, a1, 16\n\t" + "ret\n\t" + ".size z2EapFree, .-z2EapFree\n\t" +); + +/* + * While some insight can be gained from the ESP32 repo for this structure. + * It does not match exactly. This alternate structure focuses on correct offset + * rather than trying to exactly reconstruct the original labels. + */ +struct StateMachine { // size 200 bytes + void* beforeConfig[16]; + void* config[26]; + // 0 - mov a2, a12, 64 // username / Identity + // 1 - mov a2, a12, 68 + // 2 - mov a2, a12, 72 // anonymous Identity + // 3 - mov a2, a12, 76 + // 4 - mov a2, a12, 80 // password + // 21 - mov a2, a12, 148 // ?? + void* afterConfig[8]; +}; + +/* + * Part 2 of Limited vPortFree Wrapper + * + * Presently, all SDKs have the same memory leaks in the same module at the + * same line. + */ +void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a12) { + if (799 == line) { + struct StateMachine* sm = (struct StateMachine*)a12; + if (ptr == sm->config[0]) { + // Fix leaky frunction - eap.o only frees one out of 4 config items + // finish the other 3 first + vPortFree(sm->config[2], file, line); + vPortFree(sm->config[4], file, line); + vPortFree(sm->config[21], file, line); + // ptr is sm->config[0], let fall through handle it + } + DEBUG_PRINTF("\nz2EapFree/vPortFree patch working\n"); + } + vPortFree(ptr, file, line); +} + +}; + +/* + * This will minimize code space for non-wifi enterprise sketches which do not + * need the patch and disable_extra4k_at_link_time(). + */ +void enable_wifi_enterprise_patch(void) { + /* + * Calling this from setup or anywhere ensures that the patch code is + * included in the build. + * + * Also, WiFi Enterprise uses a lot of system stack space and may crash + * unless we: + */ + disable_extra4k_at_link_time(); +} diff --git a/tools/sdk/lib/NONOSDK221/libwpa2.a b/tools/sdk/lib/NONOSDK221/libwpa2.a index 1f7aabb6881af8cbac966e4042736747404cebf4..f2b2178fc036f8fc0a265767f196e8d66e95a318 100644 GIT binary patch delta 104 zcmccAAbY7nc7i0QnW>S9shNR=S)m-Go7*orCvd733% delta 82 zcmccAAbY7nc7o*e`|PaTjVfDJ813Vy_kU%U-`*6@_?w&Q0K@kBY{vglY-IuYMI~;N i@396?-+PRav;Dy_Mj&PaVrC#_0bS9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_190703/libwpa2.a b/tools/sdk/lib/NONOSDK22x_190703/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_191024/libwpa2.a b/tools/sdk/lib/NONOSDK22x_191024/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_191105/libwpa2.a b/tools/sdk/lib/NONOSDK22x_191105/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK22x_191122/libwpa2.a b/tools/sdk/lib/NONOSDK22x_191122/libwpa2.a index 07420c5bf00794d2f6254497af25217c70b62696..fb08a7ee3e05bbebd14764eb09ef64dc112ed672 100644 GIT binary patch delta 99 zcmccCAbYJrc7i0QnW>S9shNR=S)5HP6geSjd ab!)$Uj1h>LfS4JGS%8>z`|V?F)s+Bcf*@i5 diff --git a/tools/sdk/lib/NONOSDK3V0/libwpa2.a b/tools/sdk/lib/NONOSDK3V0/libwpa2.a index b868608a143d1eea9745d5044979f45bc8adf4d9..0826b45e7fb87b53d011d971fbc9dcedff052070 100644 GIT binary patch delta 99 zcmccDD0{C_c7i0QnW>S9shNR=S)V{D%*0h{e1)&Kwi diff --git a/tools/sdk/lib/eval_fix_sdks.sh b/tools/sdk/lib/eval_fix_sdks.sh new file mode 100755 index 0000000000..333f0ebbc6 --- /dev/null +++ b/tools/sdk/lib/eval_fix_sdks.sh @@ -0,0 +1,103 @@ +#!/bin/bash +# set -e + +add_path_ifexist() { + if [[ -d $1 ]]; then + export PATH=$( realpath $1 ):$PATH + return 0 + fi + return 1 +} + +if ! which xtensa-lx106-elf-ar | grep "tools/xtensa-lx106-elf/bin" >>/dev/null; then + add_path_ifexist "../../../xtensa-lx106-elf/bin" || add_path_ifexist "../../xtensa-lx106-elf/bin" +fi + + +list_sdks() { + cat <new.txt + if [[ -f old.txt ]]; then + echo "eap_peer_config_deinit: diff $prev_sdk $sdk" + diff old.txt new.txt + echo "" + fi + mv new.txt old.txt + prev_sdk=${sdk} + done + + unset prev_sdk + for sdk in `list_sdks`; do + unasm -j ".text.wpa2_sm_rx_eapol" ${sdk}/eap.o >new.txt + if [[ -f old2.txt ]]; then + echo "wpa2_sm_rx_eapol: diff $prev_sdk $sdk" + diff old2.txt new.txt + echo "" + fi + mv new.txt old2.txt + prev_sdk=${sdk} + done + + # Find offsets for patching vPortFree with z2EapFree + for sdk in `list_sdks`; do + echo -en "\n${sdk}/eap.o:\n " + grep --byte-offset --only-matching --text vPortFree ${sdk}/eap.o + done + + cleanup +} + + +patch_all() { + for sdk in `list_sdks`; do + pushd $sdk + ../fix_sdk_libs.sh + popd + done +} + +# analyze +patch_all +exit 0 diff --git a/tools/sdk/lib/fix_sdk_libs.sh b/tools/sdk/lib/fix_sdk_libs.sh index b6aea12752..c94e9492d0 100755 --- a/tools/sdk/lib/fix_sdk_libs.sh +++ b/tools/sdk/lib/fix_sdk_libs.sh @@ -1,36 +1,98 @@ #!/bin/bash set -e -export PATH=../../xtensa-lx106-elf/bin:$PATH +add_path_ifexist() { + if [[ -d $1 ]]; then + export PATH=$( realpath $1 ):$PATH + return 0 + fi + return 1 +} + +if ! which xtensa-lx106-elf-ar | grep "tools/xtensa-lx106-elf/bin" >>/dev/null; then + add_path_ifexist "../../../xtensa-lx106-elf/bin" || add_path_ifexist "../../xtensa-lx106-elf/bin" +fi +WORK_SPACE=${PWD} + VERSION=$(basename ${PWD}) addSymbol_system_func1() { - ADDRESS=$1 - xtensa-lx106-elf-objcopy --add-symbol system_func1=.irom0.text:${ADDRESS},function,global user_interface.o + if ! xtensa-lx106-elf-nm user_interface.o | grep -q " T system_func1"; then # Don't add symbol if it already exists + ADDRESS=$1 + xtensa-lx106-elf-objcopy --add-symbol system_func1=.irom0.text:${ADDRESS},function,global user_interface.o + fi } +patchFile() { + FILE=$1 + ADDRESS=$2 # DO NOT PASS AS HEX! + LENGTH=$3 # DO NOT PASS AS HEX! + EXPECTED=$4 + REPLACEWITH=$5 + if [[ "$(dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0)" = "$EXPECTED" ]]; then + echo "Patching $VERSION $1 ..." + echo $5 | base64 -d | dd of=$FILE bs=1 count=$LENGTH seek=$ADDRESS conv=notrunc + elif ! [[ "$(dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0)" = "$REPLACEWITH" ]]; then + echo "PATCH FAILED!" + echo "dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0" + dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | hexdump -C + dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0 + echo "" + exit 1 + fi +} + +# # xtensa-lx106-elf-ar x libwpa2.a eap.o +if [[ "--shell" == "$1" ]]; then + # need to poke around a bit + bash --rcfile <(echo ". ~/.bashrc; cd ${WORK_SPACE}") + exit 0 +fi + +if [[ ! -f libmain.a ]]; then + echo -e "\n\n*** Archive libmain.a is missing ***\n\n" + exit 1 +fi # Remove mem_manager.o from libmain.a to use custom heap implementation, # and time.o to fix redefinition of time-related functions: xtensa-lx106-elf-ar d libmain.a mem_manager.o xtensa-lx106-elf-ar d libmain.a time.o +# Patch WPA2-Enterprise double-free +xtensa-lx106-elf-ar x libwpa2.a eap.o +eapcs=$(sha256sum eap.o | awk '{print $1}') + # Rename `hostname` and `default_hostname` symbols: xtensa-lx106-elf-ar x libmain.a eagle_lwip_if.o user_interface.o -xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname user_interface.o -xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname eagle_lwip_if.o -xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname user_interface.o -xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname eagle_lwip_if.o +lwipcs=$(sha256sum eagle_lwip_if.o | awk '{print $1}') +uics=$(sha256sum user_interface.o | awk '{print $1}') +xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname user_interface.o +xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname eagle_lwip_if.o +xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname user_interface.o +xtensa-lx106-elf-objcopy --redefine-sym default_hostname=wifi_station_default_hostname eagle_lwip_if.o + if [[ ${VERSION} == "NONOSDK221" ]]; then addSymbol_system_func1 "0x60" + patchFile "eap.o" "3055" "2" "wAA=" "8CA=" # WPA2-Enterprise patch which replaces a double-free with nop, see #8082 + patchFile "eap.o" "26352" "9" "dlBvcnRGcmVl" "ejJFYXBGcmVl" # special vPortFree to recover leaked memory elif [[ ${VERSION} == "NONOSDK22x"* ]]; then addSymbol_system_func1 "0x54" + patchFile "eap.o" "3059" "2" "wAA=" "8CA=" # WPA2-Enterprise patch which replaces a double-free with nop, see #8082 + patchFile "eap.o" "26356" "9" "dlBvcnRGcmVl" "ejJFYXBGcmVl" # special vPortFree to recover leaked memory elif [[ ${VERSION} == "NONOSDK3"* ]]; then addSymbol_system_func1 "0x60" + patchFile "eap.o" "3059" "2" "wAA=" "8CA=" # WPA2-Enterprise patch which replaces a double-free with nop, see #8082 + patchFile "eap.o" "26356" "9" "dlBvcnRGcmVl" "ejJFYXBGcmVl" # special vPortFree to recover leaked memory else echo "WARN: Unknown address for system_func1() called by system_restart_local()" fi -xtensa-lx106-elf-ar r libmain.a eagle_lwip_if.o user_interface.o -rm -f eagle_lwip_if.o user_interface.o +if [[ $(sha256sum eap.o | awk '{print $1}') != $eapcs ]]; then + xtensa-lx106-elf-ar r libwpa2.a eap.o +fi +if [[ $(sha256sum user_interface.o | awk '{print $1}') != $uics || $(sha256sum eagle_lwip_if.o | awk '{print $1}') != $lwipcs ]]; then + xtensa-lx106-elf-ar r libmain.a eagle_lwip_if.o user_interface.o +fi +rm -f eagle_lwip_if.o user_interface.o eap.o From 1d88df0bf0fcf91355d3fd9a0fd4b25def54900d Mon Sep 17 00:00:00 2001 From: M Hightower <27247790+mhightower83@users.noreply.github.com> Date: Sun, 15 May 2022 14:16:50 -0700 Subject: [PATCH 4/4] Add some dev debug code and improve comments --- cores/esp8266/wpa2_eap_patch.cpp | 53 +++++++++++++++++++++++++++----- tools/sdk/lib/eval_fix_sdks.sh | 18 +++++++++-- 2 files changed, 61 insertions(+), 10 deletions(-) diff --git a/cores/esp8266/wpa2_eap_patch.cpp b/cores/esp8266/wpa2_eap_patch.cpp index 95c74dca19..4a49cb216c 100644 --- a/cores/esp8266/wpa2_eap_patch.cpp +++ b/cores/esp8266/wpa2_eap_patch.cpp @@ -10,7 +10,8 @@ #include #include #include "coredecls.h" -#if 0 + +#ifdef DEBUG_WPA2_EAP_PATCH #include "esp8266_undocumented.h" #define DEBUG_PRINTF ets_uart_printf #else @@ -67,16 +68,28 @@ asm( * While some insight can be gained from the ESP32 repo for this structure. * It does not match exactly. This alternate structure focuses on correct offset * rather than trying to exactly reconstruct the original labels. + * These offset were found in libwpa2.a:eap.o .text.eap_peer_config_init */ struct StateMachine { // size 200 bytes void* beforeConfig[16]; void* config[26]; - // 0 - mov a2, a12, 64 // username / Identity - // 1 - mov a2, a12, 68 - // 2 - mov a2, a12, 72 // anonymous Identity - // 3 - mov a2, a12, 76 - // 4 - mov a2, a12, 80 // password - // 21 - mov a2, a12, 148 // ?? + // 0 - s32i a2, a12, 64 // username / Identity + // 1 - s32i a2, a12, 68 // length + // 2 - s32i a2, a12, 72 // anonymous Identity + // 3 - s32i a2, a12, 76 + // 4 - s32i a2, a12, 80 // password + // 5 - s32i a2, a12, 84 + // + // "new password" - From wifi_station_set_enterprise_new_password(), we see + // global saved value .bss+32 and .bss+36 which are later used to populate + // ".config" in eap_peer_config_init(). I do not have an environment to + // exercise this parameter. In my tests, the "new password" element in the + // ".config" is never initialized. At the moment, I don't see any code that + // would free the allocation. + // allocated via pvPortZalloc from line 0x30f, 783 + // 21 - s32i a2, a12, 148 // new password + // 22 - s32i a2, a12, 152 + void* afterConfig[8]; }; @@ -88,6 +101,7 @@ struct StateMachine { // size 200 bytes */ void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a12) { if (799 == line) { + // This caller is eap_peer_config_deinit() struct StateMachine* sm = (struct StateMachine*)a12; if (ptr == sm->config[0]) { // Fix leaky frunction - eap.o only frees one out of 4 config items @@ -97,8 +111,31 @@ void patch_wpa2_eap_vPortFree_a12(void *ptr, const char* file, int line, void* a vPortFree(sm->config[21], file, line); // ptr is sm->config[0], let fall through handle it } - DEBUG_PRINTF("\nz2EapFree/vPortFree patch working\n"); +#ifdef DEBUG_WPA2_EAP_PATCH + DEBUG_PRINTF("\nz2EapFree/vPortFree patch struct StateMachine * = %8p\n", a12); + DEBUG_PRINTF(" config[0] vPortFree(%8p, file, line);\n", ptr); + DEBUG_PRINTF(" config[2] vPortFree(%8p, file, line);\n", sm->config[2]); + DEBUG_PRINTF(" config[4] vPortFree(%8p, file, line);\n", sm->config[4]); + DEBUG_PRINTF(" config[21] vPortFree(%8p, file, line);\n", sm->config[21]); + if (a12) { + void** pw = (void**)a12; + DEBUG_PRINTF("\nhexdump struct StateMachine:\n"); + for (size_t i=0; i<200/4; i+=4) { + DEBUG_PRINTF("%03u: %8p %8p %8p %8p\n", i*4, pw[i], pw[i+1], pw[i+2], pw[i+3]); + } + } +#endif + } +#if 0 + // This is not needed because the call was NO-OPed in the library. This code + // snippit is just to show how a future memory free issue might be resolved. + else if (672 == line) { + // This caller is wpa2_sm_rx_eapol() + // 1st of a double free + // let the 2nd free handle it. + return; } +#endif vPortFree(ptr, file, line); } diff --git a/tools/sdk/lib/eval_fix_sdks.sh b/tools/sdk/lib/eval_fix_sdks.sh index 333f0ebbc6..217157d325 100755 --- a/tools/sdk/lib/eval_fix_sdks.sh +++ b/tools/sdk/lib/eval_fix_sdks.sh @@ -13,6 +13,15 @@ if ! which xtensa-lx106-elf-ar | grep "tools/xtensa-lx106-elf/bin" >>/dev/null; add_path_ifexist "../../../xtensa-lx106-elf/bin" || add_path_ifexist "../../xtensa-lx106-elf/bin" fi +help_msg() { + cat <