netdev - Re: [PATCH net] batman-adv: fix duplicate MAC address check

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: <0c288b2e-9747-4a50-a16f-bf4238829ffa@redhat.com>
Date: Thu, 10 Apr 2025 11:38:03 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: Matthias Schiffer <mschiffer@...verse-factory.net>,
 Marek Lindner <marek.lindner@...lbox.org>,
 Simon Wunderlich <sw@...onwunderlich.de>,
 Antonio Quartulli <antonio@...delbit.com>,
 Sven Eckelmann <sven@...fation.org>
Cc: "David S. Miller" <davem@...emloft.net>,
 Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>,
 Simon Horman <horms@...nel.org>, b.a.t.m.a.n@...ts.open-mesh.org,
 netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net] batman-adv: fix duplicate MAC address check

On 4/8/25 6:30 PM, Matthias Schiffer wrote:
> batadv_check_known_mac_addr() is both too lenient and too strict:
> 
> - It is called from batadv_hardif_add_interface(), which means that it
>   checked interfaces that are not used for batman-adv at all. Move it
>   to batadv_hardif_enable_interface(). Also, restrict it to hardifs of
>   the same mesh interface; different mesh interfaces should not interact
>   at all. The batadv_check_known_mac_addr() argument is changed from
>   `struct net_device` to `struct batadv_hard_iface` to achieve this.
> - The check only cares about hardifs in BATADV_IF_ACTIVE and
>   BATADV_IF_TO_BE_ACTIVATED states, but interfaces in BATADV_IF_INACTIVE
>   state should be checked as well, or the following steps will not
>   result in a warning then they should:
> 
>   - Add two interfaces on down state with different MAC addresses to
>     a mesh as hardifs
>   - Change the MAC addresses so they confliect
>   - Set interfaces to up state
> 
>   Now there will be two active hardifs with the same MAC address, but no
>   warning. Fix by only ignoring hardifs in BATADV_IF_NOT_IN_USE state.
> 
> The RCU lock can be dropped, as we're holding RTNL anyways when the
> function is called.
> 
> While we're at it, also switch from pr_warn() to netdev_warn().
> 
> Fixes: c6c8fea29769 ("net: Add batman-adv meshing protocol")
> Signed-off-by: Matthias Schiffer <mschiffer@...verse-factory.net>

Even if marked for net I assume this will eventually go first via the
batman tree.

> ---
> 
> Aside: batadv_hardif_add_interface() being called for all existing
> interfaces and having a global batadv_hardif_list at all is also not
> very nice, but this will be addressed separately, as changing it will
> require more refactoring.
> 
>  net/batman-adv/hard-interface.c | 37 ++++++++++++++++++++-------------
>  1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/net/batman-adv/hard-interface.c b/net/batman-adv/hard-interface.c
> index f145f9662653..07b436626afb 100644
> --- a/net/batman-adv/hard-interface.c
> +++ b/net/batman-adv/hard-interface.c
> @@ -506,28 +506,34 @@ batadv_hardif_is_iface_up(const struct batadv_hard_iface *hard_iface)
>  	return false;
>  }
>  
> -static void batadv_check_known_mac_addr(const struct net_device *net_dev)
> +static void batadv_check_known_mac_addr(const struct batadv_hard_iface *hard_iface)
>  {
> -	const struct batadv_hard_iface *hard_iface;
> +	const struct net_device *mesh_iface = hard_iface->mesh_iface;
> +	const struct batadv_hard_iface *tmp_hard_iface;
>  
> -	rcu_read_lock();
> -	list_for_each_entry_rcu(hard_iface, &batadv_hardif_list, list) {
> -		if (hard_iface->if_status != BATADV_IF_ACTIVE &&
> -		    hard_iface->if_status != BATADV_IF_TO_BE_ACTIVATED)
> +	if (!mesh_iface)
> +		return;
> +
> +	list_for_each_entry(tmp_hard_iface, &batadv_hardif_list, list) {
> +		if (tmp_hard_iface == hard_iface)
> +			continue;
> +
> +		if (tmp_hard_iface->mesh_iface != mesh_iface)
>  			continue;
>  
> -		if (hard_iface->net_dev == net_dev)
> +		if (tmp_hard_iface->if_status == BATADV_IF_NOT_IN_USE)
>  			continue;
>  
> -		if (!batadv_compare_eth(hard_iface->net_dev->dev_addr,
> -					net_dev->dev_addr))
> +		if (!batadv_compare_eth(tmp_hard_iface->net_dev->dev_addr,
> +					hard_iface->net_dev->dev_addr))
>  			continue;
>  
> -		pr_warn("The newly added mac address (%pM) already exists on: %s\n",
> -			net_dev->dev_addr, hard_iface->net_dev->name);
> -		pr_warn("It is strongly recommended to keep mac addresses unique to avoid problems!\n");
> +		netdev_warn(hard_iface->net_dev,
> +			    "The newly added mac address (%pM) already exists on: %s\n",
> +			    hard_iface->net_dev->dev_addr, tmp_hard_iface->net_dev->name);
> +		netdev_warn(hard_iface->net_dev,
> +			    "It is strongly recommended to keep mac addresses unique to avoid problems!\n");
>  	}
> -	rcu_read_unlock();
>  }

I feel like the above code mixes unnecessarily fix and refactor
(variable rename, different print helper usage).

I think the fix should be minimal, the refactor should land in a
different patch for next.

/P