Merge request !1509 from 'netlink-completion-use-after-free'

netlink: fix use-after-free calling completion_fn on transaction completion https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1509
netlink: fix use-after-free calling completion_fn on transaction completion
2026-10-04 23:10:41 -05:00 · 2026-10-04 23:07:53 -05:00 · 2026-10-04 08:52:00 -05:00 · 2026-10-04 08:52:02 -05:00 · 2026-10-04 08:48:38 -05:00 · 2026-10-04 08:47:30 -05:00
6 changed files with 73 additions and 7 deletions
@@ -10,6 +10,7 @@ BusName=org.freedesktop.ModemManager1
 ExecStart=@sbindir@/ModemManager
 StandardError=null
 Restart=on-abort
+KillMode=mixed
 CapabilityBoundingSet=CAP_SYS_ADMIN CAP_NET_ADMIN
 ProtectSystem=true
 ProtectHome=true
@@ -641,9 +641,10 @@ ip_configuration_async_cleanup (GTask *task)

    ctx  = g_task_get_task_data (task);

-    g_assert (ctx->async_slaac_cancellation_id);
-    g_cancellable_disconnect (g_task_get_cancellable (task), ctx->async_slaac_cancellation_id);
-    ctx->async_slaac_cancellation_id = 0;
+    if (ctx->async_slaac_cancellation_id) {
+        g_cancellable_disconnect (g_task_get_cancellable (task), ctx->async_slaac_cancellation_id);
+        ctx->async_slaac_cancellation_id = 0;
+    }

    g_assert (ctx->async_slaac_notification_id);
    if (g_signal_handler_is_connected (ctx->mbim, ctx->async_slaac_notification_id))
