fix(modem): spinlock_acquire assert (use-after-free) when tearing down / switching modes while RX data is in flight

components/esp_modem/Kconfig

@@ -36,6 +36,12 @@ menu "esp-modem"
             The typical reason for failing SABM request without a delay is that
             some devices (SIM800) send MSC requests just after opening a new DLCI.
 
+    config ESP_MODEM_CMUX_DELAY_AFTER_NETIF_DISCONNECT
+        int "Delay in ms to wait before closing virtual terminal abd rigt after PPP disconnect(NETIF)"
+        default 0
+        help
+            Some devices might need a pause before sending disconnect command that closes
+            virtual terminal. This delay applies only in CMUX mode.
     config ESP_MODEM_CMUX_USE_SHORT_PAYLOADS_ONLY
         bool "CMUX to support only short payloads (<128 bytes)"
         default n

components/esp_modem/include/cxx_include/esp_modem_cmux.hpp

@@ -49,7 +49,7 @@ class CMux {
 public:
     explicit CMux(std::shared_ptr<Terminal> t, unique_buffer &&b):
         term(std::move(t)), payload_start(nullptr), total_payload_size(0), buffer(std::move(b))  {}
-    ~CMux() = default;
+    ~CMux();
 
     /**
      * @brief Initializes CMux protocol
@@ -96,6 +96,14 @@ class CMux {
      */
     bool recover();
 
+    /**
+     * @brief Stops the underlying (physical) terminal's RX task.
+     *
+     * Used on teardown to quiesce the data pump that drives the virtual terminals before the
+     * CMux/DTE state it touches is destroyed.
+     */
+    void stop();
+
 private:
 
     enum class protocol_mismatch_reason {
@@ -150,6 +158,13 @@ class CMux {
     unique_buffer buffer;
 
     Lock lock;
+    /**
+     * @brief Serializes (re)assignment of read_cb[] (set_read_cb()) against their invocation from
+     * the underlying terminal's RX task. Deliberately separate from `lock`: the read callback
+     * upcall may take a long time (or block in the network stack) and must not block write(),
+     * which uses `lock`. Recursive, so the upcall may re-enter write().
+     */
+    Lock cb_lock;
 };
 
 /**
@@ -173,7 +188,12 @@ class CMuxInstance: public Terminal {
         return  0;
     }
     void start() override { }
-    void stop() override { }
+    // A virtual terminal has no task of its own; the physical terminal pumps all of them. Forward
+    // stop() so that tearing down a DTE in CMUX mode quiesces that physical terminal's RX task.
+    void stop() override
+    {
+        cmux->stop();
+    }
 private:
     std::shared_ptr<CMux> cmux;
     size_t instance;

components/esp_modem/include/cxx_include/esp_modem_dte.hpp

@@ -53,7 +53,7 @@ class DTE : public CommandableIf {
     explicit DTE(const esp_modem_dte_config *config, std::unique_ptr<Terminal> t, std::unique_ptr<Terminal> s);
     explicit DTE(std::unique_ptr<Terminal> t, std::unique_ptr<Terminal> s);
 
-    ~DTE() = default;
+    ~DTE();
 
     /**
      * @brief Writing to the underlying terminal

components/esp_modem/include/cxx_include/esp_modem_terminal.hpp

@@ -45,11 +45,13 @@ class Terminal {
 
     void set_error_cb(std::function<void(terminal_error)> f)
     {
+        Scoped<Lock> l(cb_lock);
         on_error = std::move(f);
     }
 
     virtual void set_read_cb(std::function<bool(uint8_t *data, size_t len)> f)
     {
+        Scoped<Lock> l(cb_lock);
         on_read = std::move(f);
     }
 
@@ -74,6 +76,14 @@ class Terminal {
     virtual void stop() = 0;
 
 protected:
+    /**
+     * @brief Serializes (re)assignment of on_read/on_error (set_read_cb/set_error_cb) against their
+     * invocation by the terminal's RX task. Recursive, so a callback may re-enter set_read_cb()
+     * (e.g. DTE clears its own read callback from within the callback). Holding this lock while
+     * clearing a callback guarantees no callback is in flight afterwards, which is what makes it
+     * safe to tear down the state the callback captures.
+     */
+    Lock cb_lock{};
     std::function<bool(uint8_t *data, size_t len)> on_read;
     std::function<void(terminal_error)> on_error;
 };

components/esp_modem/src/esp_modem_cmux.cpp

@@ -124,6 +124,10 @@ bool CMux::data_available(uint8_t *data, size_t len)
     if (data && (type & FT_UIH) == FT_UIH && len > 0 && dlci > 0) { // valid payload on a virtual term
         int virtual_term = dlci - 1;
         if (virtual_term < MAX_TERMINALS_NUM) {
+            // Hold cb_lock (not the state lock) across the read_cb check and invocation, so the
+            // callback cannot be reassigned/cleared (set_read_cb()/teardown) while we're using it,
+            // without blocking write() during the (potentially long) upcall.
+            Scoped<Lock> l(cb_lock);
             if (read_cb[virtual_term] == nullptr) {
                 // ignore all virtual terminal's data before we completely establish CMUX
                 ESP_LOG_BUFFER_HEXDUMP("CMUX Rx before init", data, len, ESP_LOG_DEBUG);
@@ -148,6 +152,7 @@ bool CMux::data_available(uint8_t *data, size_t len)
     } else if (data == nullptr && dlci > 0) {
         int virtual_term = dlci - 1;
         if (virtual_term < MAX_TERMINALS_NUM) {
+            Scoped<Lock> l(cb_lock);
             if (read_cb[virtual_term] == nullptr) {
                 // silently ignore this CMUX frame (not finished entering CMUX, yet)
                 return true;
@@ -430,6 +435,18 @@ bool CMux::deinit()
     return true;
 }
 
+CMux::~CMux()
+{
+    // The underlying terminal's RX task drives on_cmux_data(), which touches CMux state (lock,
+    // buffer, read_cb[]). Detach it before that state is destroyed, otherwise a late RX event
+    // would use this CMux after free. set_read_cb()/set_error_cb() are synchronized against the
+    // RX task, so once they return no callback is in flight.
+    if (term) {
+        term->set_read_cb(nullptr);
+        term->set_error_cb(nullptr);
+    }
+}
+
 bool CMux::init()
 {
     frame_header_offset = 0;
@@ -494,6 +511,7 @@ int CMux::write(int virtual_term, uint8_t *data, size_t len)
 
 void CMux::set_read_cb(int inst, std::function<bool(uint8_t *, size_t)> f)
 {
+    Scoped<Lock> l(cb_lock);
     if (inst < MAX_TERMINALS_NUM) {
         read_cb[inst] = std::move(f);
     }
@@ -519,3 +537,10 @@ bool CMux::recover()
     recover_protocol(protocol_mismatch_reason::UNKNOWN);
     return true;
 }
+
+void CMux::stop()
+{
+    if (term) {
+        term->stop();
+    }
+}

components/esp_modem/src/esp_modem_dce.cpp

@@ -160,6 +160,7 @@ bool DCE_Mode::set_unsafe(DTE *dte, ModuleIf *device, Netif &netif, modem_mode m
         if (mode == modem_mode::CMUX_MODE) {
             netif.stop();
             netif.wait_until_ppp_exits();
+            usleep(CONFIG_ESP_MODEM_CMUX_DELAY_AFTER_NETIF_DISCONNECT * 1'000);
             if (!dte->set_mode(modem_mode::COMMAND_MODE)) {
                 return false;
             }

components/esp_modem/src/esp_modem_dte.cpp

@@ -61,6 +61,28 @@ DTE::DTE(std::unique_ptr<Terminal> t, std::unique_ptr<Terminal> s)
 }
 
 
+DTE::~DTE()
+{
+    // The terminals' RX tasks invoke read/error callbacks that capture this DTE (and reference
+    // command_cb.line_lock). Members are destroyed after this body runs, and command_cb is
+    // destroyed first of all (declared last), so we must guarantee no callback can fire into a
+    // half-destroyed DTE.
+    //
+    // stop() quiesces the RX task (for CMUX, CMuxInstance::stop() forwards to the physical
+    // terminal); clearing the callbacks afterwards is synchronized against the RX task, so once
+    // these return no callback is in flight and none will start.
+    if (primary_term) {
+        primary_term->stop();
+        primary_term->set_read_cb(nullptr);
+        primary_term->set_error_cb(nullptr);
+    }
+    if (secondary_term && secondary_term != primary_term) {
+        secondary_term->stop();
+        secondary_term->set_read_cb(nullptr);
+        secondary_term->set_error_cb(nullptr);
+    }
+}
+
 void DTE::set_command_callbacks()
 {
     primary_term->set_read_cb([this](uint8_t *data, size_t len) {

components/esp_modem/src/esp_modem_term_fs.cpp

@@ -45,7 +45,16 @@ class FdTerminal : public Terminal {
 
     void stop() override
     {
-        signal.clear(TASK_START);
+        if (signal.is_any(TASK_STOPPED)) {
+            return;     // already stopped (stop() is also called from the destructor)
+        }
+        signal.clear(TASK_START);   // make the processing loop exit
+        signal.set(TASK_STOP);      // release the task if it never started
+        // Block until the task confirms it has left the processing loop, so no read callback is
+        // in flight when the task is deleted .
+        if (!signal.wait_any(TASK_STOPPED, stop_timeout_ms)) {
+            ESP_LOGW(TAG, "Timed out waiting for fs task to stop");
+        }
     }
 
     int write(uint8_t *data, size_t len) override;
@@ -54,17 +63,28 @@ class FdTerminal : public Terminal {
 
     void set_read_cb(std::function<bool(uint8_t *data, size_t len)> f) override
     {
+        Scoped<Lock> l(cb_lock);
         on_read = std::move(f);
-        signal.set(TASK_PARAMS);
     }
 
 private:
     void task();
 
+    // Invoke on_read under cb_lock so it cannot be reassigned/cleared (set_mode()/teardown on
+    // another task) while we're executing it; re-check under the lock.
+    void notify_read()
+    {
+        Scoped<Lock> l(cb_lock);
+        if (on_read) {
+            on_read(nullptr, 0);
+        }
+    }
+
     static const size_t TASK_INIT = SignalGroup::bit0;
     static const size_t TASK_START = SignalGroup::bit1;
     static const size_t TASK_STOP = SignalGroup::bit2;
-    static const size_t TASK_PARAMS = SignalGroup::bit3;
+    static const size_t TASK_STOPPED = SignalGroup::bit3;   /*!< Set by the task once it has left the processing loop */
+    static const uint32_t stop_timeout_ms = 2000;           /*!< Max wait for a graceful stop (must exceed the select() timeout) */
 
     File f;
     SignalGroup signal;
@@ -86,13 +106,22 @@ FdTerminal::FdTerminal(const esp_modem_dte_config *config) :
 {
     auto t = static_cast<FdTerminal *>(p);
     t->task();
-    Task::Delete();
+    // task() returns only after stop() requested it. Acknowledge that the processing loop has
+    // been left -- this is the last access to any member.
+    t->signal.set(TASK_STOPPED);
+#if !defined(CONFIG_IDF_TARGET_LINUX)
+    // FreeRTOS: a task function must not return. Idle until the owner (Task destructor) deletes
+    // us; we deliberately do not self-delete, to avoid racing that delete.
+    while (true) {
+        Task::Delay(3600 * 1000);
+    }
+#endif
+    // Linux: returning ends the std::thread, which the Task destructor join()s.
 })
 {}
 
 void FdTerminal::task()
 {
-    std::function<bool(uint8_t *data, size_t len)> on_read_priv = nullptr;
     signal.set(TASK_INIT);
     signal.wait_any(TASK_START | TASK_STOP, portMAX_DELAY);
     if (signal.is_any(TASK_STOP)) {
@@ -110,20 +139,14 @@ void FdTerminal::task()
         FD_SET(f.fd, &rfds);
 
         s = select(f.fd + 1, &rfds, nullptr, nullptr, &tv);
-        if (signal.is_any(TASK_PARAMS)) {
-            on_read_priv = on_read;
-            signal.clear(TASK_PARAMS);
-        }
 
         if (s < 0) {
             break;
         } else if (s == 0) {
 //            ESP_LOGV(TAG, "Select exited with timeout");
         } else {
             if (FD_ISSET(f.fd, &rfds)) {
-                if (on_read_priv) {
-                    on_read_priv(nullptr, 0);
-                }
+                notify_read();
             }
         }
         Task::Relinquish();