From ef567805bb35ce872d01bff694364e91be14baad Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 13:23:31 +0100 Subject: [PATCH 01/15] shared: reject '%' from nm_utils_ifname_valid() for kernel names Generally, it's dangerous to reject values that were accepted previously. This will lead to NetworkManager being unable to load a profile from disk, which was loadable previously. On the other hand, kernel would not have treated this setting as it was intended. So, I would argue that the such a setting was not working (as intended) anyway. We can only hope that users don't configure arbitrary interface names. It generally isn't a good idea to do, so "breaking" such things is less of a concern. --- shared/nm-glib-aux/nm-shared-utils.c | 30 ++++++++++++++++++++++++++-- 1 file changed, 28 insertions(+), 2 deletions(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index c9bfbb07fe..ace652f4c3 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -4126,8 +4126,34 @@ nm_utils_ifname_valid_kernel (const char *name, GError **error) return FALSE; } +/*****************************************************************************/ + static gboolean -_nm_utils_ifname_valid_ovs (const char* name, GError **error) +_nm_utils_ifname_valid_kernel (const char *name, GError **error) +{ + if (!nm_utils_ifname_valid_kernel (name, error)) + return FALSE; + + if (strchr (name, '%')) { + /* Kernel's dev_valid_name() accepts (almost) any binary up to 15 chars. + * However, '%' is treated special as a format specifier. Try + * + * ip link add 'dummy%dx' type dummy + * + * Don't allow that for "connection.interface-name", which either + * matches an existing netdev name (thus, it cannot have a '%') or + * is used to configure a name (in which case we don't want kernel + * to replace the format specifier). */ + g_set_error_literal (error, NM_UTILS_ERROR, NM_UTILS_ERROR_UNKNOWN, + _("'%%' is not allowed in interface names")); + return FALSE; + } + + return TRUE; +} + +static gboolean +_nm_utils_ifname_valid_ovs (const char *name, GError **error) { const char *ch; @@ -4169,7 +4195,7 @@ nm_utils_ifname_valid (const char* name, switch (type) { case NMU_IFACE_KERNEL: - return nm_utils_ifname_valid_kernel (name, error); + return _nm_utils_ifname_valid_kernel (name, error); case NMU_IFACE_OVS: return _nm_utils_ifname_valid_ovs (name, error); } From d4d9e9e7bb2d3934008d94027945e0dcacd07dff Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 17:12:51 +0100 Subject: [PATCH 02/15] shared: reject reserved names from "connection.interface-name" "all" and "default" never works. "bonding_masters" works if you unload the bonding module. Well, that should not really be called working... Reject these names. --- shared/nm-glib-aux/nm-shared-utils.c | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index ace652f4c3..6e1bcd74cb 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -4149,6 +4149,19 @@ _nm_utils_ifname_valid_kernel (const char *name, GError **error) return FALSE; } + if (NM_IN_STRSET (name, "all", + "default", + "bonding_masters")) { + /* Certain names are not allowed. The "all" and "default" names are reserved + * due to their directories in "/proc/sys/net/ipv4/conf/" and "/proc/sys/net/ipv6/conf/". + * + * Also, there is "/sys/class/net/bonding_masters" file. + */ + nm_utils_error_set (error, NM_UTILS_ERROR_UNKNOWN, + _("'%s' is not allowed as interface name"), name); + return FALSE; + } + return TRUE; } From 0718098dcbfcf4af027a181a50057facdf426b08 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 17:20:44 +0100 Subject: [PATCH 03/15] libnm: use nm_utils_hash_values_to_array() to implement nm_connection_get_settings() --- libnm-core/nm-connection.c | 55 ++++++++++++-------------------------- 1 file changed, 17 insertions(+), 38 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index 072b568ed6..1c3f2d0e0a 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -2316,16 +2316,21 @@ nm_connection_is_type (NMConnection *connection, const char *type) } static int -_for_each_sort (NMSetting **p_a, NMSetting **p_b, void *unused) +_get_settings_sort (gconstpointer p_a, gconstpointer p_b, gpointer unused) { - NMSetting *a = *p_a; - NMSetting *b = *p_b; - int c; + NMSetting *a = *((NMSetting **) p_a); + NMSetting *b = *((NMSetting **) p_b); - c = _nm_setting_compare_priority (a, b); - if (c != 0) - return c; - return strcmp (nm_setting_get_name (a), nm_setting_get_name (b)); + nm_assert (NM_IS_SETTING (a)); + nm_assert (NM_IS_SETTING (b)); + nm_assert (a != b); + nm_assert (G_OBJECT_TYPE (a) != G_OBJECT_TYPE (b)); + + NM_CMP_RETURN (_nm_setting_compare_priority (a, b)); + NM_CMP_DIRECT_STRCMP (nm_setting_get_name (a), nm_setting_get_name (b)); + + nm_assert_not_reached (); + return 0; } /** @@ -2348,38 +2353,12 @@ NMSetting ** nm_connection_get_settings (NMConnection *connection, guint *out_length) { - NMConnectionPrivate *priv; - NMSetting **arr; - GHashTableIter iter; - NMSetting *setting; - guint i, size; - g_return_val_if_fail (NM_IS_CONNECTION (connection), NULL); - priv = NM_CONNECTION_GET_PRIVATE (connection); - - size = g_hash_table_size (priv->settings); - - if (!size) { - NM_SET_OUT (out_length, 0); - return NULL; - } - - arr = g_new (NMSetting *, size + 1); - - g_hash_table_iter_init (&iter, priv->settings); - for (i = 0; g_hash_table_iter_next (&iter, NULL, (gpointer *) &setting); i++) - arr[i] = setting; - nm_assert (i == size); - arr[size] = NULL; - - /* sort the settings. This has an effect on the order in which keyfile - * prints them. */ - if (size > 1) - g_qsort_with_data (arr, size, sizeof (NMSetting *), (GCompareDataFunc) _for_each_sort, NULL); - - NM_SET_OUT (out_length, size); - return arr; + return (NMSetting **) nm_utils_hash_values_to_array (NM_CONNECTION_GET_PRIVATE (connection)->settings, + _get_settings_sort, + NULL, + out_length); } /** From f3dd41ad7ef45f888c9f605015869e362fa25a4a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 17:27:27 +0100 Subject: [PATCH 04/15] libnm: validate settings in _nm_connection_verify() in defined order Fully sort the settings in _nm_connection_verify(). Previously, only the NMSettingConnection instance was sorted first (as required). The remaining settings were in undefined order. That means, we would validate settings in undefined order, and if multiple settings have an issue, the reported error would be undefined. Instead, use nm_connection_get_settings() which fully sorts the settings (and of course, sorts NMSettingConnection first as we require it). Also, this way we no longer need to allocate multiple GSList instances but only malloc() one array large enough to contain all settings. --- libnm-core/nm-connection.c | 39 ++++++++++---------------------------- 1 file changed, 10 insertions(+), 29 deletions(-) diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index 1c3f2d0e0a..5c54c5140f 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -1404,24 +1404,19 @@ nm_connection_verify (NMConnection *connection, GError **error) NMSettingVerifyResult _nm_connection_verify (NMConnection *connection, GError **error) { - NMConnectionPrivate *priv; - NMSettingConnection *s_con; NMSettingIPConfig *s_ip4, *s_ip6; NMSettingProxy *s_proxy; - GHashTableIter iter; - gpointer value; - GSList *all_settings = NULL, *setting_i; + gs_free NMSetting **settings = NULL; gs_free_error GError *normalizable_error = NULL; NMSettingVerifyResult normalizable_error_type = NM_SETTING_VERIFY_SUCCESS; + guint i; g_return_val_if_fail (NM_IS_CONNECTION (connection), NM_SETTING_VERIFY_ERROR); g_return_val_if_fail (!error || !*error, NM_SETTING_VERIFY_ERROR); - priv = NM_CONNECTION_GET_PRIVATE (connection); - - /* First, make sure there's at least 'connection' setting */ - s_con = nm_connection_get_setting_connection (connection); - if (!s_con) { + settings = nm_connection_get_settings (connection, NULL); + if ( !settings + || !NM_IS_SETTING_CONNECTION (settings[0])) { g_set_error_literal (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_MISSING_SETTING, @@ -1430,25 +1425,13 @@ _nm_connection_verify (NMConnection *connection, GError **error) return NM_SETTING_VERIFY_ERROR; } - /* Build up the list of settings */ - g_hash_table_iter_init (&iter, priv->settings); - while (g_hash_table_iter_next (&iter, NULL, &value)) { - /* Order NMSettingConnection so that it will be verified first. - * The reason is, that errors in this setting might be more fundamental - * and should be checked and reported with higher priority. - */ - if (value == s_con) - all_settings = g_slist_append (all_settings, value); - else - all_settings = g_slist_prepend (all_settings, value); - } - all_settings = g_slist_reverse (all_settings); - - /* Now, run the verify function of each setting */ - for (setting_i = all_settings; setting_i; setting_i = setting_i->next) { + for (i = 0; settings[i]; i++) { GError *verify_error = NULL; NMSettingVerifyResult verify_result; + nm_assert (NM_IS_SETTING (settings[i])); + nm_assert (NM_IS_SETTING_CONNECTION (settings[i]) == (i == 0)); + /* verify all settings. We stop if we find the first non-normalizable * @NM_SETTING_VERIFY_ERROR. If we find normalizable errors we continue * but remember the error to return it to the user. @@ -1456,7 +1439,7 @@ _nm_connection_verify (NMConnection *connection, GError **error) * @NM_SETTING_VERIFY_NORMALIZABLE, so, if we encounter such an error type, * we remember it instead (to return it as output). **/ - verify_result = _nm_setting_verify (NM_SETTING (setting_i->data), connection, &verify_error); + verify_result = _nm_setting_verify (settings[i], connection, &verify_error); if (verify_result == NM_SETTING_VERIFY_NORMALIZABLE || verify_result == NM_SETTING_VERIFY_NORMALIZABLE_ERROR) { if ( verify_result == NM_SETTING_VERIFY_NORMALIZABLE_ERROR @@ -1471,13 +1454,11 @@ _nm_connection_verify (NMConnection *connection, GError **error) } } else if (verify_result != NM_SETTING_VERIFY_SUCCESS) { g_propagate_error (error, verify_error); - g_slist_free (all_settings); g_return_val_if_fail (verify_result == NM_SETTING_VERIFY_ERROR, NM_SETTING_VERIFY_ERROR); return NM_SETTING_VERIFY_ERROR; } g_clear_error (&verify_error); } - g_slist_free (all_settings); s_ip4 = nm_connection_get_setting_ip4_config (connection); s_ip6 = nm_connection_get_setting_ip6_config (connection); From 74e2203e19996dcb6301b2396c82ebb036148171 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 16:29:25 +0100 Subject: [PATCH 05/15] libnm: in find_virtual_interface_name() ensure return value stays alive It's not clear that the returned string is still valid after we unref the GVariant that contains it. Also return the reference to the variant. --- libnm-core/nm-setting-connection.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index 230c06fe2e..6ffa4e4cfb 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -1251,11 +1251,14 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) } static const char * -find_virtual_interface_name (GVariant *connection_dict) +find_virtual_interface_name (GVariant *connection_dict, + GVariant **variant_to_free) { GVariant *setting_dict; const char *interface_name; + nm_assert (variant_to_free && !*variant_to_free); + setting_dict = g_variant_lookup_value (connection_dict, NM_SETTING_BOND_SETTING_NAME, NM_VARIANT_TYPE_SETTING); if (!setting_dict) setting_dict = g_variant_lookup_value (connection_dict, NM_SETTING_BRIDGE_SETTING_NAME, NM_VARIANT_TYPE_SETTING); @@ -1267,11 +1270,12 @@ find_virtual_interface_name (GVariant *connection_dict) if (!setting_dict) return NULL; + *variant_to_free = setting_dict; + /* All of the deprecated virtual interface name properties were named "interface-name". */ if (!g_variant_lookup (setting_dict, "interface-name", "&s", &interface_name)) interface_name = NULL; - g_variant_unref (setting_dict); return interface_name; } @@ -1284,12 +1288,13 @@ nm_setting_connection_set_interface_name (NMSetting *setting, GError **error) { const char *interface_name; + gs_unref_variant GVariant *variant_to_free = NULL; /* For compatibility reasons, if there is an invalid virtual interface name, * we need to make verification fail, even if that virtual name would be * overridden by a valid connection.interface-name. */ - interface_name = find_virtual_interface_name (connection_dict); + interface_name = find_virtual_interface_name (connection_dict, &variant_to_free); if (!interface_name || nm_utils_ifname_valid_kernel (interface_name, NULL)) interface_name = g_variant_get_string (value, NULL); @@ -1308,8 +1313,9 @@ nm_setting_connection_no_interface_name (NMSetting *setting, GError **error) { const char *virtual_interface_name; + gs_unref_variant GVariant *variant_to_free = NULL; - virtual_interface_name = find_virtual_interface_name (connection_dict); + virtual_interface_name = find_virtual_interface_name (connection_dict, &variant_to_free); g_object_set (G_OBJECT (setting), NM_SETTING_CONNECTION_INTERFACE_NAME, virtual_interface_name, NULL); From 41480d48aea10238f48baa34b7c1b1a610fbdf2b Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 17:01:27 +0100 Subject: [PATCH 06/15] libnm: don't validate "connection.interface-name" from "nm-setting-infiniband.c"'s verify() There should not be multiple places to validate the interface-name. The check in "nm-setting-infiniband.c" is unnecessary and wrong. It's unnecessary, because _nm_connection_verify() takes care to first verify the NMSettingConnection instance. It's wrong, because it does not check the property the same way as NMSettingConnection does (e.g. it does not check for valid UTF-8). --- libnm-core/nm-setting-infiniband.c | 46 ++++++++++-------------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/libnm-core/nm-setting-infiniband.c b/libnm-core/nm-setting-infiniband.c index 3efe37a01a..1ad062c776 100644 --- a/libnm-core/nm-setting-infiniband.c +++ b/libnm-core/nm-setting-infiniband.c @@ -211,39 +211,23 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) s_con = nm_connection_get_setting_connection (connection); if (s_con) { const char *interface_name = nm_setting_connection_get_interface_name (s_con); - GError *tmp_error = NULL; - if (!interface_name) - ; - else if (!nm_utils_ifname_valid_kernel (interface_name, &tmp_error)) { - /* report the error for NMSettingConnection:interface-name, because - * it's that property that is invalid -- although we currently verify() - * NMSettingInfiniband. - **/ - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - "'%s': %s", interface_name, tmp_error->message); - g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME); - g_error_free (tmp_error); - return FALSE; - } else { - if (priv->p_key != -1) { - if (!priv->virtual_iface_name) - priv->virtual_iface_name = g_strdup_printf ("%s.%04x", priv->parent, priv->p_key); + if ( interface_name + && priv->p_key != -1) { + if (!priv->virtual_iface_name) + priv->virtual_iface_name = g_strdup_printf ("%s.%04x", priv->parent, priv->p_key); - if (strcmp (interface_name, priv->virtual_iface_name) != 0) { - /* We don't support renaming software infiniband devices. Later we might, but - * for now just reject such connections. - **/ - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - _("interface name of software infiniband device must be '%s' or unset (instead it is '%s')"), - priv->virtual_iface_name, interface_name); - g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME); - return FALSE; - } + if (strcmp (interface_name, priv->virtual_iface_name) != 0) { + /* We don't support renaming software infiniband devices. Later we might, but + * for now just reject such connections. + **/ + g_set_error (error, + NM_CONNECTION_ERROR, + NM_CONNECTION_ERROR_INVALID_PROPERTY, + _("interface name of software infiniband device must be '%s' or unset (instead it is '%s')"), + priv->virtual_iface_name, interface_name); + g_prefix_error (error, "%s.%s: ", NM_SETTING_CONNECTION_SETTING_NAME, NM_SETTING_CONNECTION_INTERFACE_NAME); + return FALSE; } } } From 47a654d398485d574c1ac9df2ff38114a28783f8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 17:36:55 +0100 Subject: [PATCH 07/15] clients: use nm_utils_ifname_valid() to validate interface name in nm_vpn_wireguard_import() We use the filename of the imported .conf file for "connection.interface-name". That follows what `wg-quick` does. However, we also validate that the interface name is valid UTF-8 (otherwise -- as it currently is -- the setting couldn't be send via D-Bus). As such, we have stricter requirements. We want to fail early and tell the user when the filename is unsuitable. Failing later gives a worse user experience, because the failure message about invalid "connection.interface-name" wouldn't make it clear that the filename is wrong. Use the appropriate function to validate "connection.interface-name". Before: $ touch $'./a\344b.conf' $ nmcli connection import type wireguard file $'./a\344b.conf' Error: failed to import './a?b.conf': Failed to create WireGuard connection: connection.interface-name: 'a?b': interface name must be UTF-8 encoded. Now: $ nmcli connection import type wireguard file $'./a\344b.conf' Error: failed to import './a?b.conf': The name of the WireGuard config must be a valid interface name followed by ".conf". --- clients/common/nm-vpn-helpers.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/clients/common/nm-vpn-helpers.c b/clients/common/nm-vpn-helpers.c index e1d8355147..33490c57d5 100644 --- a/clients/common/nm-vpn-helpers.c +++ b/clients/common/nm-vpn-helpers.c @@ -368,13 +368,13 @@ nm_vpn_wireguard_import (const char *filename, memcpy (ifname, cstr, len); ifname[len] = '\0'; - if (nm_utils_ifname_valid_kernel (ifname, NULL)) + if (nm_utils_ifname_valid (ifname, NMU_IFACE_KERNEL, NULL)) ifname_valid = TRUE; } } if (!ifname_valid) { nm_utils_error_set_literal (error, NM_UTILS_ERROR_UNKNOWN, - _("The WireGuard config file must be a valid interface name followed by \".conf\"")); + _("The name of the WireGuard config must be a valid interface name followed by \".conf\"")); return FALSE; } From f725209bb448a2216cbe942c8a3d75df49a7759f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 17:40:17 +0100 Subject: [PATCH 08/15] settings: simplify property setter from GVariant to NMSettingConnection:interface-name The interface-name property has several deprecated aliases, like "bridge.interface-name". For backward compatibility, we keep handling them. In particular, the "missing_from_dbus_fcn" handler is set. This handles the case where GVariant only contains the deprecated form, but not "connection.interface-name". Previously, from_dbus_fcn() would check whether the deprecated form was present, and -- only if that form was invalid -- prefer it. The idea was to fail validation if the deprecated property was invalid. I think that is not necessary. Just completely ignore the deprecated property, if the new property is present. What might make sense is to check whether the deprecated and the new form are both present, that they are identical. However, I don't think that is worth the effort. --- libnm-core/nm-setting-connection.c | 27 --------------------------- libnm-core/tests/test-general.c | 7 ++++--- 2 files changed, 4 insertions(+), 30 deletions(-) diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index 6ffa4e4cfb..6ea3c5fb29 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -1279,32 +1279,6 @@ find_virtual_interface_name (GVariant *connection_dict, return interface_name; } -static gboolean -nm_setting_connection_set_interface_name (NMSetting *setting, - GVariant *connection_dict, - const char *property, - GVariant *value, - NMSettingParseFlags parse_flags, - GError **error) -{ - const char *interface_name; - gs_unref_variant GVariant *variant_to_free = NULL; - - /* For compatibility reasons, if there is an invalid virtual interface name, - * we need to make verification fail, even if that virtual name would be - * overridden by a valid connection.interface-name. - */ - interface_name = find_virtual_interface_name (connection_dict, &variant_to_free); - if (!interface_name || nm_utils_ifname_valid_kernel (interface_name, NULL)) - interface_name = g_variant_get_string (value, NULL); - - g_object_set (G_OBJECT (setting), - NM_SETTING_CONNECTION_INTERFACE_NAME, interface_name, - NULL); - - return TRUE; -} - static gboolean nm_setting_connection_no_interface_name (NMSetting *setting, GVariant *connection_dict, @@ -1765,7 +1739,6 @@ nm_setting_connection_class_init (NMSettingConnectionClass *klass) obj_properties[PROP_INTERFACE_NAME], NM_SETT_INFO_PROPERT_TYPE ( .dbus_type = G_VARIANT_TYPE_STRING, - .from_dbus_fcn = nm_setting_connection_set_interface_name, .missing_from_dbus_fcn = nm_setting_connection_no_interface_name, )); diff --git a/libnm-core/tests/test-general.c b/libnm-core/tests/test-general.c index 532e2c2449..e4baaacfe5 100644 --- a/libnm-core/tests/test-general.c +++ b/libnm-core/tests/test-general.c @@ -4647,7 +4647,7 @@ test_connection_normalize_virtual_iface_name (void) g_variant_unref (setting_dict); g_variant_unref (var); - /* If vlan.interface-name is invalid, deserialization will fail. */ + /* If vlan.interface-name will be ignored. */ NMTST_VARIANT_EDITOR (connection_dict, NMTST_VARIANT_CHANGE_PROPERTY (NM_SETTING_VLAN_SETTING_NAME, "interface-name", @@ -4656,8 +4656,9 @@ test_connection_normalize_virtual_iface_name (void) ); con = _connection_new_from_dbus (connection_dict, &error); - g_assert_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY); - g_clear_error (&error); + nmtst_assert_success (con, error); + g_assert_cmpstr (nm_connection_get_interface_name (con), ==, IFACE_NAME); + g_clear_object (&con); /* If vlan.interface-name is valid, but doesn't match, it will be ignored. */ NMTST_VARIANT_EDITOR (connection_dict, From de19631e9f80480596141291c916605803229c67 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 13:43:15 +0100 Subject: [PATCH 09/15] libnm: remove redundant check from "nm-setting-bond.c"'s validate_ifname() --- libnm-core/nm-setting-bond.c | 3 --- 1 file changed, 3 deletions(-) diff --git a/libnm-core/nm-setting-bond.c b/libnm-core/nm-setting-bond.c index a654e65c39..ffcec1e9af 100644 --- a/libnm-core/nm-setting-bond.c +++ b/libnm-core/nm-setting-bond.c @@ -328,9 +328,6 @@ validate_ip (const char *name, const char *value) static gboolean validate_ifname (const char *name, const char *value) { - if (!value || !value[0]) - return FALSE; - return nm_utils_ifname_valid_kernel (value, NULL); } From aa6bc2868da43199be001e54385c4230e3efc4ca Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 13:50:16 +0100 Subject: [PATCH 10/15] ifcfg-rh: use nm_utils_ifname_valid() for validating interface-name in reader Maybe the reader should not try to add its own validation. It could just read the value, set it in the profile, and let nm_connection_verify() handle it. However: - in this form the code only logs a warning about invalid setting. If we let it come to nm_connection_verify(), the connection profile will be entirely rejected. I think this makes sense, because ifcfg files may be edited by the user and we don't know what is out there. - it's nicer to show a warning that specifically mentions the DEVICE= variable. There error message we get from nm_connection_verify() is no longer aware of ifcfg peculiarities. Instead: use the appropriate validation function. --- src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c index 06ba11a22e..ab73b153a0 100644 --- a/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c +++ b/src/settings/plugins/ifcfg-rh/nms-ifcfg-rh-reader.c @@ -399,7 +399,9 @@ make_connection_setting (const char *file, if (v) { GError *error = NULL; - if (nm_utils_ifname_valid_kernel (v, &error)) { + /* Only validate for NMU_IFACE_KERNEL, because ifcfg plugin anyway + * doesn't support OVS types. */ + if (nm_utils_ifname_valid (v, NMU_IFACE_KERNEL, &error)) { g_object_set (s_con, NM_SETTING_CONNECTION_INTERFACE_NAME, v, NULL); From 07b7c82d04ff294277254b195e1a2fa0ffd8875f Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 14:52:28 +0100 Subject: [PATCH 11/15] libnm: allow _nm_setting_ovs_interface_verify_interface_type() without NMSettingOvsInterface instance _nm_setting_ovs_interface_verify_interface_type() does verify and normalize both. Especially for verify, it's useful to run the operation without having a NMSettingOvsInterface instance, because we might want to know how normalization would react, if we had a NMSettingOvsInterface instance. Allow for that. --- libnm-core/nm-connection-private.h | 1 + libnm-core/nm-connection.c | 1 + libnm-core/nm-setting-ovs-interface.c | 11 ++++++----- 3 files changed, 8 insertions(+), 5 deletions(-) diff --git a/libnm-core/nm-connection-private.h b/libnm-core/nm-connection-private.h index 034c350f84..90d91439bc 100644 --- a/libnm-core/nm-connection-private.h +++ b/libnm-core/nm-connection-private.h @@ -24,6 +24,7 @@ gboolean _nm_connection_verify_required_interface_name (NMConnection *connect GError **error); int _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, + const char *type, NMConnection *connection, gboolean normalize, gboolean *out_modified, diff --git a/libnm-core/nm-connection.c b/libnm-core/nm-connection.c index 5c54c5140f..58c9f0df59 100644 --- a/libnm-core/nm-connection.c +++ b/libnm-core/nm-connection.c @@ -1236,6 +1236,7 @@ _normalize_ovs_interface_type (NMConnection *self) return FALSE; v = _nm_setting_ovs_interface_verify_interface_type (s_ovs_interface, + nm_setting_ovs_interface_get_interface_type (s_ovs_interface), self, TRUE, &modified, diff --git a/libnm-core/nm-setting-ovs-interface.c b/libnm-core/nm-setting-ovs-interface.c index 9a7e1fb2ba..85e77b5443 100644 --- a/libnm-core/nm-setting-ovs-interface.c +++ b/libnm-core/nm-setting-ovs-interface.c @@ -64,31 +64,31 @@ nm_setting_ovs_interface_get_interface_type (NMSettingOvsInterface *self) int _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, + const char *type, NMConnection *connection, gboolean normalize, gboolean *out_modified, const char **normalized_type, GError **error) { - const char *type; const char *type_from_setting = NULL; const char *type_setting = NULL; const char *connection_type; gboolean is_ovs_connection_type; - g_return_val_if_fail (NM_IS_SETTING_OVS_INTERFACE (self), FALSE); if (normalize) { + g_return_val_if_fail (NM_IS_SETTING_OVS_INTERFACE (self), FALSE); g_return_val_if_fail (NM_IS_CONNECTION (connection), FALSE); nm_assert (self == nm_connection_get_setting_ovs_interface (connection)); - } else + } else { + g_return_val_if_fail (!self || NM_IS_SETTING_OVS_INTERFACE (self), FALSE); g_return_val_if_fail (!connection || NM_IS_CONNECTION (connection), FALSE); + } g_return_val_if_fail (!normalized_type || !(*normalized_type), FALSE); NM_SET_OUT (out_modified, FALSE); - type = self->type; - if ( type && !NM_IN_STRSET (type, "internal", "system", "patch", "dpdk")) { g_set_error (error, @@ -296,6 +296,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) } result = _nm_setting_ovs_interface_verify_interface_type (self, + self->type, connection, FALSE, NULL, From a11edd4a82e019cd0666b398827c037bf718a7c2 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 15:33:33 +0100 Subject: [PATCH 12/15] libnm: always return normalized-type from _nm_setting_ovs_interface_verify_interface_type() We should return the chosen type whenever we can verify the setting. Previously, the normalized-type output argument was only set when normalization was actually necessary. On most cases, the caller cares whether the setting verifies and which interface type is chosen. It's much less likely that a caller cares only about the normalized-type if normalization is actually necessary. Whenever we return TRUE (indicating that the setting is valid), also return the chosen interface-type. --- libnm-core/nm-connection-private.h | 2 +- libnm-core/nm-setting-ovs-interface.c | 19 ++++++++++--------- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/libnm-core/nm-connection-private.h b/libnm-core/nm-connection-private.h index 90d91439bc..3c505834b4 100644 --- a/libnm-core/nm-connection-private.h +++ b/libnm-core/nm-connection-private.h @@ -28,7 +28,7 @@ int _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self NMConnection *connection, gboolean normalize, gboolean *out_modified, - const char **normalized_type, + const char **out_normalized_type, GError **error); #endif /* __NM_CONNECTION_PRIVATE_H__ */ diff --git a/libnm-core/nm-setting-ovs-interface.c b/libnm-core/nm-setting-ovs-interface.c index 85e77b5443..114e0205d3 100644 --- a/libnm-core/nm-setting-ovs-interface.c +++ b/libnm-core/nm-setting-ovs-interface.c @@ -68,7 +68,7 @@ _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, NMConnection *connection, gboolean normalize, gboolean *out_modified, - const char **normalized_type, + const char **out_normalized_type, GError **error) { const char *type_from_setting = NULL; @@ -85,9 +85,8 @@ _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, g_return_val_if_fail (!connection || NM_IS_CONNECTION (connection), FALSE); } - g_return_val_if_fail (!normalized_type || !(*normalized_type), FALSE); - NM_SET_OUT (out_modified, FALSE); + NM_SET_OUT (out_normalized_type, NULL); if ( type && !NM_IN_STRSET (type, "internal", "system", "patch", "dpdk")) { @@ -100,8 +99,10 @@ _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, return FALSE; } - if (!connection) + if (!connection) { + NM_SET_OUT (out_normalized_type, type); return TRUE; + } connection_type = nm_connection_get_connection_type (connection); if (!connection_type) { @@ -192,6 +193,7 @@ _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, g_prefix_error (error, "%s.%s: ", NM_SETTING_OVS_INTERFACE_SETTING_NAME, NM_SETTING_OVS_INTERFACE_TYPE); return FALSE; } + NM_SET_OUT (out_normalized_type, type); return TRUE; } type = type_from_setting; @@ -208,16 +210,17 @@ _nm_setting_ovs_interface_verify_interface_type (NMSettingOvsInterface *self, } } - if (type) + if (type) { + NM_SET_OUT (out_normalized_type, type); return TRUE; + } if (is_ovs_connection_type) type = "internal"; else type = "system"; - if (normalized_type) - *normalized_type = type; + NM_SET_OUT (out_normalized_type, type); normalize: if (!normalize) { @@ -309,8 +312,6 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) gs_free_error GError *ifname_error = NULL; const char *ifname = nm_setting_connection_get_interface_name (s_con); - normalized_type = self->type ? self->type : normalized_type; - if ( ifname && !nm_streq0 (normalized_type, "patch") && !nm_utils_ifname_valid (ifname, From 807cddc7546d6da222aed2759215ae93dcb2c8d7 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Mon, 17 Feb 2020 16:44:32 +0100 Subject: [PATCH 13/15] shared: add NMU_IFACE_ANY for nm_utils_ifname_valid() nm_utils_ifname_valid() is to validate "connection.interface-name" property. But the exact validation depends on the connection type. Add "NMU_IFACE_ANY" to validate the name to check whether it would be valid for any connection type. This is for completeness and for places where the caller might not know the connection type. --- shared/nm-glib-aux/nm-shared-utils.c | 11 +++++++++++ shared/nm-glib-aux/nm-shared-utils.h | 3 ++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 6e1bcd74cb..26eba35279 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -4211,6 +4211,17 @@ nm_utils_ifname_valid (const char* name, return _nm_utils_ifname_valid_kernel (name, error); case NMU_IFACE_OVS: return _nm_utils_ifname_valid_ovs (name, error); + case NMU_IFACE_ANY: { + gs_free_error GError *local = NULL; + + if (_nm_utils_ifname_valid_kernel (name, error ? &local : NULL)) + return TRUE; + if (_nm_utils_ifname_valid_ovs (name, NULL)) + return TRUE; + if (error) + g_propagate_error (error, g_steal_pointer (&local)); + return FALSE; + } } g_return_val_if_reached (FALSE); diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index a5d2ee7292..dc29b85d02 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1678,7 +1678,8 @@ nm_utils_strdup_reset (char **dst, const char *src) /*****************************************************************************/ typedef enum { - NMU_IFACE_KERNEL = 0, + NMU_IFACE_ANY, + NMU_IFACE_KERNEL, NMU_IFACE_OVS, } NMUtilsIfaceType; From 294de8487ea004361a118b86b728d30034c809ed Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 16:51:03 +0100 Subject: [PATCH 14/15] shared: add NMU_IFACE_OVS_OR_KERNEL for nm_utils_ifname_valid() Depending on the type, OVS interfaces also have a corresponding netdev in kernel (e.g. type "internal" does, type "patch" does not). Such a case is neither NMU_IFACE_OVS nor NMU_IFACE_KERNEL (alone). There should be a special type to represent those cases. Add NMU_IFACE_OVS_OR_KERNEL for that. --- shared/nm-glib-aux/nm-shared-utils.c | 3 +++ shared/nm-glib-aux/nm-shared-utils.h | 1 + 2 files changed, 4 insertions(+) diff --git a/shared/nm-glib-aux/nm-shared-utils.c b/shared/nm-glib-aux/nm-shared-utils.c index 26eba35279..2a7926d75b 100644 --- a/shared/nm-glib-aux/nm-shared-utils.c +++ b/shared/nm-glib-aux/nm-shared-utils.c @@ -4211,6 +4211,9 @@ nm_utils_ifname_valid (const char* name, return _nm_utils_ifname_valid_kernel (name, error); case NMU_IFACE_OVS: return _nm_utils_ifname_valid_ovs (name, error); + case NMU_IFACE_OVS_AND_KERNEL: + return _nm_utils_ifname_valid_kernel (name, error) + && _nm_utils_ifname_valid_ovs (name, error); case NMU_IFACE_ANY: { gs_free_error GError *local = NULL; diff --git a/shared/nm-glib-aux/nm-shared-utils.h b/shared/nm-glib-aux/nm-shared-utils.h index dc29b85d02..63badb3cd6 100644 --- a/shared/nm-glib-aux/nm-shared-utils.h +++ b/shared/nm-glib-aux/nm-shared-utils.h @@ -1681,6 +1681,7 @@ typedef enum { NMU_IFACE_ANY, NMU_IFACE_KERNEL, NMU_IFACE_OVS, + NMU_IFACE_OVS_AND_KERNEL, } NMUtilsIfaceType; gboolean nm_utils_ifname_valid_kernel (const char *name, GError **error); From 3efe070dfc7af799e5b3f8dc0950d9c73486b666 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 18 Feb 2020 14:00:10 +0100 Subject: [PATCH 15/15] libnm: validate "connection.interface-name" at one place only Don't spread the validation for the interface name between multiple places. There should be one place only, so when you search for how this property gets verified, you can find the single place. That requires to move the special handling for OVS interfaces to NMSettingConnection. Since we already have _nm_setting_ovs_interface_verify_interface_type(), that is easy. --- libnm-core/nm-setting-connection.c | 53 +++++++++++++++++++++------ libnm-core/nm-setting-ovs-interface.c | 41 ++++----------------- 2 files changed, 48 insertions(+), 46 deletions(-) diff --git a/libnm-core/nm-setting-connection.c b/libnm-core/nm-setting-connection.c index 6ea3c5fb29..2e8fa37a2c 100644 --- a/libnm-core/nm-setting-connection.c +++ b/libnm-core/nm-setting-connection.c @@ -1032,22 +1032,50 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) if (priv->interface_name) { GError *tmp_error = NULL; - gboolean valid_ifname = FALSE; + NMUtilsIfaceType iface_type; - /* do not perform a interface name length check for OVS connection types - * as they don't have a corresponding kernel link that enforces the 15 bytes limit. - * Here we're whitelisting the OVS interface type as well, even if most OVS - * iface types do have the limit, to let the OVS specific nm-setting verify whether the iface name - * is good or not according to the internal type (internal, patch, ...) */ if (NM_IN_STRSET (type, NM_SETTING_OVS_BRIDGE_SETTING_NAME, - NM_SETTING_OVS_PORT_SETTING_NAME, - NM_SETTING_OVS_INTERFACE_SETTING_NAME)) - valid_ifname = nm_utils_ifname_valid (priv->interface_name, NMU_IFACE_OVS, &tmp_error); - else - valid_ifname = nm_utils_ifname_valid (priv->interface_name, NMU_IFACE_KERNEL, &tmp_error); + NM_SETTING_OVS_PORT_SETTING_NAME)) + iface_type = NMU_IFACE_OVS; + else if (nm_streq (type, NM_SETTING_OVS_INTERFACE_SETTING_NAME)) { + NMSettingOvsInterface *s_ovs_iface = NULL; + const char *ovs_iface_type; - if (!valid_ifname) { + if (connection) + s_ovs_iface = nm_connection_get_setting_ovs_interface (connection); + _nm_setting_ovs_interface_verify_interface_type (s_ovs_iface, + s_ovs_iface ? nm_setting_ovs_interface_get_interface_type (s_ovs_iface) : NULL, + connection, + FALSE, + NULL, + &ovs_iface_type, + NULL); + if (!ovs_iface_type) { + /* We cannot determine to OVS interface type. Consequently, we cannot + * fully validate the interface name. + * + * If we have a connection (and we do a full validation anyway), skip the + * check. The connection will fail validation when we validate the OVS setting. + * + * Otherwise, do the most basic validation. + */ + if (connection) + goto after_interface_name; + iface_type = NMU_IFACE_ANY; + } else if (NM_IN_STRSET (ovs_iface_type, "patch")) { + /* this interface type is internal to OVS. */ + iface_type = NMU_IFACE_OVS; + } else { + /* This interface type also requires a netdev. We need to validate + * for both OVS and KERNEL. */ + nm_assert (NM_IN_STRSET (ovs_iface_type, "internal", "system", "dpdk")); + iface_type = NMU_IFACE_OVS_AND_KERNEL; + } + } else + iface_type = NMU_IFACE_KERNEL; + + if (!nm_utils_ifname_valid (priv->interface_name, iface_type, &tmp_error)) { g_set_error (error, NM_CONNECTION_ERROR, NM_CONNECTION_ERROR_INVALID_PROPERTY, @@ -1057,6 +1085,7 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) return FALSE; } } +after_interface_name: is_slave = FALSE; slave_setting_type = NULL; diff --git a/libnm-core/nm-setting-ovs-interface.c b/libnm-core/nm-setting-ovs-interface.c index 114e0205d3..b9a133d042 100644 --- a/libnm-core/nm-setting-ovs-interface.c +++ b/libnm-core/nm-setting-ovs-interface.c @@ -257,8 +257,6 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) { NMSettingOvsInterface *self = NM_SETTING_OVS_INTERFACE (setting); NMSettingConnection *s_con = NULL; - const char *normalized_type = NULL; - int result = NM_SETTING_VERIFY_ERROR; if (connection) { const char *slave_type; @@ -298,38 +296,13 @@ verify (NMSetting *setting, NMConnection *connection, GError **error) } } - result = _nm_setting_ovs_interface_verify_interface_type (self, - self->type, - connection, - FALSE, - NULL, - &normalized_type, - error); - - /* From 'man ovs-vswitchd.conf.db': OVS patch interfaces do not have - * a limit on interface name length, all the other types do */ - if (result != NM_SETTING_VERIFY_ERROR && s_con) { - gs_free_error GError *ifname_error = NULL; - const char *ifname = nm_setting_connection_get_interface_name (s_con); - - if ( ifname - && !nm_streq0 (normalized_type, "patch") - && !nm_utils_ifname_valid (ifname, - NMU_IFACE_KERNEL, - &ifname_error)) { - g_clear_error (error); - g_set_error (error, - NM_CONNECTION_ERROR, - NM_CONNECTION_ERROR_INVALID_PROPERTY, - "'%s': %s", ifname, ifname_error->message); - g_prefix_error (error, "%s.%s: ", - NM_SETTING_CONNECTION_SETTING_NAME, - NM_SETTING_CONNECTION_INTERFACE_NAME); - return NM_SETTING_VERIFY_ERROR; - } - } - - return result; + return _nm_setting_ovs_interface_verify_interface_type (self, + self->type, + connection, + FALSE, + NULL, + NULL, + error); } /*****************************************************************************/