From f9c3e5f5a497f1214ca169a3eb0910e7bc4d19e7 Mon Sep 17 00:00:00 2001 From: "Martine S. Lenders" Date: Fri, 28 Aug 2020 12:26:25 +0200 Subject: [PATCH 1/2] gnrc_ipv6_nib: check return value of gnrc_netif_ipv6_addr_idx() And acquire exclusive network interface access when necessary. Fixes #14752. --- .../gnrc/network_layer/ipv6/nib/_nib-6ln.c | 24 +++++++++++--- sys/net/gnrc/network_layer/ipv6/nib/nib.c | 31 ++++++++++++++++--- 2 files changed, 47 insertions(+), 8 deletions(-) diff --git a/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.c b/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.c index 1cbf54d102..55ffe09b06 100644 --- a/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.c +++ b/sys/net/gnrc/network_layer/ipv6/nib/_nib-6ln.c @@ -96,6 +96,14 @@ uint8_t _handle_aro(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6, uint16_t ltime = byteorder_ntohs(aro->ltime); uint32_t rereg_time; int idx = gnrc_netif_ipv6_addr_idx(netif, &ipv6->dst); + + if (idx < 0) { + DEBUG("nib: Address %s is not assigned to interface " + "%d ignoring ARO\n", + ipv6_addr_to_str(addr_str, &ipv6->dst, + sizeof(addr_str)), netif->pid); + return _ADDR_REG_STATUS_IGNORE; + } /* if ltime 1min, reschedule NS in 30sec, otherwise 1min * before timeout */ rereg_time = (ltime == 1U) ? (30 * MS_PER_SEC) : @@ -176,6 +184,8 @@ static inline bool _is_valid(const gnrc_netif_t *netif, int idx) void _handle_rereg_address(const ipv6_addr_t *addr) { gnrc_netif_t *netif = gnrc_netif_get_by_ipv6_addr(addr); + + gnrc_netif_acquire(netif); _nib_dr_entry_t *router = _nib_drl_get(NULL, netif->pid); const bool router_reachable = (router != NULL) && _is_reachable(router->next_hop); @@ -197,10 +207,15 @@ void _handle_rereg_address(const ipv6_addr_t *addr) if (netif != NULL) { int idx = gnrc_netif_ipv6_addr_idx(netif, addr); - if (router_reachable && - (_is_valid(netif, idx) || (_is_tentative(netif, idx) && - (gnrc_netif_ipv6_addr_dad_trans(netif, idx) < - SIXLOWPAN_ND_REG_TRANSMIT_NUMOF)))) { + if (idx < 0) { + DEBUG("nib: %s is not assigned to interface %d anymore.\n", + ipv6_addr_to_str(addr_str, addr, sizeof(addr_str)), + netif->pid); + } + else if (router_reachable && + (_is_valid(netif, idx) || (_is_tentative(netif, idx) && + (gnrc_netif_ipv6_addr_dad_trans(netif, idx) < + SIXLOWPAN_ND_REG_TRANSMIT_NUMOF)))) { uint32_t retrans_time; if (_is_valid(netif, idx)) { @@ -219,6 +234,7 @@ void _handle_rereg_address(const ipv6_addr_t *addr) _handle_search_rtr(netif); } } + gnrc_netif_release(netif); } #if IS_ACTIVE(CONFIG_GNRC_IPV6_NIB_MULTIHOP_P6C) diff --git a/sys/net/gnrc/network_layer/ipv6/nib/nib.c b/sys/net/gnrc/network_layer/ipv6/nib/nib.c index 549bd1521b..bb332d70e9 100644 --- a/sys/net/gnrc/network_layer/ipv6/nib/nib.c +++ b/sys/net/gnrc/network_layer/ipv6/nib/nib.c @@ -916,21 +916,33 @@ static void _handle_nbr_sol(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6, gnrc_netif_t *tgt_netif = gnrc_netif_get_by_ipv6_addr(&nbr_sol->tgt); if (tgt_netif != NULL) { - int idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_sol->tgt); + int idx; - if (gnrc_netif_ipv6_addr_dad_trans(tgt_netif, idx)) { + gnrc_netif_acquire(tgt_netif); + idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_sol->tgt); + /* if idx < 0: + * nbr_sol->tgt was removed between getting tgt_netif by nbr_sol->tgt + * and gnrc_netif_acquire(tgt_netif). This is like `tgt_netif` would + * have been NULL in the first place so just continue as if it would + * have. */ + if ((idx >= 0) && gnrc_netif_ipv6_addr_dad_trans(tgt_netif, idx)) { if (!ipv6_addr_is_unspecified(&ipv6->src)) { /* (see https://tools.ietf.org/html/rfc4862#section-5.4.3) */ DEBUG("nib: Neighbor is performing AR, but target address is " "still TENTATIVE for us => Ignoring NS\n"); + gnrc_netif_release(tgt_netif); return; } /* cancel validation timer */ evtimer_del(&_nib_evtimer, &tgt_netif->ipv6.addrs_timers[idx].event); + /* _remove_tentative_addr() context switches to `tgt_netif->pid` so + * release `tgt_netif`. We are done here anyway. */ + gnrc_netif_release(tgt_netif); _remove_tentative_addr(tgt_netif, &nbr_sol->tgt); return; } + gnrc_netif_release(tgt_netif); } #endif /* CONFIG_GNRC_IPV6_NIB_SLAAC */ if (ipv6_addr_is_unspecified(&ipv6->src)) { @@ -1048,19 +1060,30 @@ static void _handle_nbr_adv(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6, gnrc_netif_t *tgt_netif = gnrc_netif_get_by_ipv6_addr(&nbr_adv->tgt); if (tgt_netif != NULL) { - int idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_adv->tgt); + int idx; - if (gnrc_netif_ipv6_addr_dad_trans(tgt_netif, idx)) { + gnrc_netif_acquire(tgt_netif); + idx = gnrc_netif_ipv6_addr_idx(tgt_netif, &nbr_adv->tgt); + /* if idx < 0: + * nbr_sol->tgt was removed between getting tgt_netif by nbr_sol->tgt + * and gnrc_netif_acquire(tgt_netif). This is like `tgt_netif` would + * have been NULL in the first place so just continue as if it would + * have. */ + if ((idx >= 0) && gnrc_netif_ipv6_addr_dad_trans(tgt_netif, idx)) { DEBUG("nib: duplicate address detected, removing target address " "from this interface\n"); /* cancel validation timer */ evtimer_del(&_nib_evtimer, &tgt_netif->ipv6.addrs_timers[idx].event); + /* _remove_tentative_addr() context switches to `tgt_netif->pid` so + * release `tgt_netif`. We are done here anyway. */ + gnrc_netif_release(tgt_netif); _remove_tentative_addr(tgt_netif, &nbr_adv->tgt); return; } /* else case beyond scope of RFC4862: * https://tools.ietf.org/html/rfc4862#section-5.4.4 */ + gnrc_netif_release(tgt_netif); } #endif /* CONFIG_GNRC_IPV6_NIB_SLAAC */ if (((nce = _nib_onl_get(&nbr_adv->tgt, netif->pid)) != NULL) && From 91dfee7ee056a5dffeea603c3be0c984b33af9ee Mon Sep 17 00:00:00 2001 From: "Martine S. Lenders" Date: Fri, 28 Aug 2020 13:53:30 +0200 Subject: [PATCH 2/2] gnrc_ipv6_nib: fix duplicate handling for ARO When `nce` is NULL on the duplicate check, the later re-fetching of the `nce` might result in an actual NCE entry that then contains a duplicate, so we need to re-check the EUI-64 again as well. --- sys/net/gnrc/network_layer/ipv6/nib/_nib-6lr.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/sys/net/gnrc/network_layer/ipv6/nib/_nib-6lr.c b/sys/net/gnrc/network_layer/ipv6/nib/_nib-6lr.c index c892fde2f4..c73ea9d3ed 100644 --- a/sys/net/gnrc/network_layer/ipv6/nib/_nib-6lr.c +++ b/sys/net/gnrc/network_layer/ipv6/nib/_nib-6lr.c @@ -69,6 +69,21 @@ uint8_t _reg_addr_upstream(gnrc_netif_t *netif, const ipv6_hdr_t *ipv6, _handle_sl2ao(netif, ipv6, icmpv6, sl2ao); /* re-get NCE in case it was updated */ nce = _nib_onl_get(&ipv6->src, netif->pid); + /* and re-check EUI-64 in case nce was not an NC before */ + if ((memcmp(&nce->eui64, &aro->eui64, + sizeof(aro->eui64)) != 0) && + (_get_ar_state(nce) != GNRC_IPV6_NIB_NC_INFO_AR_STATE_GC)) { + /* ignore address registration requests from upstream */ + DEBUG("nib: Could not register %s, duplicate entry with " + "EUI-64 %02x:%02x:%02x:%02x:%02x:%02x:%02x:%02x\n", + ipv6_addr_to_str(addr_str, &ipv6->src, + sizeof(addr_str)), + nce->eui64.uint8[0], nce->eui64.uint8[1], + nce->eui64.uint8[2], nce->eui64.uint8[3], + nce->eui64.uint8[4], nce->eui64.uint8[5], + nce->eui64.uint8[6], nce->eui64.uint8[7]); + return SIXLOWPAN_ND_STATUS_DUP; + } return _update_nce_ar_state(aro, nce); } else if (nce != NULL) {