kevin/meshtastic_firmware
1
0
Fork 0
Code Issues Pull Requests Actions 7 Packages Projects Releases Wiki Activity

esp32: fix NimBLE use-after-free crash when disabling bluetooth while connected (#10950)

Browse Source 
* esp32: fix NimBLE use-after-free crash when disabling bluetooth while connected

Saving certain configs over BLE (e.g. MQTT module config) calls
disableBluetooth() -> NimbleBluetooth::deinit() while the phone is still
connected. deinit() called BLEDevice::deinit(true) straight away, which
deletes the BLEServer before nimble_port_stop(). Stopping the NimBLE
port with a live connection makes the host synthesize unsubscribe events
via ble_gatts_connection_broken() and dispatch them into the freed
BLEServer -> LoadProhibited panic in BLEServer::handleGATTServerEvent
(iterating server->m_notifyChrVec on freed memory).

Backtrace tail:
  BLEServer::handleGATTServerEvent (SUBSCRIBE)
  <- ble_gatts_subscribe_event <- ble_gatts_connection_broken
  <- ble_gap_conn_broken <- ble_gap_rx_disconn_complete

It was reliably preceded by our onRead callback pinning the NimBLE host
task in its up-to-20s polling loop ('BLE onRead: timeout after 19920 ms,
4000 tries'), so the host task couldn't process the teardown until long
after the objects under it were freed.

Fix: drain before demolition in deinit(). Clear
onReadCallbackIsWaitingForData so an in-flight onRead returns
immediately; disconnect the peer and wait (bounded, ~2s) for the host
task to run onDisconnect (which clears nimbleBluetoothConnHandle) so the
unsubscribe/GATT teardown lands on the still-live server; only then
BLEDevice::deinit(true). isDeInit is raised after the drain (onDisconnect
early-returns when set and would otherwise never clear the handle), the
deferred advertising restart is dropped (the stack is going away), and
the now-dangling bleServer pointer is nulled.

deinit() runs on the main task, so waiting on the host task's callbacks
is safe; the waits are bounded regardless.

Upstream ordering bug (delete BLEServer before nimble_port_stop) to be
reported to espressif/arduino-esp32 separately.

* esp32: gate onRead during BLE teardown and use Throttle for drain wait

Addresses review feedback on the NimBLE deinit UAF fix:

- Set a bleDraining flag before disconnecting so onRead bails immediately
  instead of arming the up-to-20s wait. Without this, a read that re-arms
  after deinit() clears onReadCallbackIsWaitingForData could pin the NimBLE
  host task, block onDisconnect from clearing the conn handle, and make the
  2s drain expire. onRead now skips the wait when draining, and the wait
  loop also observes the flag so an in-flight read is released.
- Use Throttle::isWithinTimespanMs() for the bounded drain wait instead of
  a raw millis() subtraction, per coding guidelines.

* esp32: reset BLE teardown guards on re-init

deinit() latches bleDraining (new) and isDeInit (pre-existing) as teardown
guards, but setup() never cleared them. AdminModule's disable-bluetooth admin
command calls deinit() directly without a reboot, and PowerFSM can re-enable
via setBluetoothEnable(true) -> setup() on the same boot (isActive() is false
because deinit() nulls bleServer). Without resetting the flags, the re-initialized
stack has onRead permanently bailing on the drain path (empty reads) and
onDisconnect early-returning without clearing the connection handle.

Clear both flags at the top of setup() so a re-init recovers cleanly.
This commit is contained in:
Ben Meadors
2026-07-09 09:42:35 -05:00
committed by GitHub
co-authored by GitHub
parent faf92f6720
commit 168f669e61
1 changed files with 39 additions and 2 deletions
Download Patch File Download Diff File Expand all files Collapse all files
src/nimble/NimbleBluetooth.cpp
+39 -2
View File
@@ -8,6 +8,7 @@
#include "concurrency/OSThread.h"
#include "main.h"
#include "mesh/PhoneAPI.h"
#include "mesh/Throttle.h"
#include "mesh/mesh-pb-constants.h"
#include "sleep.h"
#include <BLE2904.h>
@@ -107,6 +108,10 @@ static std::atomic<uint16_t> nimbleBluetoothConnHandle{BLE_HS_CONN_HANDLE_NONE};
// callback while the host is mid-reset crashes (LoadProhibited), so the main task does it instead.
static std::atomic<bool> pendingStartAdvertising{false};
// Set by deinit() before it disconnects. Makes onRead bail immediately instead of arming the
// up-to-20s wait, so a read arriving mid-teardown can't pin the NimBLE task and stall the disconnect.
static std::atomic<bool> bleDraining{false};
static void clearPairingDisplay()
{
if (!passkeyShowing) {
@@ -541,14 +546,18 @@ class NimbleBluetoothFromRadioCallback : public BLECharacteristicCallbacks
#ifdef DEBUG_NIMBLE_ON_READ_TIMING
LOG_DEBUG("BLE onRead(%d): packet already waiting, no need to set onReadCallbackIsWaitingForData", currentReadCount);
#endif
} else {
} else if (!bleDraining) {
// (If deinit() is tearing the stack down, skip the wait entirely and just return a 0-size
// response below - arming the wait here could pin this NimBLE task for ~20s and stall teardown.)
// Tell the main task that we'd like a packet.
bluetoothPhoneAPI->onReadCallbackIsWaitingForData = true;
// Wait for the main task to produce a packet for us, up to about 20 seconds.
// It normally takes just a few milliseconds, but at initial startup, etc, the main task can get blocked for longer
// doing various setup tasks.
while (bluetoothPhoneAPI->onReadCallbackIsWaitingForData && tries < 4000) {
// bleDraining lets deinit() release an in-flight wait immediately.
while (bluetoothPhoneAPI->onReadCallbackIsWaitingForData && !bleDraining && tries < 4000) {
// Schedule the main task runOnce to run ASAP.
bluetoothPhoneAPI->setIntervalFromNow(0);
concurrency::mainDelay.interrupt(); // wake up main loop if sleeping
@@ -801,13 +810,35 @@ void NimbleBluetooth::deinit()
{
#ifdef ARCH_ESP32
LOG_INFO("Disable bluetooth until reboot");
// BLEDevice::deinit() deletes the BLEServer before nimble_port_stop(); doing that with a live
// connection dispatches synthesized unsubscribe events into the freed server (LoadProhibited),
// so disconnect cleanly first. deinit() runs on the main task; the waits below are bounded.
// bleDraining must be set before we clear the flag / disconnect, so a read arriving now bails
// instead of re-arming the ~20s wait and re-pinning the NimBLE task through the whole teardown.
bleDraining = true;
if (bluetoothPhoneAPI)
bluetoothPhoneAPI->onReadCallbackIsWaitingForData = false; // release any in-flight onRead
// isDeInit must stay false here, else onDisconnect early-returns without clearing the handle.
uint16_t connHandle = nimbleBluetoothConnHandle.load();
if (connHandle != BLE_HS_CONN_HANDLE_NONE && bleServer) {
bleServer->disconnect(connHandle);
uint32_t start = millis();
while (nimbleBluetoothConnHandle.load() != BLE_HS_CONN_HANDLE_NONE && Throttle::isWithinTimespanMs(start, 2000))
delay(10);
delay(50);
}
isDeInit = true;
pendingStartAdvertising = false; // stack is going away; don't let runOnce retry the adv restart
#ifdef BLE_LED
digitalWrite(BLE_LED, LED_STATE_OFF);
#endif
BLEDevice::deinit(true);
bleServer = nullptr; // deleted by deinit(); clear the dangling copy
BatteryCharacteristic = nullptr; // freed by deinit; clear so updateBatteryLevel() won't touch it
lastBatteryLevel = -1;
#endif
@@ -850,6 +881,12 @@ void NimbleBluetooth::setup()
LOG_INFO("Init the NimBLE bluetooth module");
// deinit() latches these teardown guards; clear them so a re-init on the same boot (e.g. an
// admin disable-bluetooth followed by re-enable) doesn't leave onRead stuck draining or
// onDisconnect early-returning without clearing the connection handle.
bleDraining = false;
isDeInit = false;
#ifdef ARCH_ESP32
// Runs before BLEDevice::init() reads the bond store, but logs after the "Init" line above so
// any bond-cleanup output doesn't appear to precede the module init.