From 43c3c259c8e2fd4aa5416b79efd47a72cca790ec Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Oct 2018 13:22:08 +0200 Subject: [PATCH 1/7] ndisc: ignore addresses with preferred lifetime larger than lifetime Previously, we would coerce the value so that preferred is the same as lifetime. However, RFC4862 5.5.3.c) says: c) If the preferred lifetime is greater than the valid lifetime, silently ignore the Prefix Information option. A node MAY wish to log a system management error in this case. See-also: https://tools.ietf.org/search/rfc4862#section-5.5.3 --- src/ndisc/nm-lndp-ndisc.c | 8 ++++---- src/ndisc/nm-ndisc.c | 1 + 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index 8bb1812764..48a17bdf4f 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -221,10 +221,10 @@ receive_ra (struct ndp *ndp, struct ndp_msg *msg, gpointer user_data) .preferred = ndp_msg_opt_prefix_preferred_time (msg, offset), }; - if (address.preferred > address.lifetime) - address.preferred = address.lifetime; - if (nm_ndisc_complete_and_add_address (ndisc, &address)) - changed |= NM_NDISC_CONFIG_ADDRESSES; + if (address.preferred <= address.lifetime) { + if (nm_ndisc_complete_and_add_address (ndisc, &address)) + changed |= NM_NDISC_CONFIG_ADDRESSES; + } } } ndp_msg_opt_for_each_offset(offset, msg, NDP_MSG_OPT_ROUTE) { diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index a6b4453891..9c80c0ec15 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -416,6 +416,7 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) nm_assert (new->timestamp > 0 && new->timestamp < G_MAXINT32); nm_assert (!IN6_IS_ADDR_UNSPECIFIED (&new->address)); nm_assert (!IN6_IS_ADDR_LINKLOCAL (&new->address)); + nm_assert (new->preferred <= new->lifetime); for (i = 0; i < rdata->addresses->len; i++) { NMNDiscAddress *item = &g_array_index (rdata->addresses, NMNDiscAddress, i); From 700b04d0deded1f8b8267e13202d3c29f8748e14 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 Oct 2018 11:48:34 +0200 Subject: [PATCH 2/7] ndisc: ensure we skip unspecified IPv6 address in ndisc_set_router_config() Later, nm_ndisc_add_address() asserts that the address is not an unspecified address. Skip it, just to be sure. --- src/devices/nm-device.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/devices/nm-device.c b/src/devices/nm-device.c index d1f059abd8..8bc98d6155 100644 --- a/src/devices/nm-device.c +++ b/src/devices/nm-device.c @@ -3545,7 +3545,8 @@ ndisc_set_router_config (NMNDisc *ndisc, NMDevice *self) guint32 lifetime, preferred; gint32 base; - if (IN6_IS_ADDR_LINKLOCAL (&addr->address)) + if ( IN6_IS_ADDR_UNSPECIFIED (&addr->address) + || IN6_IS_ADDR_LINKLOCAL (&addr->address)) continue; if ( addr->n_ifa_flags & IFA_F_TENTATIVE From 23c417854a943eb796a8c06e7d94b02d18f19691 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Oct 2018 13:40:58 +0200 Subject: [PATCH 3/7] ndisc: only generate address interface identifer after checking existing prefix RFC4862 5.5.3, points d) and e) make it clear, that the list of addresses should be compared based on the prefix. d) If the prefix advertised is not equal to the prefix of an address configured by stateless autoconfiguration already in the list of addresses associated with the interface (where "equal" means the two prefix lengths are the same and the first prefix- length bits of the prefixes are identical), and if the Valid Lifetime is not 0, form an address (and add it to the list) by combining the advertised prefix with an interface identifier of the link as follows: That means, we should not initialize the interface identifier first (via complete_address()) and then search for the full address. See-also: https://tools.ietf.org/search/rfc4862#section-5.5.3 --- src/ndisc/nm-ndisc-private.h | 2 +- src/ndisc/nm-ndisc.c | 63 ++++++++++++++++++++++++------------ 2 files changed, 43 insertions(+), 22 deletions(-) diff --git a/src/ndisc/nm-ndisc-private.h b/src/ndisc/nm-ndisc-private.h index bbecb01a43..cdf8dc282a 100644 --- a/src/ndisc/nm-ndisc-private.h +++ b/src/ndisc/nm-ndisc-private.h @@ -40,7 +40,7 @@ void nm_ndisc_ra_received (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap changed) void nm_ndisc_rs_received (NMNDisc *ndisc); gboolean nm_ndisc_add_gateway (NMNDisc *ndisc, const NMNDiscGateway *new); -gboolean nm_ndisc_complete_and_add_address (NMNDisc *ndisc, NMNDiscAddress *new); +gboolean nm_ndisc_complete_and_add_address (NMNDisc *ndisc, const NMNDiscAddress *new); gboolean nm_ndisc_add_route (NMNDisc *ndisc, const NMNDiscRoute *new); gboolean nm_ndisc_add_dns_server (NMNDisc *ndisc, const NMNDiscDNSServer *new); gboolean nm_ndisc_add_dns_domain (NMNDisc *ndisc, const NMNDiscDNSDomain *new); diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 9c80c0ec15..db70bd8347 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -406,10 +406,11 @@ complete_address (NMNDisc *ndisc, NMNDiscAddress *addr) } static gboolean -nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) +nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_ra) { NMNDiscPrivate *priv = NM_NDISC_GET_PRIVATE (ndisc); NMNDiscDataInternal *rdata = &priv->rdata; + NMNDiscAddress new2; guint i; nm_assert (new); @@ -421,20 +422,33 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) for (i = 0; i < rdata->addresses->len; i++) { NMNDiscAddress *item = &g_array_index (rdata->addresses, NMNDiscAddress, i); - if (IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) { - if (new->lifetime == 0) { - g_array_remove_index (rdata->addresses, i); - return TRUE; - } + if (from_ra) { + /* RFC4862 5.5.3.d, we find an existing address with the same prefix. + * (note that all prefixes at this point have implicity length /64). */ + if (memcmp (&item->address, &new->address, 8) != 0) + continue; + } else { + if (!IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) + continue; + } - if ( item->dad_counter == new->dad_counter - && get_expiry (item) == get_expiry (new) - && get_expiry_preferred (item) == get_expiry_preferred (new)) - return FALSE; - - *item = *new; + if (new->lifetime == 0) { + g_array_remove_index (rdata->addresses, i); return TRUE; } + + if ( get_expiry (item) == get_expiry (new) + && get_expiry_preferred (item) == get_expiry_preferred (new) + && ( from_ra + || item->dad_counter == new->dad_counter)) + return FALSE; + + if (!from_ra) + item->dad_counter = new->dad_counter; + item->timestamp = new->timestamp; + item->lifetime = new->lifetime; + item->preferred = new->preferred; + return TRUE; } /* we create at most max_addresses autoconf addresses. This is different from @@ -445,18 +459,25 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) && rdata->addresses->len >= priv->max_addresses) return FALSE; - if (new->lifetime) - g_array_append_val (rdata->addresses, *new); - return !!new->lifetime; + if (new->lifetime == 0) + return FALSE; + + if (from_ra) { + new2 = *new; + new2.dad_counter = 0; + if (!complete_address (ndisc, &new2)) + return FALSE; + new = &new2; + } + + g_array_append_val (rdata->addresses, *new); + return TRUE; } gboolean -nm_ndisc_complete_and_add_address (NMNDisc *ndisc, NMNDiscAddress *new) +nm_ndisc_complete_and_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) { - if (!complete_address (ndisc, new)) - return FALSE; - - return nm_ndisc_add_address (ndisc, new); + return nm_ndisc_add_address (ndisc, new, TRUE); } gboolean @@ -754,7 +775,7 @@ nm_ndisc_set_config (NMNDisc *ndisc, guint i; for (i = 0; i < addresses->len; i++) { - if (nm_ndisc_add_address (ndisc, &g_array_index (addresses, NMNDiscAddress, i))) + if (nm_ndisc_add_address (ndisc, &g_array_index (addresses, NMNDiscAddress, i), FALSE)) changed = TRUE; } From 9d0a138ef05544954de49a8056f47e34c6bf3c5a Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Oct 2018 13:52:03 +0200 Subject: [PATCH 4/7] ndisc: minor refactoring loop in nm_ndisc_add_address() No change in behavior. Just don't do so much work inside the deeper nesting of the loop. --- src/ndisc/nm-ndisc.c | 29 ++++++++++++++++++----------- 1 file changed, 18 insertions(+), 11 deletions(-) diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index db70bd8347..0c2dbf8cdd 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -411,6 +411,7 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_r NMNDiscPrivate *priv = NM_NDISC_GET_PRIVATE (ndisc); NMNDiscDataInternal *rdata = &priv->rdata; NMNDiscAddress new2; + NMNDiscAddress *existing = NULL; guint i; nm_assert (new); @@ -425,29 +426,35 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_r if (from_ra) { /* RFC4862 5.5.3.d, we find an existing address with the same prefix. * (note that all prefixes at this point have implicity length /64). */ - if (memcmp (&item->address, &new->address, 8) != 0) - continue; + if (memcmp (&item->address, &new->address, 8) == 0) { + existing = item; + break; + } } else { - if (!IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) - continue; + if (IN6_ARE_ADDR_EQUAL (&item->address, &new->address)) { + existing = item; + break; + } } + } + if (existing) { if (new->lifetime == 0) { g_array_remove_index (rdata->addresses, i); return TRUE; } - if ( get_expiry (item) == get_expiry (new) - && get_expiry_preferred (item) == get_expiry_preferred (new) + if ( get_expiry (existing) == get_expiry (new) + && get_expiry_preferred (existing) == get_expiry_preferred (new) && ( from_ra - || item->dad_counter == new->dad_counter)) + || existing->dad_counter == new->dad_counter)) return FALSE; if (!from_ra) - item->dad_counter = new->dad_counter; - item->timestamp = new->timestamp; - item->lifetime = new->lifetime; - item->preferred = new->preferred; + existing->dad_counter = new->dad_counter; + existing->timestamp = new->timestamp; + existing->lifetime = new->lifetime; + existing->preferred = new->preferred; return TRUE; } From b086535cb7f28a495a742c64ed4e212175f19860 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Fri, 12 Oct 2018 13:05:51 +0200 Subject: [PATCH 5/7] ndisc: handle integer overflows better for lifetime handling we use get_expiry() to compare two lifetimes. Note, that previously, it would correctly truncate the calculated expiry at G_MAXINT32-1. However, that means, that two different lifetimes that both lie more than 68 years in the future would compare equal. Fix that, but extending the range to int64, so that no overflow can happen. --- src/ndisc/nm-ndisc.c | 112 ++++++++++++++++++++----------------------- src/ndisc/nm-ndisc.h | 3 ++ 2 files changed, 56 insertions(+), 59 deletions(-) diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 0c2dbf8cdd..6c619ad3ce 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -124,53 +124,66 @@ _preference_to_priority (NMIcmpv6RouterPref pref) /*****************************************************************************/ -static gint32 +/* we rely on the fact, that _EXPIRY_INFINITY > any other valid gint64 timestamps. */ +#define _EXPIRY_INFINITY G_MAXINT64 + +static gint64 get_expiry_time (guint32 timestamp, guint32 lifetime) { - gint64 t; - - /* timestamp is supposed to come from nm_utils_get_monotonic_timestamp_s(). - * It is expected to be within a certain range. */ nm_assert (timestamp > 0); nm_assert (timestamp <= G_MAXINT32); if (lifetime == NM_NDISC_INFINITY) - return G_MAXINT32; - - t = (gint64) timestamp + (gint64) lifetime; - return CLAMP (t, 0, G_MAXINT32 - 1); + return _EXPIRY_INFINITY; + return ((gint64) timestamp) + ((gint64) lifetime); } #define get_expiry(item) \ ({ \ typeof (item) _item = (item); \ nm_assert (_item); \ - get_expiry_time ((_item->timestamp), (_item->lifetime)); \ + get_expiry_time (_item->timestamp, _item->lifetime); \ }) #define get_expiry_half(item) \ ({ \ typeof (item) _item = (item); \ nm_assert (_item); \ - get_expiry_time ((_item->timestamp),\ - (_item->lifetime) == NM_NDISC_INFINITY \ - ? NM_NDISC_INFINITY \ - : (_item->lifetime) / 2); \ + (_item->lifetime == NM_NDISC_INFINITY) \ + ? _EXPIRY_INFINITY \ + : get_expiry_time (_item->timestamp, _item->lifetime / 2); \ }) #define get_expiry_preferred(item) \ ({ \ typeof (item) _item = (item); \ nm_assert (_item); \ - get_expiry_time ((_item->timestamp), (_item->preferred)); \ + get_expiry_time (_item->timestamp, _item->preferred); \ }) +static gboolean +expiry_next (gint32 now_s, gint64 expiry_timestamp, gint32 *nextevent) +{ + gint32 e; + + if (expiry_timestamp == _EXPIRY_INFINITY) + return TRUE; + e = MIN (expiry_timestamp, ((gint64) (G_MAXINT32 - 1))); + if (now_s >= e) + return FALSE; + if (nextevent) { + if (*nextevent > e) + *nextevent = e; + } + return TRUE; +} + static const char * -_get_exp (char *buf, gsize buf_size, gint64 now_ns, gint32 expiry_time) +_get_exp (char *buf, gsize buf_size, gint64 now_ns, gint64 expiry_time) { int l; - if (expiry_time == G_MAXINT32) + if (expiry_time == _EXPIRY_INFINITY) return "permanent"; l = g_snprintf (buf, buf_size, "%.4f", @@ -1029,17 +1042,12 @@ clean_gateways (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap *changed, gint32 *n for (i = 0; i < rdata->gateways->len; ) { NMNDiscGateway *item = &g_array_index (rdata->gateways, NMNDiscGateway, i); - if (item->lifetime != NM_NDISC_INFINITY) { - gint32 expiry = get_expiry (item); - - if (now >= expiry) { - g_array_remove_index (rdata->gateways, i); - *changed |= NM_NDISC_CONFIG_GATEWAYS; - continue; - } - if (*nextevent > expiry) - *nextevent = expiry; + if (!expiry_next (now, get_expiry (item), nextevent)) { + g_array_remove_index (rdata->gateways, i); + *changed |= NM_NDISC_CONFIG_GATEWAYS; + continue; } + i++; } @@ -1057,17 +1065,12 @@ clean_addresses (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap *changed, gint32 * for (i = 0; i < rdata->addresses->len; ) { const NMNDiscAddress *item = &g_array_index (rdata->addresses, NMNDiscAddress, i); - if (item->lifetime != NM_NDISC_INFINITY) { - gint32 expiry = get_expiry (item); - - if (now >= expiry) { - g_array_remove_index (rdata->addresses, i); - *changed |= NM_NDISC_CONFIG_ADDRESSES; - continue; - } - if (*nextevent > expiry) - *nextevent = expiry; + if (!expiry_next (now, get_expiry (item), nextevent)) { + g_array_remove_index (rdata->addresses, i); + *changed |= NM_NDISC_CONFIG_ADDRESSES; + continue; } + i++; } } @@ -1083,17 +1086,12 @@ clean_routes (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap *changed, gint32 *nex for (i = 0; i < rdata->routes->len; ) { NMNDiscRoute *item = &g_array_index (rdata->routes, NMNDiscRoute, i); - if (item->lifetime != NM_NDISC_INFINITY) { - gint32 expiry = get_expiry (item); - - if (now >= expiry) { - g_array_remove_index (rdata->routes, i); - *changed |= NM_NDISC_CONFIG_ROUTES; - continue; - } - if (*nextevent > expiry) - *nextevent = expiry; + if (!expiry_next (now, get_expiry (item), nextevent)) { + g_array_remove_index (rdata->routes, i); + *changed |= NM_NDISC_CONFIG_ROUTES; + continue; } + i++; } } @@ -1108,18 +1106,16 @@ clean_dns_servers (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap *changed, gint32 for (i = 0; i < rdata->dns_servers->len; ) { NMNDiscDNSServer *item = &g_array_index (rdata->dns_servers, NMNDiscDNSServer, i); + gint64 refresh; - if (item->lifetime != NM_NDISC_INFINITY) { - gint32 expiry = get_expiry (item); - gint32 refresh; - - if (now >= expiry) { + refresh = get_expiry_half (item); + if (refresh != _EXPIRY_INFINITY) { + if (!expiry_next (now, get_expiry (item), NULL)) { g_array_remove_index (rdata->dns_servers, i); *changed |= NM_NDISC_CONFIG_DNS_SERVERS; continue; } - refresh = get_expiry_half (item); if (now >= refresh) solicit_routers (ndisc); else if (*nextevent > refresh) @@ -1139,18 +1135,16 @@ clean_dns_domains (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap *changed, gint32 for (i = 0; i < rdata->dns_domains->len; ) { NMNDiscDNSDomain *item = &g_array_index (rdata->dns_domains, NMNDiscDNSDomain, i); + gint64 refresh; - if (item->lifetime != NM_NDISC_INFINITY) { - gint32 expiry = get_expiry (item); - gint32 refresh; - - if (now >= expiry) { + refresh = get_expiry_half (item); + if (refresh != _EXPIRY_INFINITY) { + if (!expiry_next (now, get_expiry (item), NULL)) { g_array_remove_index (rdata->dns_domains, i); *changed |= NM_NDISC_CONFIG_DNS_DOMAINS; continue; } - refresh = get_expiry_half (item); if (now >= refresh) solicit_routers (ndisc); else if (*nextevent > refresh) diff --git a/src/ndisc/nm-ndisc.h b/src/ndisc/nm-ndisc.h index fdc5615f54..73eef368ed 100644 --- a/src/ndisc/nm-ndisc.h +++ b/src/ndisc/nm-ndisc.h @@ -58,6 +58,9 @@ typedef enum { NM_NDISC_DHCP_LEVEL_MANAGED } NMNDiscDHCPLevel; +/* we rely on the fact that NM_NDISC_INFINITY is the largest possible + * time duration (G_MAXUINT32) and that the range of finite values + * goes from 0 to G_MAXUINT32-1. */ #define NM_NDISC_INFINITY G_MAXUINT32 struct _NMNDiscGateway { From 27be3e03382deb3da29e0c31dc6cb871bd6be0e8 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Thu, 11 Oct 2018 14:57:38 +0200 Subject: [PATCH 6/7] ndisc: fix updating address lifetime on Router Announcement according to RFC4862 This is a denial-of-service protection, where a malicious router advertisement can expire the addresses. See-also: https://github.com/systemd/systemd/commit/6554550f35a7976f9110aff94743d3576d5f02dd See-also: https://tools.ietf.org/search/rfc4862#section-5.5.3 https://bugs.launchpad.net/ubuntu/+source/network-manager/+bug/1796622 --- src/ndisc/nm-fake-ndisc.c | 2 +- src/ndisc/nm-lndp-ndisc.c | 2 +- src/ndisc/nm-ndisc-private.h | 2 +- src/ndisc/nm-ndisc.c | 65 +++++++++++++++++++++++++++---- src/ndisc/tests/test-ndisc-fake.c | 7 ++-- 5 files changed, 64 insertions(+), 14 deletions(-) diff --git a/src/ndisc/nm-fake-ndisc.c b/src/ndisc/nm-fake-ndisc.c index 15abee88f1..6e9a72c6f4 100644 --- a/src/ndisc/nm-fake-ndisc.c +++ b/src/ndisc/nm-fake-ndisc.c @@ -296,7 +296,7 @@ receive_ra (gpointer user_data) .dad_counter = 0, }; - if (nm_ndisc_complete_and_add_address (ndisc, &address)) + if (nm_ndisc_complete_and_add_address (ndisc, &address, now)) changed |= NM_NDISC_CONFIG_ADDRESSES; } } diff --git a/src/ndisc/nm-lndp-ndisc.c b/src/ndisc/nm-lndp-ndisc.c index 48a17bdf4f..9f427d4d59 100644 --- a/src/ndisc/nm-lndp-ndisc.c +++ b/src/ndisc/nm-lndp-ndisc.c @@ -222,7 +222,7 @@ receive_ra (struct ndp *ndp, struct ndp_msg *msg, gpointer user_data) }; if (address.preferred <= address.lifetime) { - if (nm_ndisc_complete_and_add_address (ndisc, &address)) + if (nm_ndisc_complete_and_add_address (ndisc, &address, now)) changed |= NM_NDISC_CONFIG_ADDRESSES; } } diff --git a/src/ndisc/nm-ndisc-private.h b/src/ndisc/nm-ndisc-private.h index cdf8dc282a..ae03c0ee43 100644 --- a/src/ndisc/nm-ndisc-private.h +++ b/src/ndisc/nm-ndisc-private.h @@ -40,7 +40,7 @@ void nm_ndisc_ra_received (NMNDisc *ndisc, gint32 now, NMNDiscConfigMap changed) void nm_ndisc_rs_received (NMNDisc *ndisc); gboolean nm_ndisc_add_gateway (NMNDisc *ndisc, const NMNDiscGateway *new); -gboolean nm_ndisc_complete_and_add_address (NMNDisc *ndisc, const NMNDiscAddress *new); +gboolean nm_ndisc_complete_and_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gint32 now_s); gboolean nm_ndisc_add_route (NMNDisc *ndisc, const NMNDiscRoute *new); gboolean nm_ndisc_add_dns_server (NMNDisc *ndisc, const NMNDiscDNSServer *new); gboolean nm_ndisc_add_dns_domain (NMNDisc *ndisc, const NMNDiscDNSDomain *new); diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 6c619ad3ce..684fd78b4b 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -419,7 +419,10 @@ complete_address (NMNDisc *ndisc, NMNDiscAddress *addr) } static gboolean -nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_ra) +nm_ndisc_add_address (NMNDisc *ndisc, + const NMNDiscAddress *new, + gint32 now_s, + gboolean from_ra) { NMNDiscPrivate *priv = NM_NDISC_GET_PRIVATE (ndisc); NMNDiscDataInternal *rdata = &priv->rdata; @@ -432,6 +435,7 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_r nm_assert (!IN6_IS_ADDR_UNSPECIFIED (&new->address)); nm_assert (!IN6_IS_ADDR_LINKLOCAL (&new->address)); nm_assert (new->preferred <= new->lifetime); + nm_assert (!from_ra || now_s > 0); for (i = 0; i < rdata->addresses->len; i++) { NMNDiscAddress *item = &g_array_index (rdata->addresses, NMNDiscAddress, i); @@ -452,6 +456,51 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_r } if (existing) { + if (from_ra) { + const gint32 NM_NDISC_PREFIX_LFT_MIN = 7200; /* seconds, RFC4862 5.5.3.e */ + gint64 old_expiry_lifetime, old_expiry_preferred; + + old_expiry_lifetime = get_expiry (existing); + old_expiry_preferred = get_expiry_preferred (existing); + + if (new->lifetime == NM_NDISC_INFINITY) + existing->lifetime = NM_NDISC_INFINITY; + else { + gint64 new_lifetime, remaining_lifetime; + + /* see RFC4862 5.5.3.e */ + if (existing->lifetime == NM_NDISC_INFINITY) + remaining_lifetime = G_MAXINT64; + else + remaining_lifetime = ((gint64) existing->timestamp) + ((gint64) existing->lifetime) - ((gint64) now_s); + new_lifetime = ((gint64) new->timestamp) + ((gint64) new->lifetime) - ((gint64) now_s); + + if ( new_lifetime > (gint64) NM_NDISC_PREFIX_LFT_MIN + || new_lifetime > remaining_lifetime) { + existing->timestamp = now_s; + existing->lifetime = CLAMP (new_lifetime, (gint64) 0, (gint64) (G_MAXUINT32 - 1)); + } else if (remaining_lifetime <= (gint64) NM_NDISC_PREFIX_LFT_MIN) { + /* keep the current lifetime. */ + } else { + existing->timestamp = now_s; + existing->lifetime = NM_NDISC_PREFIX_LFT_MIN; + } + } + + if (new->preferred == NM_NDISC_INFINITY) { + nm_assert (existing->lifetime == NM_NDISC_INFINITY); + existing->preferred = new->preferred; + } else { + existing->preferred = NM_CLAMP (((gint64) new->timestamp) + ((gint64) new->preferred) - ((gint64) existing->timestamp), + 0, G_MAXUINT32 - 1); + if (existing->lifetime != NM_NDISC_INFINITY) + existing->preferred = MIN (existing->preferred, existing->lifetime); + } + + return old_expiry_lifetime != get_expiry (existing) + || old_expiry_preferred != get_expiry_preferred (existing); + } + if (new->lifetime == 0) { g_array_remove_index (rdata->addresses, i); return TRUE; @@ -459,12 +508,10 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_r if ( get_expiry (existing) == get_expiry (new) && get_expiry_preferred (existing) == get_expiry_preferred (new) - && ( from_ra - || existing->dad_counter == new->dad_counter)) + && existing->dad_counter == new->dad_counter) return FALSE; - if (!from_ra) - existing->dad_counter = new->dad_counter; + existing->dad_counter = new->dad_counter; existing->timestamp = new->timestamp; existing->lifetime = new->lifetime; existing->preferred = new->preferred; @@ -495,9 +542,11 @@ nm_ndisc_add_address (NMNDisc *ndisc, const NMNDiscAddress *new, gboolean from_r } gboolean -nm_ndisc_complete_and_add_address (NMNDisc *ndisc, const NMNDiscAddress *new) +nm_ndisc_complete_and_add_address (NMNDisc *ndisc, + const NMNDiscAddress *new, + gint32 now_s) { - return nm_ndisc_add_address (ndisc, new, TRUE); + return nm_ndisc_add_address (ndisc, new, now_s, TRUE); } gboolean @@ -795,7 +844,7 @@ nm_ndisc_set_config (NMNDisc *ndisc, guint i; for (i = 0; i < addresses->len; i++) { - if (nm_ndisc_add_address (ndisc, &g_array_index (addresses, NMNDiscAddress, i), FALSE)) + if (nm_ndisc_add_address (ndisc, &g_array_index (addresses, NMNDiscAddress, i), 0, FALSE)) changed = TRUE; } diff --git a/src/ndisc/tests/test-ndisc-fake.c b/src/ndisc/tests/test-ndisc-fake.c index e99d2fc55e..268f3b49b4 100644 --- a/src/ndisc/tests/test-ndisc-fake.c +++ b/src/ndisc/tests/test-ndisc-fake.c @@ -233,8 +233,9 @@ test_everything_changed (NMNDisc *ndisc, const NMNDiscData *rdata, guint changed g_assert_cmpint (rdata->gateways_n, ==, 1); match_gateway (rdata, 0, "fe80::2", data->timestamp1, 10, NM_ICMPV6_ROUTER_PREF_MEDIUM); - g_assert_cmpint (rdata->addresses_n, ==, 1); - match_address (rdata, 0, "2001:db8:a:b::1", data->timestamp1, 10, 10); + g_assert_cmpint (rdata->addresses_n, ==, 2); + match_address (rdata, 0, "2001:db8:a:a::1", data->timestamp1, 10, 0); + match_address (rdata, 1, "2001:db8:a:b::1", data->timestamp1, 10, 10); g_assert_cmpint (rdata->routes_n, ==, 1); match_route (rdata, 0, "2001:db8:a:b::", 64, "fe80::2", data->timestamp1, 10, 10); g_assert_cmpint (rdata->dns_servers_n, ==, 1); @@ -384,7 +385,7 @@ test_preference_changed_cb (NMNDisc *ndisc, const NMNDiscData *rdata, guint chan match_gateway (rdata, 0, "fe80::1", data->timestamp1 + 2, 10, NM_ICMPV6_ROUTER_PREF_HIGH); match_gateway (rdata, 1, "fe80::2", data->timestamp1 + 1, 10, NM_ICMPV6_ROUTER_PREF_MEDIUM); g_assert_cmpint (rdata->addresses_n, ==, 2); - match_address (rdata, 0, "2001:db8:a:a::1", data->timestamp1 + 2, 10, 10); + match_address (rdata, 0, "2001:db8:a:a::1", data->timestamp1 + 3, 9, 9); match_address (rdata, 1, "2001:db8:a:b::1", data->timestamp1 + 1, 10, 10); g_assert_cmpint (rdata->routes_n, ==, 2); match_route (rdata, 0, "2001:db8:a:a::", 64, "fe80::1", data->timestamp1 + 2, 10, 15); From 8c6629b356039e2b2bbb87574755dad298cb0615 Mon Sep 17 00:00:00 2001 From: Thomas Haller Date: Sat, 13 Oct 2018 17:11:09 +0200 Subject: [PATCH 7/7] ndisc: don't update dad_counter for addresses in router config I am not sure, we ever call complete_address() for router-configurations. Maybe not, so the dad-counter is never incremented and does not matter either. If we however do, then we certainly want to preserve the DAD counter when the address is already tracked. --- src/ndisc/nm-ndisc.c | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/ndisc/nm-ndisc.c b/src/ndisc/nm-ndisc.c index 684fd78b4b..1dd8398c75 100644 --- a/src/ndisc/nm-ndisc.c +++ b/src/ndisc/nm-ndisc.c @@ -507,11 +507,9 @@ nm_ndisc_add_address (NMNDisc *ndisc, } if ( get_expiry (existing) == get_expiry (new) - && get_expiry_preferred (existing) == get_expiry_preferred (new) - && existing->dad_counter == new->dad_counter) + && get_expiry_preferred (existing) == get_expiry_preferred (new)) return FALSE; - existing->dad_counter = new->dad_counter; existing->timestamp = new->timestamp; existing->lifetime = new->lifetime; existing->preferred = new->preferred;