From 693e1e9742791c4885d4e23c0f72729889c43853 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 25 Apr 2025 14:35:02 +0200 Subject: [PATCH 1/2] nm-initrd-generator: fix IPv6 with square brackets in bond options If any bond option contains an IPv6 address it needs to be enclosed with []. Otherwise the ':' separators from the IP address can be confused with the ':' separators from the 'bond=' cmdline arguments. However, the square brackets were ignored: $ nm-initrd-generator -s "bond=bond0:eth0,eth1:ns_ip6_target=[FC08::789:1:0:0:3]" NetworkManager-Message: 08:46:55.114: [1745498815.1146] cmdline-reader: Ignoring invalid bond option: "ns_ip6_target" = "[FC08": '[FC08' is not a valid IPv6 address for 'ns_ip6_target' option NetworkManager-Message: 08:46:55.114: [1745498815.1148] cmdline-reader: Ignoring extra: '789:1:0:0:3]'. The opening '[' was only being considered if it was the first character in `get_word`. Fix it and consider it if it's in the middle too. If the brackets are used first and last, directly remove them as it is what most callers expect. However, if it's in the middle there is no reasonable way to remove them, so don't do it. Instead, the caller will have to consider this possibility when processing the content. Fixes: ecc074b2f8a6 ('initrd: add command line parser') Fixes https://gitlab.freedesktop.org/NetworkManager/NetworkManager/-/issues/1755 (cherry picked from commit aeaf8ca23c49cd0c90a692771697abf1a62796d2) --- src/nm-initrd-generator/nmi-cmdline-reader.c | 60 ++++++--- .../tests/test-cmdline-reader.c | 122 +++++++++++++++++- 2 files changed, 159 insertions(+), 23 deletions(-) diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index d6dc1fcb7c..5ee8595e5e 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -299,33 +299,44 @@ get_word(char **argument, const char separator) { char *word; int nest = 0; + char *last_ch; + char *first_close = NULL; if (*argument == NULL) return NULL; - if (**argument == '[') { - nest++; - (*argument)++; - } - - word = *argument; + word = last_ch = *argument; while (**argument != '\0') { - if (nest && **argument == ']') { - **argument = '\0'; - (*argument)++; - nest--; - continue; - } - if (nest == 0 && **argument == separator) { **argument = '\0'; (*argument)++; break; } + if (**argument == '[') { + nest++; + } else if (nest && **argument == ']') { + nest--; + if (!first_close && nest == 0) + first_close = *argument; + } + + last_ch = *argument; (*argument)++; } + /* If the word is surrounded with the nesting symbols [], strip them so we return + * the inner content only. + * If there were nesting symbols but embracing only part of the inner content, don't + * remove them. Example: + * Remove [] in get_word("[fc08::1]:other_token", ":") + * Don't remove [] in get_word("ip6=[fc08::1]:other_token", ":") + */ + if (*word == '[' && *last_ch == ']' && last_ch == first_close) { + word++; + *last_ch = '\0'; + } + return *word ? word : NULL; } @@ -905,14 +916,25 @@ reader_parse_controller(Reader *reader, opts = get_word(&argument, ':'); while (opts && *opts) { - gs_free_error GError *error = NULL; - char *opt; - const char *opt_name; + gs_free_error GError *error = NULL; + char *tmp; + const char *opt_name; + char *opt; + const char *opt_value; + nm_auto_unref_ptrarray GPtrArray *opt_values = g_ptr_array_new(); + gs_free char *opt_normalized = NULL; + opt_name = get_word(&opts, '='); opt = get_word(&opts, ','); - opt_name = get_word(&opt, '='); - if (!_nm_setting_bond_validate_option(opt_name, opt, &error)) { + /* Normalize: convert ';' to ',' and remove '[]' from IPv6 addresses */ + tmp = opt; + while ((opt_value = get_word(&tmp, ';'))) + g_ptr_array_add(opt_values, (gpointer) opt_value); + g_ptr_array_add(opt_values, NULL); + opt_normalized = g_strjoinv(",", (char **) opt_values->pdata); + + if (!_nm_setting_bond_validate_option(opt_name, opt_normalized, &error)) { _LOGW(LOGD_CORE, "Ignoring invalid bond option: %s%s%s = %s%s%s: %s", NM_PRINT_FMT_QUOTE_STRING(opt_name), @@ -920,7 +942,7 @@ reader_parse_controller(Reader *reader, error->message); continue; } - nm_setting_bond_add_option(s_bond, opt_name, opt); + nm_setting_bond_add_option(s_bond, opt_name, opt_normalized); } mtu = get_word(&argument, ':'); diff --git a/src/nm-initrd-generator/tests/test-cmdline-reader.c b/src/nm-initrd-generator/tests/test-cmdline-reader.c index a0100764ca..af13849c8c 100644 --- a/src/nm-initrd-generator/tests/test-cmdline-reader.c +++ b/src/nm-initrd-generator/tests/test-cmdline-reader.c @@ -975,8 +975,8 @@ static void test_bond(void) { gs_unref_hashtable GHashTable *connections = NULL; - const char *const *ARGV = NM_MAKE_STRV("rd.route=192.0.2.53::bong0", - "bond=bong0:eth0,eth1:mode=balance-rr:9000", + const char *const *ARGV = NM_MAKE_STRV("rd.route=192.0.2.53::bond0", + "bond=bond0:eth0,eth1:mode=balance-rr:9000", "nameserver=203.0.113.53"); NMConnection *connection; NMSettingConnection *s_con; @@ -990,12 +990,12 @@ test_bond(void) connections = _parse_cons(ARGV); g_assert_cmpint(g_hash_table_size(connections), ==, 3); - connection = g_hash_table_lookup(connections, "bong0"); + connection = g_hash_table_lookup(connections, "bond0"); nmtst_assert_connection_verifies_without_normalization(connection); g_assert_cmpstr(nm_connection_get_connection_type(connection), ==, NM_SETTING_BOND_SETTING_NAME); - g_assert_cmpstr(nm_connection_get_id(connection), ==, "bong0"); + g_assert_cmpstr(nm_connection_get_id(connection), ==, "bond0"); controller_uuid = nm_connection_get_uuid(connection); g_assert(controller_uuid); @@ -1162,6 +1162,118 @@ test_bond_ip(void) NM_CONNECTION_MULTI_CONNECT_SINGLE); } +static void +test_bond_ip6_option(void) +{ + /* Test that IPv6 addresses within [] are parsed fine in different positions */ + + gs_unref_hashtable GHashTable *connections = NULL; + const char *const *ARGV = + NM_MAKE_STRV("bond=bond0:eth0,eth1:arp_interval=100,ns_ip6_target=[fc08::1]", + "bond=bond1:eth2,eth3:arp_interval=100,ns_ip6_target=[fc08::1]:9000", + "bond=bond2:eth4,eth5:ns_ip6_target=[fc08::1],arp_interval=100"); + NMConnection *connection; + NMSettingBond *s_bond; + + connections = _parse_cons(ARGV); + g_assert_cmpint(g_hash_table_size(connections), ==, 9); + + connection = g_hash_table_lookup(connections, "bond0"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), ==, "fc08::1"); + + connection = g_hash_table_lookup(connections, "bond1"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), ==, "fc08::1"); + + connection = g_hash_table_lookup(connections, "bond2"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), ==, "fc08::1"); +} + +static void +test_bond_multi_values_option(void) +{ + /* Test that semicolon-separated multi-valued options are parsed fine in different positions */ + + gs_unref_hashtable GHashTable *connections = NULL; + const char *const *ARGV = + NM_MAKE_STRV("bond=bond0:eth0,eth1:arp_interval=100,ns_ip6_target=[fc08::1];[fc08::2]", + "bond=bond1:eth2,eth3:arp_interval=100,ns_ip6_target=[fc08::1];[fc08::2]:9000", + "bond=bond2:eth4,eth5:ns_ip6_target=[fc08::1];[fc08::2],arp_interval=100", + "bond=bond3:eth6,eth7:arp_interval=100,arp_ip_target=10.0.0.1;10.0.0.2", + "bond=bond4:eth8,eth9:arp_interval=100,arp_ip_target=10.0.0.1;10.0.0.2:9000", + "bond=bond5:eth10,eth11:arp_ip_target=10.0.0.1;10.0.0.2,arp_interval=100"); + NMConnection *connection; + NMSettingBond *s_bond; + + connections = _parse_cons(ARGV); + g_assert_cmpint(g_hash_table_size(connections), ==, 18); + + connection = g_hash_table_lookup(connections, "bond0"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), + ==, + "fc08::1,fc08::2"); + + connection = g_hash_table_lookup(connections, "bond1"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), + ==, + "fc08::1,fc08::2"); + + connection = g_hash_table_lookup(connections, "bond2"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "ns_ip6_target"), + ==, + "fc08::1,fc08::2"); + + connection = g_hash_table_lookup(connections, "bond3"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "arp_ip_target"), + ==, + "10.0.0.1,10.0.0.2"); + + connection = g_hash_table_lookup(connections, "bond4"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "arp_ip_target"), + ==, + "10.0.0.1,10.0.0.2"); + + connection = g_hash_table_lookup(connections, "bond5"); + nmtst_assert_connection_verifies_without_normalization(connection); + s_bond = nm_connection_get_setting_bond(connection); + g_assert(s_bond); + g_assert_cmpint(nm_setting_bond_get_num_options(s_bond), ==, 3); + g_assert_cmpstr(nm_setting_bond_get_option_by_name(s_bond, "arp_ip_target"), + ==, + "10.0.0.1,10.0.0.2"); +} + static void test_bond_default(void) { @@ -2701,6 +2813,8 @@ main(int argc, char **argv) g_test_add_func("/initrd/cmdline/bootdev", test_bootdev); g_test_add_func("/initrd/cmdline/bond", test_bond); g_test_add_func("/initrd/cmdline/bond/ip", test_bond_ip); + g_test_add_func("/initrd/cmdline/bond/ip6-option", test_bond_ip6_option); + g_test_add_func("/initrd/cmdline/bond/multi-values-option", test_bond_multi_values_option); g_test_add_func("/initrd/cmdline/bond/default", test_bond_default); g_test_add_func("/initrd/cmdline/team", test_team); g_test_add_func("/initrd/cmdline/vlan", test_vlan); From 399b08aab645503999232bff38f7ffacb41e8f60 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=8D=C3=B1igo=20Huguet?= Date: Fri, 25 Apr 2025 14:45:02 +0200 Subject: [PATCH 2/2] nm-initrd-generator: fix ignored prefix for IPv6 address with brackets When defining an IPv6 address with square brackets and prefix, like [dead::beef]/64, the prefix was silently ignored. The address was accepted only accidentally, because get_word replaced ']' with '\0' so it resulted in a valid IPv6 address string, but without the prefix. The previous commit has fixed get_word with better logic to handle the square brackets, uncovering this issue. Fix it by explicitly splitting IP addresses and prefixes in reader_parse_ip so we get a valid address and prefix. Also, use a prefix different to 64 in the test test_if_ip6_manual. 64 is the default one, making that the test passed despite the defined prefix was actually ignored. Fixes: ecc074b2f8a6 ('initrd: add command line parser') (cherry picked from commit 6f6bb17a28e7247bfae8794f622b6855440b53e1) --- src/nm-initrd-generator/nmi-cmdline-reader.c | 22 ++++++++++++------- .../tests/test-cmdline-reader.c | 4 ++-- 2 files changed, 16 insertions(+), 10 deletions(-) diff --git a/src/nm-initrd-generator/nmi-cmdline-reader.c b/src/nm-initrd-generator/nmi-cmdline-reader.c index 5ee8595e5e..5389748d82 100644 --- a/src/nm-initrd-generator/nmi-cmdline-reader.c +++ b/src/nm-initrd-generator/nmi-cmdline-reader.c @@ -544,7 +544,7 @@ reader_parse_ip(Reader *reader, const char *sysfs_dir, char *argument) NMSettingConnection *s_con; NMSettingIPConfig *s_ip4 = NULL, *s_ip6 = NULL; gs_unref_hashtable GHashTable *ibft = NULL; - const char *tmp; + char *tmp; const char *tmp2; const char *tmp3; const char *kind; @@ -589,15 +589,25 @@ reader_parse_ip(Reader *reader, const char *sysfs_dir, char *argument) kind = tmp3; } else { /* :[]:::: */ - client_ip = tmp; + + /* note: split here address and prefix to normalize IPs defined as + * [dead::beef]/64. Latter parsing would fail due to the '[]'. */ + client_ip = get_word(&tmp, '/'); + if (client_ip) { - client_ip_family = get_ip_address_family(client_ip, TRUE); + client_ip_family = get_ip_address_family(client_ip, FALSE); if (client_ip_family == AF_UNSPEC) { _LOGW(LOGD_CORE, "Invalid IP address '%s'.", client_ip); return; } } + if (!nm_str_is_empty(tmp)) { + gboolean is_ipv4 = client_ip_family == AF_INET; + + client_ip_prefix = _nm_utils_ascii_str_to_int64(tmp, 10, 0, is_ipv4 ? 32 : 128, -1); + } + peer = tmp2; gateway_ip = get_word(&argument, ':'); netmask = get_word(&argument, ':'); @@ -672,11 +682,7 @@ reader_parse_ip(Reader *reader, const char *sysfs_dir, char *argument) NMIPAddress *address = NULL; NMIPAddr addr; - if (nm_inet_parse_with_prefix_bin(client_ip_family, - client_ip, - NULL, - &addr, - client_ip_prefix == -1 ? &client_ip_prefix : NULL)) { + if (nm_inet_parse_bin(client_ip_family, client_ip, NULL, &addr)) { if (client_ip_prefix == -1) { switch (client_ip_family) { case AF_INET: diff --git a/src/nm-initrd-generator/tests/test-cmdline-reader.c b/src/nm-initrd-generator/tests/test-cmdline-reader.c index af13849c8c..cd7b1069b6 100644 --- a/src/nm-initrd-generator/tests/test-cmdline-reader.c +++ b/src/nm-initrd-generator/tests/test-cmdline-reader.c @@ -597,7 +597,7 @@ static void test_if_ip6_manual(void) { gs_unref_hashtable GHashTable *connections = NULL; - const char *const *ARGV = NM_MAKE_STRV("ip=[2001:0db8::02]/64::[2001:0db8::01]::" + const char *const *ARGV = NM_MAKE_STRV("ip=[2001:0db8::02]/56::[2001:0db8::01]::" "hostname0.example.com:eth4::[2001:0db8::53]"); NMConnection *connection; NMSettingIPConfig *s_ip4; @@ -633,7 +633,7 @@ test_if_ip6_manual(void) ip_addr = nm_setting_ip_config_get_address(s_ip6, 0); g_assert(ip_addr); g_assert_cmpstr(nm_ip_address_get_address(ip_addr), ==, "2001:db8::2"); - g_assert_cmpint(nm_ip_address_get_prefix(ip_addr), ==, 64); + g_assert_cmpint(nm_ip_address_get_prefix(ip_addr), ==, 56); g_assert_cmpstr(nm_setting_ip_config_get_gateway(s_ip6), ==, "2001:db8::1"); g_assert_cmpstr(nm_setting_ip_config_get_dhcp_hostname(s_ip6), ==, NULL); }