netdev - Re: [PATCH net] bonding: guard ns_targets by CONFIG

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] [day] [month] [year] [list]
Message-ID: <85b6e408-6feb-02bf-fb19-bec3fbb419b9@redhat.com>
Date:   Mon, 30 May 2022 23:26:44 -0400
From:   Jonathan Toppins <jtoppins@...hat.com>
To:     Hangbin Liu <liuhangbin@...il.com>, netdev@...r.kernel.org
Cc:     Jay Vosburgh <j.vosburgh@...il.com>,
        Veaceslav Falico <vfalico@...il.com>,
        Andy Gospodarek <andy@...yhouse.net>,
        "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <eric.dumazet@...il.com>,
        Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net] bonding: guard ns_targets by CONFIG_IPV6

On 5/30/22 03:21, Hangbin Liu wrote:
> Guard ns_targets in struct bond_params by CONFIG_IPV6, which could save
> 256 bytes if IPv6 not configed. Also add this protection for function
> bond_is_ip6_target_ok() and bond_get_targets_ip6().
> 
> Remove the IS_ENABLED() check for bond_opts[] as this will make
> BOND_OPT_NS_TARGETS uninitialized if CONFIG_IPV6 not enabled. Add
> a dummy bond_option_ns_ip6_targets_set() for this situation.
> 
> Fixes: 4e24be018eb9 ("bonding: add new parameter ns_targets")
> Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> ---
>   drivers/net/bonding/bond_main.c    |  2 ++
>   drivers/net/bonding/bond_options.c | 10 ++++++----
>   include/net/bonding.h              |  6 ++++++
>   3 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index 3b7baaeae82c..f85372adf042 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -6159,7 +6159,9 @@ static int bond_check_params(struct bond_params *params)
>   		strscpy_pad(params->primary, primary, sizeof(params->primary));
>   
>   	memcpy(params->arp_targets, arp_target, sizeof(arp_target));
> +#if IS_ENABLED(CONFIG_IPV6)
>   	memset(params->ns_targets, 0, sizeof(struct in6_addr) * BOND_MAX_NS_TARGETS);
> +#endif
>   
>   	return 0;
>   }
> diff --git a/drivers/net/bonding/bond_options.c b/drivers/net/bonding/bond_options.c
> index 64f7db2627ce..a8a0c5a22517 100644
> --- a/drivers/net/bonding/bond_options.c
> +++ b/drivers/net/bonding/bond_options.c
> @@ -34,10 +34,8 @@ static int bond_option_arp_ip_target_add(struct bonding *bond, __be32 target);
>   static int bond_option_arp_ip_target_rem(struct bonding *bond, __be32 target);
>   static int bond_option_arp_ip_targets_set(struct bonding *bond,
>   					  const struct bond_opt_value *newval);
> -#if IS_ENABLED(CONFIG_IPV6)
>   static int bond_option_ns_ip6_targets_set(struct bonding *bond,
>   					  const struct bond_opt_value *newval);
> -#endif
>   static int bond_option_arp_validate_set(struct bonding *bond,
>   					const struct bond_opt_value *newval);
>   static int bond_option_arp_all_targets_set(struct bonding *bond,
> @@ -299,7 +297,6 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
>   		.flags = BOND_OPTFLAG_RAWVAL,
>   		.set = bond_option_arp_ip_targets_set
>   	},
> -#if IS_ENABLED(CONFIG_IPV6)
>   	[BOND_OPT_NS_TARGETS] = {
>   		.id = BOND_OPT_NS_TARGETS,
>   		.name = "ns_ip6_target",
> @@ -307,7 +304,6 @@ static const struct bond_option bond_opts[BOND_OPT_LAST] = {
>   		.flags = BOND_OPTFLAG_RAWVAL,
>   		.set = bond_option_ns_ip6_targets_set
>   	},
> -#endif
>   	[BOND_OPT_DOWNDELAY] = {
>   		.id = BOND_OPT_DOWNDELAY,
>   		.name = "downdelay",
> @@ -1254,6 +1250,12 @@ static int bond_option_ns_ip6_targets_set(struct bonding *bond,
>   
>   	return 0;
>   }
> +#else
> +static int bond_option_ns_ip6_targets_set(struct bonding *bond,
> +					  const struct bond_opt_value *newval)
> +{
> +	return 0;

I would return -EPERM here as the values cannot actually be set. You do 
not want userspace thinking the command was accepted when it actually 
didn't do anything with the values.

> +}
>   #endif
>   
>   static int bond_option_arp_validate_set(struct bonding *bond,
> diff --git a/include/net/bonding.h b/include/net/bonding.h
> index b14f4c0b4e9e..cb904d356e31 100644
> --- a/include/net/bonding.h
> +++ b/include/net/bonding.h
> @@ -149,7 +149,9 @@ struct bond_params {
>   	struct reciprocal_value reciprocal_packets_per_slave;
>   	u16 ad_actor_sys_prio;
>   	u16 ad_user_port_key;
> +#if IS_ENABLED(CONFIG_IPV6)
>   	struct in6_addr ns_targets[BOND_MAX_NS_TARGETS];
> +#endif
>   
>   	/* 2 bytes of padding : see ether_addr_equal_64bits() */
>   	u8 ad_actor_system[ETH_ALEN + 2];
> @@ -503,12 +505,14 @@ static inline int bond_is_ip_target_ok(__be32 addr)
>   	return !ipv4_is_lbcast(addr) && !ipv4_is_zeronet(addr);
>   }
>   
> +#if IS_ENABLED(CONFIG_IPV6)
>   static inline int bond_is_ip6_target_ok(struct in6_addr *addr)
>   {
>   	return !ipv6_addr_any(addr) &&
>   	       !ipv6_addr_loopback(addr) &&
>   	       !ipv6_addr_is_multicast(addr);
>   }
> +#endif
>   
>   /* Get the oldest arp which we've received on this slave for bond's
>    * arp_targets.
> @@ -746,6 +750,7 @@ static inline int bond_get_targets_ip(__be32 *targets, __be32 ip)
>   	return -1;
>   }
>   
> +#if IS_ENABLED(CONFIG_IPV6)
>   static inline int bond_get_targets_ip6(struct in6_addr *targets, struct in6_addr *ip)
>   {
>   	int i;
> @@ -758,6 +763,7 @@ static inline int bond_get_targets_ip6(struct in6_addr *targets, struct in6_addr
>   
>   	return -1;
>   }
> +#endif
>   
>   /* exported from bond_main.c */
>   extern unsigned int bond_net_id;