From 471d385cd183221427b7e58a5afe0deac0ecc591 Mon Sep 17 00:00:00 2001 From: Lubomir Rintel Date: Tue, 25 Jun 2024 10:49:26 +0200 Subject: [PATCH] Add patches for a couple of bugs that make static analysis unhappy Real bugs, but no security implications. Let's check the boxes and fix them, to reduce the static analysis tooling noise. Resolves: https://issues.redhat.com/browse/RHEL-38991 --- ...i-do-not-leak-a-string-in-error-path.patch | 29 ++++++++++++++ ...x-propagation-of-URAT-response-parse.patch | 38 ++++++++++++++++++ ...fix-a-potential-leak-in-cusd_process.patch | 31 +++++++++++++++ ...mi-fix-a-leak-in-error-handling-path.patch | 30 ++++++++++++++ ...ectel-shared-do-not-leak-name-string.patch | 34 ++++++++++++++++ 0006-port-qmi-fix-array-bound-check.patch | 26 +++++++++++++ ...li-sms-do-not-leak-message_reference.patch | 25 ++++++++++++ ...-bearer-qmi-fix-a-copy-n-paste-error.patch | 27 +++++++++++++ ...i-do-not-leak-access-technology-name.patch | 33 ++++++++++++++++ ...glib-signal-fix-a-copy-n-paste-error.patch | 25 ++++++++++++ ...elpers-do-not-leak-past-PDP-on-error.patch | 39 +++++++++++++++++++ ModemManager.spec | 20 +++++++++- 12 files changed, 355 insertions(+), 2 deletions(-) create mode 100644 0001-shared-qmi-do-not-leak-a-string-in-error-path.patch create mode 100644 0002-ublox-helpers-fix-propagation-of-URAT-response-parse.patch create mode 100644 0003-broadband-modem-fix-a-potential-leak-in-cusd_process.patch create mode 100644 0004-shared-qmi-fix-a-leak-in-error-handling-path.patch create mode 100644 0005-quectel-shared-do-not-leak-name-string.patch create mode 100644 0006-port-qmi-fix-array-bound-check.patch create mode 100644 0007-mmcli-sms-do-not-leak-message_reference.patch create mode 100644 0008-bearer-qmi-fix-a-copy-n-paste-error.patch create mode 100644 0009-sim-qmi-do-not-leak-access-technology-name.patch create mode 100644 0010-libmm-glib-signal-fix-a-copy-n-paste-error.patch create mode 100644 0011-modem-helpers-do-not-leak-past-PDP-on-error.patch diff --git a/0001-shared-qmi-do-not-leak-a-string-in-error-path.patch b/0001-shared-qmi-do-not-leak-a-string-in-error-path.patch new file mode 100644 index 0000000..7459ad8 --- /dev/null +++ b/0001-shared-qmi-do-not-leak-a-string-in-error-path.patch @@ -0,0 +1,29 @@ +From 4c28ea13d9b1e75d0bc0dfa4c588b478635c12c0 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Thu, 23 May 2024 14:43:15 +0200 +Subject: [PATCH 01/11] shared-qmi: do not leak a string in error path + +--- + src/mm-shared-qmi.c | 5 +++-- + 1 file changed, 3 insertions(+), 2 deletions(-) + +diff --git a/src/mm-shared-qmi.c b/src/mm-shared-qmi.c +index 8da4ecc69..923c6b51a 100644 +--- a/src/mm-shared-qmi.c ++++ b/src/mm-shared-qmi.c +@@ -4905,9 +4905,10 @@ loc_location_get_server_indication_cb (QmiClientLoc *client, + str = g_strdup (""); + + out: +- if (error) ++ if (error) { ++ g_free (str); + g_task_return_error (task, error); +- else { ++ } else { + g_assert (str); + g_task_return_pointer (task, str, g_free); + } +-- +2.45.2 + diff --git a/0002-ublox-helpers-fix-propagation-of-URAT-response-parse.patch b/0002-ublox-helpers-fix-propagation-of-URAT-response-parse.patch new file mode 100644 index 0000000..3bfe541 --- /dev/null +++ b/0002-ublox-helpers-fix-propagation-of-URAT-response-parse.patch @@ -0,0 +1,38 @@ +From d550a24ffdae4492e9f9e18fdf294adcda28fb65 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Thu, 23 May 2024 14:45:22 +0200 +Subject: [PATCH 02/11] ublox/helpers: fix propagation of +URAT response parse + error + +We have been constructing a GError too late, just leaking it instead of +propagating. +--- + src/plugins/ublox/mm-modem-helpers-ublox.c | 9 ++++----- + 1 file changed, 4 insertions(+), 5 deletions(-) + +diff --git a/src/plugins/ublox/mm-modem-helpers-ublox.c b/src/plugins/ublox/mm-modem-helpers-ublox.c +index 0fd1c5b0b..ffb1374de 100644 +--- a/src/plugins/ublox/mm-modem-helpers-ublox.c ++++ b/src/plugins/ublox/mm-modem-helpers-ublox.c +@@ -1813,14 +1813,13 @@ mm_ublox_parse_urat_read_response (const gchar *response, + } + + out: +- if (inner_error) { +- g_propagate_error (error, inner_error); +- return FALSE; +- } +- + if (allowed == MM_MODEM_MODE_NONE) { + inner_error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_FAILED, + "Couldn't parse +URAT response: %s", response); ++ } ++ ++ if (inner_error) { ++ g_propagate_error (error, inner_error); + return FALSE; + } + +-- +2.45.2 + diff --git a/0003-broadband-modem-fix-a-potential-leak-in-cusd_process.patch b/0003-broadband-modem-fix-a-potential-leak-in-cusd_process.patch new file mode 100644 index 0000000..cce513c --- /dev/null +++ b/0003-broadband-modem-fix-a-potential-leak-in-cusd_process.patch @@ -0,0 +1,31 @@ +From eebd31b92e4e60078911decd87a8c6e658d20cd1 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Thu, 23 May 2024 14:54:03 +0200 +Subject: [PATCH 03/11] broadband-modem: fix a potential leak in + cusd_process_string() error handling + +On error, *converted may already be allocated and we need to free it +(but not in case we're returning it from the task). +--- + src/mm-broadband-modem.c | 3 ++- + 1 file changed, 2 insertions(+), 1 deletion(-) + +diff --git a/src/mm-broadband-modem.c b/src/mm-broadband-modem.c +index 41906c2e5..b924c3a96 100644 +--- a/src/mm-broadband-modem.c ++++ b/src/mm-broadband-modem.c +@@ -6306,9 +6306,10 @@ out: + if (error) + g_task_return_error (task, error); + else if (converted) +- g_task_return_pointer (task, converted, g_free); ++ g_task_return_pointer (task, g_steal_pointer(&converted), g_free); + else + g_assert_not_reached (); ++ g_clear_pointer (&converted, g_free); + return; + } + +-- +2.45.2 + diff --git a/0004-shared-qmi-fix-a-leak-in-error-handling-path.patch b/0004-shared-qmi-fix-a-leak-in-error-handling-path.patch new file mode 100644 index 0000000..ec46872 --- /dev/null +++ b/0004-shared-qmi-fix-a-leak-in-error-handling-path.patch @@ -0,0 +1,30 @@ +From 4a3542b2d00c4104e9735af9eea906c703faed20 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Thu, 23 May 2024 14:56:40 +0200 +Subject: [PATCH 04/11] shared-qmi: fix a leak in error handling path + +str needs to be freed in pds_get_agps_config_ready(). +--- + src/mm-shared-qmi.c | 5 +++-- + 1 file changed, 3 insertions(+), 2 deletions(-) + +diff --git a/src/mm-shared-qmi.c b/src/mm-shared-qmi.c +index 923c6b51a..789b92a4d 100644 +--- a/src/mm-shared-qmi.c ++++ b/src/mm-shared-qmi.c +@@ -4795,9 +4795,10 @@ pds_get_agps_config_ready (QmiClientPds *client, + str = g_strdup (""); + + out: +- if (error) ++ if (error) { ++ g_free (str); + g_task_return_error (task, error); +- else { ++ } else { + g_assert (str); + g_task_return_pointer (task, str, g_free); + } +-- +2.45.2 + diff --git a/0005-quectel-shared-do-not-leak-name-string.patch b/0005-quectel-shared-do-not-leak-name-string.patch new file mode 100644 index 0000000..2a1fd18 --- /dev/null +++ b/0005-quectel-shared-do-not-leak-name-string.patch @@ -0,0 +1,34 @@ +From 84941606219fe440f59fbbc6057ab3782c09bba4 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Sun, 26 May 2024 23:42:41 +0200 +Subject: [PATCH] quectel/shared: do not leak name string + +--- + src/plugins/quectel/mm-shared-quectel.c | 3 ++- + 1 file changed, 2 insertions(+), 1 deletion(-) + +diff --git a/src/plugins/quectel/mm-shared-quectel.c b/src/plugins/quectel/mm-shared-quectel.c +index 816a570..acd7c3c 100644 +--- a/src/plugins/quectel/mm-shared-quectel.c ++++ b/src/plugins/quectel/mm-shared-quectel.c +@@ -285,8 +285,8 @@ quectel_at_port_get_firmware_revision_ready (MMBaseModem *self, + MMFirmwareUpdateSettings *update_settings; + MMModemFirmwareUpdateMethod update_methods; + const gchar *revision; +- const gchar *name; + const gchar *id; ++ gchar *name; + g_autoptr(GPtrArray) ids = NULL; + GError *error = NULL; + +@@ -309,6 +309,7 @@ quectel_at_port_get_firmware_revision_ready (MMBaseModem *self, + mm_obj_dbg (self, "revision %s converted to modem name %s", revision, name); + id = (const gchar *) g_ptr_array_index (ids, 0); + g_ptr_array_insert (ids, 0, g_strdup_printf ("%s&NAME_%s", id, name)); ++ g_free (name); + } + + mm_firmware_update_settings_set_device_ids (update_settings, (const gchar **)ids->pdata); +-- +2.45.2 + diff --git a/0006-port-qmi-fix-array-bound-check.patch b/0006-port-qmi-fix-array-bound-check.patch new file mode 100644 index 0000000..8ba7935 --- /dev/null +++ b/0006-port-qmi-fix-array-bound-check.patch @@ -0,0 +1,26 @@ +From 02d893f8abaffa51ad61b0bb6ce67f55de5cd217 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Sun, 26 May 2024 23:54:50 +0200 +Subject: [PATCH 06/11] port-qmi: fix array bound check + +There's an off-by-one error. +--- + src/mm-port-qmi.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/mm-port-qmi.c b/src/mm-port-qmi.c +index f3c64464a..38703bc20 100644 +--- a/src/mm-port-qmi.c ++++ b/src/mm-port-qmi.c +@@ -1641,7 +1641,7 @@ check_data_format_combination (GTask *task) + + /* go on to the next supported combination */ + for (++ctx->data_format_combination_i; +- ctx->data_format_combination_i <= (gint)G_N_ELEMENTS (data_format_combinations); ++ ctx->data_format_combination_i < (gint)G_N_ELEMENTS (data_format_combinations); + ctx->data_format_combination_i++) { + const DataFormatCombination *combination; + g_autofree gchar *kernel_data_mode_str = NULL; +-- +2.45.2 + diff --git a/0007-mmcli-sms-do-not-leak-message_reference.patch b/0007-mmcli-sms-do-not-leak-message_reference.patch new file mode 100644 index 0000000..9c4121e --- /dev/null +++ b/0007-mmcli-sms-do-not-leak-message_reference.patch @@ -0,0 +1,25 @@ +From 7658e4be92a26c5e0245a3169fb61922f0e96811 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Sun, 26 May 2024 23:58:41 +0200 +Subject: [PATCH 07/11] mmcli/sms: do not leak message_reference + +--- + cli/mmcli-sms.c | 2 ++ + 1 file changed, 2 insertions(+) + +diff --git a/cli/mmcli-sms.c b/cli/mmcli-sms.c +index 6586d0373..46bc95ef8 100644 +--- a/cli/mmcli-sms.c ++++ b/cli/mmcli-sms.c +@@ -191,6 +191,8 @@ print_sms_info (MMSms *sms) + mmcli_output_string (MMC_F_SMS_PROPERTIES_DELIVERY_STATE, delivery_state); + mmcli_output_string (MMC_F_SMS_PROPERTIES_DISCH_TIMESTAMP, mm_sms_get_discharge_timestamp (sms)); + mmcli_output_dump (); ++ ++ g_free (message_reference); + } + + static void +-- +2.45.2 + diff --git a/0008-bearer-qmi-fix-a-copy-n-paste-error.patch b/0008-bearer-qmi-fix-a-copy-n-paste-error.patch new file mode 100644 index 0000000..11e5cf9 --- /dev/null +++ b/0008-bearer-qmi-fix-a-copy-n-paste-error.patch @@ -0,0 +1,27 @@ +From f8f41b677a234f98085854253b2a1a42b0f09d6f Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Sun, 26 May 2024 23:59:47 +0200 +Subject: [PATCH 08/11] bearer/qmi: fix a copy'n'paste error + +Probably not a real issue, given if there's a password there's probably +an user name too. +--- + src/mm-bearer-qmi.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/src/mm-bearer-qmi.c b/src/mm-bearer-qmi.c +index 504134759..4e64e220c 100644 +--- a/src/mm-bearer-qmi.c ++++ b/src/mm-bearer-qmi.c +@@ -1330,7 +1330,7 @@ build_start_network_input (ConnectContext *ctx) + if (ctx->auth != QMI_WDS_AUTHENTICATION_NONE) { + if (ctx->user) + qmi_message_wds_start_network_input_set_username (input, ctx->user, NULL); +- if (ctx->user) ++ if (ctx->password) + qmi_message_wds_start_network_input_set_password (input, ctx->password, NULL); + } + } +-- +2.45.2 + diff --git a/0009-sim-qmi-do-not-leak-access-technology-name.patch b/0009-sim-qmi-do-not-leak-access-technology-name.patch new file mode 100644 index 0000000..9d939da --- /dev/null +++ b/0009-sim-qmi-do-not-leak-access-technology-name.patch @@ -0,0 +1,33 @@ +From 6a11c906d0604748db9a81bf470c821db2b862fb Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Mon, 27 May 2024 00:03:23 +0200 +Subject: [PATCH 09/11] sim/qmi: do not leak access technology name + +--- + src/mm-sim-qmi.c | 8 +++++--- + 1 file changed, 5 insertions(+), 3 deletions(-) + +diff --git a/src/mm-sim-qmi.c b/src/mm-sim-qmi.c +index 03a442403..1d24c23b4 100644 +--- a/src/mm-sim-qmi.c ++++ b/src/mm-sim-qmi.c +@@ -991,11 +991,13 @@ set_preferred_networks_reload_ready (MMBaseSim *self, + } + /* Check if there are access technology bits requested but unset */ + if ((loaded_act & set_act) != set_act) { +- MMModemAccessTechnology unset = set_act & ~loaded_act; ++ MMModemAccessTechnology unset = set_act & ~loaded_act; ++ gchar *act; + ++ act = mm_modem_access_technology_build_string_from_mask (unset); + mm_obj_warn (self, "access technologies '%s' not set for operator code '%s'", +- mm_modem_access_technology_build_string_from_mask (unset), +- set_op_code); ++ act, set_op_code); ++ g_free (act); + error = g_error_new (MM_CORE_ERROR, MM_CORE_ERROR_UNSUPPORTED, + "Access technology unsupported by modem or SIM"); + break; +-- +2.45.2 + diff --git a/0010-libmm-glib-signal-fix-a-copy-n-paste-error.patch b/0010-libmm-glib-signal-fix-a-copy-n-paste-error.patch new file mode 100644 index 0000000..f2c0147 --- /dev/null +++ b/0010-libmm-glib-signal-fix-a-copy-n-paste-error.patch @@ -0,0 +1,25 @@ +From 91f7bbf85b1fccc0c3297acf4203f32b1f195397 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Mon, 27 May 2024 00:04:38 +0200 +Subject: [PATCH 10/11] libmm-glib/signal: fix a copy'n'paste error + +--- + libmm-glib/mm-signal.c | 2 +- + 1 file changed, 1 insertion(+), 1 deletion(-) + +diff --git a/libmm-glib/mm-signal.c b/libmm-glib/mm-signal.c +index e71cd24ba..f8d8c4aa5 100644 +--- a/libmm-glib/mm-signal.c ++++ b/libmm-glib/mm-signal.c +@@ -379,7 +379,7 @@ mm_signal_get_string (MMSignal *self) + g_string_append_printf (printable, "%serror rate: %f %%", printable->len ? ", " : "", self->priv->error_rate); + if (self->priv->rscp != MM_SIGNAL_UNKNOWN) + g_string_append_printf (printable, "%sRSCP: %f dBm", printable->len ? ", " : "", self->priv->rscp); +- if (self->priv->rscp != MM_SIGNAL_UNKNOWN) ++ if (self->priv->rsrp != MM_SIGNAL_UNKNOWN) + g_string_append_printf (printable, "%sRSRP: %f dBm", printable->len ? ", " : "", self->priv->rsrp); + if (self->priv->rsrq != MM_SIGNAL_UNKNOWN) + g_string_append_printf (printable, "%sRSRQ: %f dB", printable->len ? ", " : "", self->priv->rsrq); +-- +2.45.2 + diff --git a/0011-modem-helpers-do-not-leak-past-PDP-on-error.patch b/0011-modem-helpers-do-not-leak-past-PDP-on-error.patch new file mode 100644 index 0000000..cf53f97 --- /dev/null +++ b/0011-modem-helpers-do-not-leak-past-PDP-on-error.patch @@ -0,0 +1,39 @@ +From a877ed8015a4fcfb05961b5bfe9d03f47cdc55a5 Mon Sep 17 00:00:00 2001 +From: Lubomir Rintel +Date: Mon, 27 May 2024 00:06:45 +0200 +Subject: [PATCH 11/11] modem-helpers: do not leak past PDP on error + +If CID parsing from the +CGDCONT response fails, the very last PDP +structure allocated is not put on the list yet and therefore +mm_3gpp_pdp_context_list_free() wouldn't free it. + +Let's put it on the list first, as to not leak it on error. +--- + src/mm-modem-helpers.c | 4 ++-- + 1 file changed, 2 insertions(+), 2 deletions(-) + +diff --git a/src/mm-modem-helpers.c b/src/mm-modem-helpers.c +index 8dad5a398..9f0c8a8ce 100644 +--- a/src/mm-modem-helpers.c ++++ b/src/mm-modem-helpers.c +@@ -1870,6 +1870,8 @@ mm_3gpp_parse_cgdcont_read_response (const gchar *reply, + MM3gppPdpContext *pdp; + + pdp = g_slice_new0 (MM3gppPdpContext); ++ list = g_list_prepend (list, pdp); ++ + if (!mm_get_uint_from_match_info (match_info, 1, &pdp->cid)) { + inner_error = g_error_new (MM_CORE_ERROR, + MM_CORE_ERROR_FAILED, +@@ -1879,8 +1881,6 @@ mm_3gpp_parse_cgdcont_read_response (const gchar *reply, + } + pdp->pdp_type = ip_family; + pdp->apn = mm_get_string_unquoted_from_match_info (match_info, 3); +- +- list = g_list_prepend (list, pdp); + } + + g_free (str); +-- +2.45.2 + diff --git a/ModemManager.spec b/ModemManager.spec index c4053d7..ce6b66c 100644 --- a/ModemManager.spec +++ b/ModemManager.spec @@ -5,12 +5,25 @@ Name: ModemManager Version: 1.22.0 -Release: 4%{?dist} +Release: 5%{?dist} Summary: Mobile broadband modem management service License: GPL-2.0-or-later URL: http://www.freedesktop.org/wiki/Software/ModemManager/ Source: https://gitlab.com/linux-mobile-broadband/ModemManager/-/archive/%{version}/%{name}-%{version}.tar.bz2 +# All of these are applied upstream. Can frop on rebase to 1.24 +Patch0: 0001-shared-qmi-do-not-leak-a-string-in-error-path.patch +Patch1: 0002-ublox-helpers-fix-propagation-of-URAT-response-parse.patch +Patch2: 0003-broadband-modem-fix-a-potential-leak-in-cusd_process.patch +Patch3: 0004-shared-qmi-fix-a-leak-in-error-handling-path.patch +Patch4: 0005-quectel-shared-do-not-leak-name-string.patch +Patch5: 0006-port-qmi-fix-array-bound-check.patch +Patch6: 0007-mmcli-sms-do-not-leak-message_reference.patch +Patch7: 0008-bearer-qmi-fix-a-copy-n-paste-error.patch +Patch8: 0009-sim-qmi-do-not-leak-access-technology-name.patch +Patch9: 0010-libmm-glib-signal-fix-a-copy-n-paste-error.patch +Patch10: 0011-modem-helpers-do-not-leak-past-PDP-on-error.patch + # For mbim-proxy and qmi-proxy Requires: libmbim-utils Requires: libqmi-utils @@ -190,6 +203,9 @@ cp -a cli/mmcli-completion %{buildroot}%{_datadir}/bash-completion/completions/m %changelog +* Tue Jun 25 2024 Lubomir Rintel - 1.22.0-5 +- Add patches for a couple of bugs that make static analysis unhappy (RHEL-38991) + * Mon Jun 24 2024 Troy Dawson - 1.22.0-4 - Bump release for June 2024 mass rebuild @@ -202,7 +218,7 @@ cp -a cli/mmcli-completion %{buildroot}%{_datadir}/bash-completion/completions/m * Tue Jan 09 2024 Dennis Gilmore - 1.22.0-1 - update to 1.22.0 -* Wed Jul 30 2023 Tao Jin - 1.20.6-3 +* Sun Jul 30 2023 Tao Jin - 1.20.6-3 - Rebuilt for RHBZ#2226577 * Wed Jul 19 2023 Fedora Release Engineering - 1.20.6-2