Skip to content

Commit

Permalink
Patch eap.o memory leak (#8566)
Browse files Browse the repository at this point in the history
* 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.

* Add some dev debug code and improve comments

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

* Add some dev debug code and improve comments
  • Loading branch information
mhightower83 committed Jun 2, 2022
1 parent 5f2af19 commit 9e2103f
Show file tree
Hide file tree
Showing 12 changed files with 317 additions and 5 deletions.
1 change: 1 addition & 0 deletions cores/esp8266/coredecls.h
Original file line number Diff line number Diff line change
Expand Up @@ -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));
Expand Down
8 changes: 7 additions & 1 deletion cores/esp8266/heap.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,13 @@

#include <stdlib.h>
#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
Expand Down
157 changes: 157 additions & 0 deletions cores/esp8266/wpa2_eap_patch.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,157 @@
/*
* 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 <string.h>
#include <ets_sys.h>
#include <pgmspace.h>
#include "coredecls.h"

#ifdef DEBUG_WPA2_EAP_PATCH
#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.
* 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 - 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];
};

/*
* 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) {
// 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
// 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
}
#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);
}

};

/*
* 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();
}
Binary file modified tools/sdk/lib/NONOSDK221/libwpa2.a
Binary file not shown.
Binary file modified tools/sdk/lib/NONOSDK22x_190313/libwpa2.a
Binary file not shown.
Binary file modified tools/sdk/lib/NONOSDK22x_190703/libwpa2.a
Binary file not shown.
Binary file modified tools/sdk/lib/NONOSDK22x_191024/libwpa2.a
Binary file not shown.
Binary file modified tools/sdk/lib/NONOSDK22x_191105/libwpa2.a
Binary file not shown.
Binary file modified tools/sdk/lib/NONOSDK22x_191122/libwpa2.a
Binary file not shown.
Binary file modified tools/sdk/lib/NONOSDK3V0/libwpa2.a
Binary file not shown.
117 changes: 117 additions & 0 deletions tools/sdk/lib/eval_fix_sdks.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,117 @@
#!/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

help_msg() {
cat <<EOF
Try:
eval_fix_sdks.sh --analyze
or
eval_fix_sdks.sh --patch
EOF
}

list_sdks() {
cat <<EOF
NONOSDK22x_190313
NONOSDK22x_190703
NONOSDK22x_191024
NONOSDK22x_191105
NONOSDK22x_191122
NONOSDK221
NONOSDK3V0
EOF
}

remove_ifexist() {
[[ -f $1 ]] && rm $1
}

cleanup() {
remove_ifexist old.txt
remove_ifexist old2.txt
remove_ifexist new.txt
for sdk in `list_sdks`; do
remove_ifexist $sdk/eap.o
done
}

unasm() {
xtensa-lx106-elf-objdump -d $*
}

analyze() {
cleanup

for sdk in `list_sdks`; do
pushd $sdk
xtensa-lx106-elf-ar x libwpa2.a eap.o
popd
done
echo ""

find . -name eap.o -exec md5sum {} \; | sort
echo ""

unset prev_sdk
for sdk in `list_sdks`; do
unasm -j ".text.eap_peer_config_deinit" ${sdk}/eap.o >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
}

if [[ "${1}" == "--analyze" ]]; then
analyze
elif [[ "${1}" == "--patch" ]]; then
patch_all
else
help_msg
fi
exit 0
39 changes: 35 additions & 4 deletions tools/sdk/lib/fix_sdk_libs.sh
Original file line number Diff line number Diff line change
@@ -1,7 +1,19 @@
#!/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() {
Expand All @@ -18,14 +30,30 @@ patchFile() {
EXPECTED=$4
REPLACEWITH=$5
if [[ "$(dd if=$FILE bs=1 count=$LENGTH skip=$ADDRESS status=none | base64 -w0)" = "$EXPECTED" ]]; then
echo "Patching $1..."
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!"
exit 0
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
Expand All @@ -44,15 +72,19 @@ xtensa-lx106-elf-objcopy --redefine-sym hostname=wifi_station_hostname eagle_lwi
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
Expand All @@ -64,4 +96,3 @@ if [[ $(sha256sum user_interface.o | awk '{print $1}') != $uics || $(sha256sum e
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

0 comments on commit 9e2103f

Please sign in to comment.