netdev - Re: [PATCH net 1/3] bonding: fix ns validation on backup slaves

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <21417.1680235361@famine>
Date:   Thu, 30 Mar 2023 21:02:41 -0700
From:   Jay Vosburgh <jay.vosburgh@...onical.com>
To:     Hangbin Liu <liuhangbin@...il.com>
cc:     netdev@...r.kernel.org, "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Jonathan Toppins <jtoppins@...hat.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Eric Dumazet <edumazet@...gle.com>, Liang Li <liali@...hat.com>
Subject: Re: [PATCH net 1/3] bonding: fix ns validation on backup slaves

Hangbin Liu <liuhangbin@...il.com> wrote:

>When arp_validate is set to 2, 3, or 6, validation is performed for
>backup slaves as well. As stated in the bond documentation, validation
>involves checking the broadcast ARP request sent out via the active
>slave. This helps determine which slaves are more likely to function in
>the event of an active slave failure.
>
>However, when the target is an IPv6 address, the NS message sent from
>the active interface is not checked on backup slaves. Additionally,
>based on the bond_arp_rcv() rule b, we must reverse the saddr and daddr
>when checking the NS message.
>
>Note that when checking the NS message, the destination address is a
>multicast address. Therefore, we must convert the target address to
>solicited multicast in the bond_get_targets_ip6() function.
>
>Prior to the fix, the backup slaves had a mii status of "down", but
>after the fix, all of the slaves' mii status was updated to "UP".
>
>Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
>Signed-off-by: Hangbin Liu <liuhangbin@...il.com>

Acked-by: Jay Vosburgh <jay.vosburgh@...onical.com>

>---
> drivers/net/bonding/bond_main.c | 5 +++--
> include/net/bonding.h           | 8 ++++++--
> 2 files changed, 9 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>index 236e5219c811..8cc9a74789b7 100644
>--- a/drivers/net/bonding/bond_main.c
>+++ b/drivers/net/bonding/bond_main.c
>@@ -3269,7 +3269,8 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> 
> 	combined = skb_header_pointer(skb, 0, sizeof(_combined), &_combined);
> 	if (!combined || combined->ip6.nexthdr != NEXTHDR_ICMP ||
>-	    combined->icmp6.icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT)
>+	    (combined->icmp6.icmp6_type != NDISC_NEIGHBOUR_SOLICITATION &&
>+	     combined->icmp6.icmp6_type != NDISC_NEIGHBOUR_ADVERTISEMENT))
> 		goto out;
> 
> 	saddr = &combined->ip6.saddr;
>@@ -3291,7 +3292,7 @@ static int bond_na_rcv(const struct sk_buff *skb, struct bonding *bond,
> 	else if (curr_active_slave &&
> 		 time_after(slave_last_rx(bond, curr_active_slave),
> 			    curr_active_slave->last_link_up))
>-		bond_validate_na(bond, slave, saddr, daddr);
>+		bond_validate_na(bond, slave, daddr, saddr);
> 	else if (curr_arp_slave &&
> 		 bond_time_in_interval(bond, slave_last_tx(curr_arp_slave), 1))
> 		bond_validate_na(bond, slave, saddr, daddr);
>diff --git a/include/net/bonding.h b/include/net/bonding.h
>index ea36ab7f9e72..c3843239517d 100644
>--- a/include/net/bonding.h
>+++ b/include/net/bonding.h
>@@ -761,13 +761,17 @@ static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
> #if IS_ENABLED(CONFIG_IPV6)
> static inline int bond_get_targets_ip6(struct in6_addr *targets, struct in6_addr *ip)
> {
>+	struct in6_addr mcaddr;
> 	int i;
> 
>-	for (i = 0; i < BOND_MAX_NS_TARGETS; i++)
>-		if (ipv6_addr_equal(&targets[i], ip))
>+	for (i = 0; i < BOND_MAX_NS_TARGETS; i++) {
>+		addrconf_addr_solict_mult(&targets[i], &mcaddr);
>+		if ((ipv6_addr_equal(&targets[i], ip)) ||
>+		    (ipv6_addr_equal(&mcaddr, ip)))
> 			return i;
> 		else if (ipv6_addr_any(&targets[i]))
> 			break;
>+	}
> 
> 	return -1;
> }
>-- 
>2.38.1
>