From 5a213541ea2b90478498cb8ec8e55969d34e38e5 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Jan 2021 19:03:42 +0100 Subject: [PATCH 1/6] shared: add nm_str_buf_append_{dirty,c_len}() helpers --- shared/nm-glib-aux/nm-str-buf.h | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/shared/nm-glib-aux/nm-str-buf.h b/shared/nm-glib-aux/nm-str-buf.h index d770b8266a..6edd451ee5 100644 --- a/shared/nm-glib-aux/nm-str-buf.h +++ b/shared/nm-glib-aux/nm-str-buf.h @@ -267,6 +267,31 @@ nm_str_buf_append_required_delimiter(NMStrBuf *strbuf, char delimiter) return strbuf; } +static inline void +nm_str_buf_append_dirty(NMStrBuf *strbuf, gsize len) +{ + _nm_str_buf_assert(strbuf); + + /* this append @len bytes to the buffer, but it does not + * initialize them! */ + if (len > 0) { + nm_str_buf_maybe_expand(strbuf, len, FALSE); + strbuf->_priv_len += len; + } +} + +static inline void +nm_str_buf_append_c_len(NMStrBuf *strbuf, char ch, gsize len) +{ + _nm_str_buf_assert(strbuf); + + if (len > 0) { + nm_str_buf_maybe_expand(strbuf, len, FALSE); + memset(&strbuf->_priv_str[strbuf->_priv_len], ch, len); + strbuf->_priv_len += len; + } +} + static inline void nm_str_buf_reset(NMStrBuf *strbuf, const char *str) { From 96d7ddc8652074d2e8a86042a90e10f127a0748a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Jan 2021 16:53:22 +0100 Subject: [PATCH 2/6] ndisc: add comment and static assert for struct sizes for ndisc packet layout Obviously, there is no change in behavior. It's just an assertion. --- src/ndisc/nm-lndp-ndisc.c | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index 22f784712a..5447ebe2dd 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -333,7 +333,12 @@ _ndp_msg_add_option(struct ndp_msg *msg, int len) return ret; } +/*****************************************************************************/ + +/* "Recursive DNS Server Option" at https://tools.ietf.org/html/rfc8106#section-5.1 */ + #define NM_ND_OPT_RDNSS 25 + typedef struct { struct nd_opt_hdr header; uint16_t reserved; @@ -341,7 +346,14 @@ typedef struct { struct in6_addr addrs[0]; } NMLndpRdnssOption; +G_STATIC_ASSERT(sizeof(NMLndpRdnssOption) == 8u); + +/*****************************************************************************/ + +/* "DNS Search List Option" at https://tools.ietf.org/html/rfc8106#section-5.2 */ + #define NM_ND_OPT_DNSSL 31 + typedef struct { struct nd_opt_hdr header; uint16_t reserved; @@ -349,6 +361,10 @@ typedef struct { char search_list[0]; } NMLndpDnsslOption; +G_STATIC_ASSERT(sizeof(NMLndpDnsslOption) == 8u); + +/*****************************************************************************/ + static gboolean send_ra(NMNDisc *ndisc, GError **error) { From e3c464b56cbc6c76f9d5125270dba29a10716cf3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Jan 2021 16:51:18 +0100 Subject: [PATCH 3/6] ndisc: pack structs for lndp options There is no actual change in behavior, because "struct nd_opt_hdr" as two uint8_t, so in practice this struct was always packed already. But make it explicit, because it's clear that we use these structs to set the binary message and they need a well defined (packed) memory layout. --- src/ndisc/nm-lndp-ndisc.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index 5447ebe2dd..a561f3950b 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -339,7 +339,7 @@ _ndp_msg_add_option(struct ndp_msg *msg, int len) #define NM_ND_OPT_RDNSS 25 -typedef struct { +typedef struct _nm_packed { struct nd_opt_hdr header; uint16_t reserved; uint32_t lifetime; @@ -354,7 +354,7 @@ G_STATIC_ASSERT(sizeof(NMLndpRdnssOption) == 8u); #define NM_ND_OPT_DNSSL 31 -typedef struct { +typedef struct _nm_packed { struct nd_opt_hdr header; uint16_t reserved; uint32_t lifetime; From 8d9662e16fec84cce97cff024395b8a77a107feb Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Jan 2021 19:15:46 +0100 Subject: [PATCH 4/6] ndisc: minor cleanup in send_ra() - use size_t variable for memory sizes and guint for iterating over GArray. --- src/ndisc/nm-lndp-ndisc.c | 61 ++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index a561f3950b..cc3647916b 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -321,15 +321,19 @@ receive_ra(struct ndp *ndp, struct ndp_msg *msg, gpointer user_data) } static void * -_ndp_msg_add_option(struct ndp_msg *msg, int len) +_ndp_msg_add_option(struct ndp_msg *msg, gsize len) { - void *ret = (uint8_t *) msg + ndp_msg_payload_len(msg); + gsize payload_len = ndp_msg_payload_len(msg); + void *ret = &((uint8_t *) msg)[payload_len]; + + nm_assert(len <= G_MAXSIZE - payload_len); + len += payload_len; - len += ndp_msg_payload_len(msg); if (len > ndp_msg_payload_maxlen(msg)) return NULL; ndp_msg_payload_len_set(msg, len); + nm_assert(len == ndp_msg_payload_len(msg)); return ret; } @@ -358,7 +362,7 @@ typedef struct _nm_packed { struct nd_opt_hdr header; uint16_t reserved; uint32_t lifetime; - char search_list[0]; + uint8_t search_list[0]; } NMLndpDnsslOption; G_STATIC_ASSERT(sizeof(NMLndpDnsslOption) == 8u); @@ -368,14 +372,13 @@ G_STATIC_ASSERT(sizeof(NMLndpDnsslOption) == 8u); static gboolean send_ra(NMNDisc *ndisc, GError **error) { - NMLndpNDiscPrivate * priv = NM_LNDP_NDISC_GET_PRIVATE(ndisc); - NMNDiscDataInternal * rdata = ndisc->rdata; - gint32 now = nm_utils_get_monotonic_timestamp_sec(); - int errsv; - struct in6_addr * addr; - struct ndp_msg * msg; - struct nd_opt_prefix_info *prefix; - int i; + NMLndpNDiscPrivate * priv = NM_LNDP_NDISC_GET_PRIVATE(ndisc); + NMNDiscDataInternal *rdata = ndisc->rdata; + gint32 now = nm_utils_get_monotonic_timestamp_sec(); + int errsv; + struct in6_addr * addr; + struct ndp_msg * msg; + guint i; errsv = ndp_msg_new(&msg, NDP_MSG_RA); if (errsv) { @@ -400,10 +403,11 @@ send_ra(NMNDisc *ndisc, GError **error) /* The device let us know about all addresses that the device got * whose prefixes are suitable for delegating. Let's announce them. */ for (i = 0; i < rdata->addresses->len; i++) { - NMNDiscAddress *address = &g_array_index(rdata->addresses, NMNDiscAddress, i); + const NMNDiscAddress *address = &g_array_index(rdata->addresses, NMNDiscAddress, i); guint32 age = NM_CLAMP((gint64) now - (gint64) address->timestamp, 0, G_MAXUINT32 - 1); guint32 lifetime = address->lifetime; guint32 preferred = address->preferred; + struct nd_opt_prefix_info *prefix; /* Clamp the life times if they're not forever. */ if (lifetime != NM_NDISC_INFINITY) @@ -431,31 +435,34 @@ send_ra(NMNDisc *ndisc, GError **error) prefix->nd_opt_pi_prefix.s6_addr32[3] = 0; } - if (rdata->dns_servers->len) { + if (rdata->dns_servers->len > 0u) { NMLndpRdnssOption *option; - int len = sizeof(*option) + sizeof(option->addrs[0]) * rdata->dns_servers->len; + gsize len = sizeof(*option) + (sizeof(option->addrs[0]) * rdata->dns_servers->len); option = _ndp_msg_add_option(msg, len); - if (option) { - option->header.nd_opt_type = NM_ND_OPT_RDNSS; - option->header.nd_opt_len = len / 8; - option->lifetime = htonl(900); - - for (i = 0; i < rdata->dns_servers->len; i++) { - NMNDiscDNSServer *dns_server = - &g_array_index(rdata->dns_servers, NMNDiscDNSServer, i); - option->addrs[i] = dns_server->address; - } - } else { + if (!option) { _LOGW("The RA is too big, had to omit DNS information."); + goto dns_servers_done; + } + + option->header.nd_opt_type = NM_ND_OPT_RDNSS; + option->header.nd_opt_len = len / 8; + option->lifetime = htonl(900); + + for (i = 0; i < rdata->dns_servers->len; i++) { + const NMNDiscDNSServer *dns_server = + &g_array_index(rdata->dns_servers, NMNDiscDNSServer, i); + + option->addrs[i] = dns_server->address; } } +dns_servers_done: if (rdata->dns_domains->len) { NMLndpDnsslOption *option; NMNDiscDNSDomain * dns_server; int len = sizeof(*option); - char * search_list; + uint8_t * search_list; for (i = 0; i < rdata->dns_domains->len; i++) { dns_server = &g_array_index(rdata->dns_domains, NMNDiscDNSDomain, i); From 872f26585992acc9f8b6324d0e715a0de76bd0e3 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Tue, 12 Jan 2021 19:17:09 +0100 Subject: [PATCH 5/6] ndisc: fix encoding DNS Search List Option in RA The format is different than what was implemented. Read [1] or see systemd's implementation ([2]). [1] https://tools.ietf.org/html/rfc8106#section-5.2 [2] https://github.com/systemd/systemd/blob/65ab27211c32089e038de7352091b46903c91ee6/src/libsystemd-network/sd-radv.c#L791 Fixes: 63878566020b ('ndisc/lndp: add ability to announce the managed IPv6 configuration') --- src/ndisc/nm-lndp-ndisc.c | 110 ++++++++++++++++++++++++++------------ 1 file changed, 77 insertions(+), 33 deletions(-) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index cc3647916b..34d701aa01 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -13,6 +13,8 @@ #include #include +#include "nm-glib-aux/nm-str-buf.h" +#include "systemd/nm-sd-utils-shared.h" #include "nm-ndisc-private.h" #include "NetworkManagerUtils.h" #include "platform/nm-platform.h" @@ -372,13 +374,14 @@ G_STATIC_ASSERT(sizeof(NMLndpDnsslOption) == 8u); static gboolean send_ra(NMNDisc *ndisc, GError **error) { - NMLndpNDiscPrivate * priv = NM_LNDP_NDISC_GET_PRIVATE(ndisc); - NMNDiscDataInternal *rdata = ndisc->rdata; - gint32 now = nm_utils_get_monotonic_timestamp_sec(); - int errsv; - struct in6_addr * addr; - struct ndp_msg * msg; - guint i; + NMLndpNDiscPrivate * priv = NM_LNDP_NDISC_GET_PRIVATE(ndisc); + NMNDiscDataInternal * rdata = ndisc->rdata; + gint32 now = nm_utils_get_monotonic_timestamp_sec(); + int errsv; + struct in6_addr * addr; + struct ndp_msg * msg; + guint i; + nm_auto_str_buf NMStrBuf sbuf = NM_STR_BUF_INIT(0, FALSE); errsv = ndp_msg_new(&msg, NDP_MSG_RA); if (errsv) { @@ -458,39 +461,80 @@ send_ra(NMNDisc *ndisc, GError **error) } dns_servers_done: - if (rdata->dns_domains->len) { + if (rdata->dns_domains->len > 0u) { NMLndpDnsslOption *option; - NMNDiscDNSDomain * dns_server; - int len = sizeof(*option); - uint8_t * search_list; + gsize padding; + gsize len; + + nm_str_buf_reset(&sbuf, NULL); for (i = 0; i < rdata->dns_domains->len; i++) { - dns_server = &g_array_index(rdata->dns_domains, NMNDiscDNSDomain, i); - len += strlen(dns_server->domain) + 2; - } - len = (len + 8) & ~0x7; + const NMNDiscDNSDomain *dns_domain = + &g_array_index(rdata->dns_domains, NMNDiscDNSDomain, i); + const char *domain = dns_domain->domain; + gsize domain_l; + gsize n_reserved; + int r; - option = _ndp_msg_add_option(msg, len); - if (option) { - option->header.nd_opt_type = NM_ND_OPT_DNSSL; - option->header.nd_opt_len = len / 8; - option->lifetime = htonl(900); - - search_list = option->search_list; - for (i = 0; i < rdata->dns_domains->len; i++) { - NMNDiscDNSDomain *dns_domain = - &g_array_index(rdata->dns_domains, NMNDiscDNSDomain, i); - uint8_t domain_len = strlen(dns_domain->domain); - - *search_list++ = domain_len; - memcpy(search_list, dns_domain->domain, domain_len); - search_list += domain_len; - *search_list++ = '\0'; + if (nm_str_is_empty(domain)) { + nm_assert_not_reached(); + continue; } - } else { - _LOGW("The RA is too big, had to omit DNS search list."); + + domain_l = strlen(domain); + + nm_str_buf_maybe_expand(&sbuf, domain_l + 2u, FALSE); + n_reserved = sbuf.allocated - sbuf.len; + + r = nm_sd_dns_name_to_wire_format( + domain, + (guint8 *) (&nm_str_buf_get_str_unsafe(&sbuf)[sbuf.len]), + n_reserved, + FALSE); + + if (r < 0 || ((gsize) r) > n_reserved) { + nm_assert(r != -ENOBUFS); + nm_assert(r < 0); + /* we don't expect errors here, unless the domain name is invalid. + * That should have been caught (and rejected) by upper layers, but + * at this point it seems dangerous to assert (as it's hard to review + * that all callers got it correct). So instead silently ignore the error. */ + continue; + } + + nm_str_buf_set_size(&sbuf, sbuf.len + ((gsize) r), TRUE, FALSE); } + + if (sbuf.len == 0) { + /* no valid domains? */ + goto dns_domains_done; + } + + len = sizeof(*option) + sbuf.len; + padding = len % 8u; + if (padding != 0u) { + padding = 8u - padding; + len += padding; + } + + nm_assert(len % 8u == 0u); + nm_assert(len > 0u); + nm_assert(len / 8u >= 2u); + + if (len / 8u >= 256u || !(option = _ndp_msg_add_option(msg, len))) { + _LOGW("The RA is too big, had to omit DNS search list."); + goto dns_domains_done; + } + + nm_str_buf_append_c_len(&sbuf, '\0', padding); + + option->header.nd_opt_type = NM_ND_OPT_DNSSL; + option->header.nd_opt_len = len / 8u; + option->reserved = 0; + option->lifetime = htonl(900); + memcpy(option->search_list, nm_str_buf_get_str_unsafe(&sbuf), sbuf.len); } +dns_domains_done: errsv = ndp_msg_send(priv->ndp, msg); From 68528f7af564762042afd47ef03fde7a0ab2a8b4 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Wed, 13 Jan 2021 17:49:49 +0100 Subject: [PATCH 6/6] ndisc: don't artificially extend the lifetime of DNSSL/RDNSS options RFCs actually expect to honor the lifetime. See for example [1]. This is just not right, and totally arbitrary. It was added when our libndp based implementation was added, but unclear why this was done (beyond the code comment). [1] page 204, v6LC.2.2.25: Processing Router Advertisement DNS (Host only) at https://ipv6ready.org/docs/Core_Conformance_5_0_0.pdf --- src/ndisc/nm-lndp-ndisc.c | 14 -------------- 1 file changed, 14 deletions(-) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index 34d701aa01..2117190584 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -249,13 +249,6 @@ receive_ra(struct ndp *ndp, struct ndp_msg *msg, gpointer user_data) .lifetime = ndp_msg_opt_rdnss_lifetime(msg, offset), }; - /* Pad the lifetime somewhat to give a bit of slack in cases - * where one RA gets lost or something (which can happen on unreliable - * links like Wi-Fi where certain types of frames are not retransmitted). - * Note that 0 has special meaning and is therefore not adjusted. - */ - if (dns_server.lifetime && dns_server.lifetime < 7200) - dns_server.lifetime = 7200; if (nm_ndisc_add_dns_server(ndisc, &dns_server)) changed |= NM_NDISC_CONFIG_DNS_SERVERS; } @@ -271,13 +264,6 @@ receive_ra(struct ndp *ndp, struct ndp_msg *msg, gpointer user_data) .lifetime = ndp_msg_opt_dnssl_lifetime(msg, offset), }; - /* Pad the lifetime somewhat to give a bit of slack in cases - * where one RA gets lost or something (which can happen on unreliable - * links like Wi-Fi where certain types of frames are not retransmitted). - * Note that 0 has special meaning and is therefore not adjusted. - */ - if (dns_domain.lifetime && dns_domain.lifetime < 7200) - dns_domain.lifetime = 7200; if (nm_ndisc_add_dns_domain(ndisc, &dns_domain)) changed |= NM_NDISC_CONFIG_DNS_DOMAINS; }