@@ -670,6 +671,12 @@ static void
 ip_configuration_async_cancelled (GCancellable *cancellable,
                                  GTask *task)
 {
+    ConnectContext *ctx;
+
+    /* Avoid the explicit g_cancellable_disconnect(), which would deadlock */
+    ctx = g_task_get_task_data (task);
+    ctx->async_slaac_cancellation_id = 0;
+
    ip_configuration_async_cleanup (task);
    g_task_return_new_error (task, MM_CORE_ERROR, MM_CORE_ERROR_CANCELLED,
                             "Cancelled waiting for SLAAC notification");
@@ -11642,6 +11642,7 @@ all_call_status_indication_cb (QmiClientVoice                        *client,
                case QMI_VOICE_CALL_STATE_CC_IN_PROGRESS:
                    call_info->state = MM_CALL_STATE_DIALING;
                    break;
+                case QMI_VOICE_CALL_STATE_PRE_ALERTING:
                case QMI_VOICE_CALL_STATE_ALERTING:
                    call_info->state = MM_CALL_STATE_RINGING_OUT;
                    break;
@@ -12085,6 +12086,7 @@ process_get_all_call_info (QmiClientVoice                       *client,
                case QMI_VOICE_CALL_STATE_CC_IN_PROGRESS:
                    call_info->state = MM_CALL_STATE_DIALING;
                    break;
+                case QMI_VOICE_CALL_STATE_PRE_ALERTING:
                case QMI_VOICE_CALL_STATE_ALERTING:
                    call_info->state = MM_CALL_STATE_RINGING_OUT;
                    break;
@@ -225,7 +225,9 @@ mm_cbm_part_new_from_binary_pdu (const guint8  *pdu,
            cbm_part->encoding = MM_SMS_ENCODING_GSM7;
        else if (charset == CBS_DATA_CODING_GENERAL_UCS2)
            cbm_part->encoding = MM_SMS_ENCODING_UCS2;
-    } else {
+    }
+
+    if (cbm_part->encoding == MM_SMS_ENCODING_UNKNOWN) {
        g_set_error (error,
                     MM_CORE_ERROR,
                     MM_CORE_ERROR_FAILED,
@@ -227,11 +227,14 @@ transaction_complete (Transaction     *tr,
                      struct nlmsghdr *hdr,
                      gint             saved_errno)
 {
-    GTask *task;
-    guint32 sequence_id;
+    GTask   *task;
+    guint32  sequence_id;
+    MsgFunc  completion_fn;

+    /* tr is freed by hashtable remove; cache values we need after */
    task = g_steal_pointer (&tr->completion_task);
    sequence_id = tr->sequence_id;
+    completion_fn = tr->completion_fn;

    g_hash_table_remove (tr->self->transactions,
                         GUINT_TO_POINTER (tr->sequence_id));
@@ -239,7 +242,7 @@ transaction_complete (Transaction     *tr,
    if (!saved_errno) {
        GError *error = NULL;

-        if (!tr->completion_fn (task, hdr, &error))
+        if (!completion_fn (task, hdr, &error))
            g_task_return_error (task, error);
    } else {
        g_task_return_new_error (task, G_IO_ERROR, g_io_error_from_errno (saved_errno),
@@ -161,6 +161,54 @@ test_cbm_eu (void)
    g_assert_cmpstr (mm_cbm_part_get_language (part), ==, "en");
 }

+static void
+test_cbm_unhandled_data_coding_group_general_charset_reserved (void)
+{
+    g_autoptr(GError) err = NULL;
+    g_autoptr(MMCbmPart) part = NULL;
+
+    static const guint8 pdu [] = {
+        0x63, 0x40, 0x00, 0x32, 0x5C, 0x14,
+        0x00, 0x20, 0x04, 0x1f, 0x04, 0x40, 0x04, 0x3e,
+        0x04, 0x42, 0x04, 0x4f, 0x04, 0x33, 0x04, 0x3e,
+        0x04, 0x3c, 0x00, 0x20, 0x04, 0x34, 0x04, 0x3d,
+        0x04, 0x4f, 0x00, 0x20, 0x04, 0x54, 0x00, 0x20,
+        0x04, 0x32, 0x04, 0x38, 0x04, 0x41, 0x04, 0x3e,
+        0x04, 0x3a, 0x04, 0x30, 0x00, 0x20, 0x04, 0x56,
+        0x04, 0x3c, 0x04, 0x3e, 0x04, 0x32, 0x04, 0x56,
+        0x04, 0x40, 0x04, 0x3d, 0x04, 0x56, 0x04, 0x41,
+        0x04, 0x42, 0x04, 0x4c, 0x00, 0x20, 0x04, 0x40,
+        0x04, 0x30, 0x04, 0x3a, 0x04, 0x35, 0x04, 0x42,
+        0x04, 0x3d, 0x16, 0x01, 0x00, 0x00};
+
+    part = mm_cbm_part_new_from_binary_pdu (pdu, G_N_ELEMENTS (pdu), NULL, &err);
+    g_assert_error (err, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
+}
+
+static void
+test_cbm_unhandled_data_coding_group_general_charset_8bit (void)
+{
+    g_autoptr(GError) err = NULL;
+    g_autoptr(MMCbmPart) part = NULL;
+
+    static const guint8 pdu [] = {
+        0x63, 0x40, 0x00, 0x32, 0x54, 0x14,
+        0x00, 0x20, 0x04, 0x1f, 0x04, 0x40, 0x04, 0x3e,
+        0x04, 0x42, 0x04, 0x4f, 0x04, 0x33, 0x04, 0x3e,
+        0x04, 0x3c, 0x00, 0x20, 0x04, 0x34, 0x04, 0x3d,
+        0x04, 0x4f, 0x00, 0x20, 0x04, 0x54, 0x00, 0x20,
+        0x04, 0x32, 0x04, 0x38, 0x04, 0x41, 0x04, 0x3e,
+        0x04, 0x3a, 0x04, 0x30, 0x00, 0x20, 0x04, 0x56,
+        0x04, 0x3c, 0x04, 0x3e, 0x04, 0x32, 0x04, 0x56,
+        0x04, 0x40, 0x04, 0x3d, 0x04, 0x56, 0x04, 0x41,
+        0x04, 0x42, 0x04, 0x4c, 0x00, 0x20, 0x04, 0x40,
+        0x04, 0x30, 0x04, 0x3a, 0x04, 0x35, 0x04, 0x42,
+        0x04, 0x3d, 0x16, 0x01, 0x00, 0x00};
+
+    part = mm_cbm_part_new_from_binary_pdu (pdu, G_N_ELEMENTS (pdu), NULL, &err);
+    g_assert_error (err, MM_CORE_ERROR, MM_CORE_ERROR_FAILED);
+}
+
 static void
 parse_cbm (const char *str, MMCbmPart **part)
 {
@@ -328,6 +376,9 @@ int main (int argc, char **argv)
    g_test_add_func ("/MM/CBM/PDU-Parser/has-7bit-lang", test_cbm_ucs2_with_7bit_lang);
    /* UCS2 message with additional padding: https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/issues/253#note_3355339 */
    g_test_add_func ("/MM/CBM/PDU-Parser/UCS2/pad", test_cbm_ucs2_pad);
+    /* Ensure unhandled encodings don't trigger asserts */
+    g_test_add_func ("/MM/CBM/PDU-Parser/8bit/error", test_cbm_unhandled_data_coding_group_general_charset_8bit);
+    g_test_add_func ("/MM/CBM/PDU-Parser/reserved/error", test_cbm_unhandled_data_coding_group_general_charset_reserved);

    return g_test_run ();
 }
Author	SHA1	Message	Date
Dan Williams	7f0bf17ab5	Merge request !1509 from 'netlink-completion-use-after-free' netlink: fix use-after-free calling completion_fn on transaction completion https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1509	2026-10-04 23:10:41 -05:00
Dan Williams	9317cbc470	netlink: fix use-after-free calling completion_fn on transaction completion 'tr' is freed when removed from the hash table so we must cache any of its member values that we need after removal. Re-implementation of https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1442 Signed-off-by: Dan Williams <dan@ioncontrol.co>	2026-10-04 23:07:53 -05:00
Dan Williams	b422bd58ef	Merge request !1507 from 'CVE-2026-95622-new' CVE-2026-95622: cbm: don't assert handling unsupported encodings (8-bit and reserved) https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1507	2026-10-04 08:52:00 -05:00
Dan Williams	ee761d009d	CVE-2026-95622: cbm: don't assert handling unsupported encodings (8-bit and reserved) A cell broadcast message using unsupported encodings would trigger an assert in ModemManager and kill the process. A compromised base station or modem firmware could craft or pass through these messages. Reported by Yehia Ezzat and Keith Linneman Impact: Moderate CVSS:3.1/AV:A/AC:L/PR:N/UI:N/S:U/C:N/I:N/A:H (6.5 Medium) CWE-617 (Reachable Assertion) Signed-off-by: Dan Williams <dan@ioncontrol.co>	2026-10-04 08:52:02 -05:00
Dan Williams	faa5642962	Merge request !1508 from 'qmi-add-pre-alerting' broadband-modem-qmi: handle new QMI_VOICE_CALL_STATE_PRE_ALERTING https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1508	2026-10-04 08:48:38 -05:00
Dan Williams	ac03e70426	broadband-modem-qmi: handle new QMI_VOICE_CALL_STATE_PRE_ALERTING From https://gitlab.freedesktop.org/mobile-broadband/libqmi/-/merge_requests/477 Signed-off-by: Dan Williams <dan@ioncontrol.co>	2026-10-04 08:47:30 -05:00
Dan Williams	338ecc06f2	Merge request !1484 from 'service-killmode-mixed' service: don't SIGTERM the QMI/MBIM proxies together with the daemon https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1484	2026-09-22 12:34:34 -05:00
Semyon Maryasin	225e66fea9	service: don't SIGTERM the QMI/MBIM proxies together with the daemon The qmi-proxy/mbim-proxy processes spawned by the daemon live in the service's cgroup, so with the default KillMode=control-group systemd sends SIGTERM to them at the same time as to the daemon on every unit stop. The proxy exits immediately, tearing the control channel from under the daemon while it is still disabling the modems: <msg> caught signal, shutting down... <msg> [modem0] state changed (connected -> disabling) <msg> [modem0] port 'cdc-wdm0' no longer controllable, reprobing <wrn> [/dev/cdc-wdm0] MBIM error: Device must be open to send commands <wrn> shutdown failed: timeout waiting for sleep preparation to complete The consequences of losing the proxy mid-shutdown: - the modem disable/disconnect never reaches the device, so an MBIM session with an active bearer is left open in the firmware (observed to occasionally wedge the MBIM function of a Sierra EM7565 until a firmware reset); - the port teardown stalls waiting on the dead proxy until the shutdown timeout, so stopping the unit always takes the full MAX_SHUTDOWN_TIME_SECS instead of a second or two; - the port invalidation triggers the device reprobe path in the middle of the shutdown. With KillMode=mixed only the daemon receives SIGTERM; the proxies survive long enough for a clean disconnect and MBIM/QMI close, and whatever remains in the cgroup afterwards (an idle, stateless proxy) is killed when the unit finally stops. Signed-off-by: Semyon Maryasin <simeon@maryasin.name>	2026-09-22 12:31:25 -05:00
Dan Williams	efb6a9833e	Merge request !1495 from 'fix-cancellation' bearer-mbim: fix cancellation when waiting for async SLAAC https://gitlab.freedesktop.org/mobile-broadband/ModemManager/-/merge_requests/1495	2026-09-20 15:52:15 -05:00
Aleksander Morgado	07893109ea	bearer-mbim: avoid deadlock when cancelling async SLAAC logic g_cancellable_disconnect() must not be called from within the cancellable callback, or it will deadlock. Signed-off-by: Aleksander Morgado <aleksandermj@google.com>	2026-09-20 15:49:48 -05:00