netdev - Re: [net-next] net: change accept_ra_min_rtr

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]

Date: Tue, 25 Jul 2023 17:28:12 -0600
From: David Ahern <dsahern@...nel.org>
To: Patrick Rohr <prohr@...gle.com>, "David S . Miller" <davem@...emloft.net>
Cc: Linux Network Development Mailing List <netdev@...r.kernel.org>,
 Maciej Żenczykowski <maze@...gle.com>,
 Lorenzo Colitti <lorenzo@...gle.com>
Subject: Re: [net-next] net: change accept_ra_min_rtr_lft to affect all RA
 lifetimes

On 7/25/23 12:31 PM, Patrick Rohr wrote:
> @@ -2727,6 +2727,11 @@ void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
>  		return;
>  	}
>  
> +	if (valid_lft != 0 && valid_lft < in6_dev->cnf.accept_ra_min_lft) {
> +		net_info_ratelimited("addrconf: prefix option lifetime too short\n");

The error message does not really provide any value besides spamming the
logs. Similar comment applies to existing error logging in that function
too. I think a counter for invalid prefix packets would be more useful.

> +		return;
> +	}
> +
>  	/*
>  	 *	Two things going on here:
>  	 *	1) Add routes for on-link prefixes
> @@ -5598,7 +5603,7 @@ static inline void ipv6_store_devconf(struct ipv6_devconf *cnf,
>  	array[DEVCONF_IOAM6_ID_WIDE] = cnf->ioam6_id_wide;
>  	array[DEVCONF_NDISC_EVICT_NOCARRIER] = cnf->ndisc_evict_nocarrier;
>  	array[DEVCONF_ACCEPT_UNTRACKED_NA] = cnf->accept_untracked_na;
> -	array[DEVCONF_ACCEPT_RA_MIN_RTR_LFT] = cnf->accept_ra_min_rtr_lft;
> +	array[DEVCONF_ACCEPT_RA_MIN_LFT] = cnf->accept_ra_min_lft;
>  }
>  
>  static inline size_t inet6_ifla6_size(void)
> @@ -6793,8 +6798,8 @@ static const struct ctl_table addrconf_sysctl[] = {
>  		.proc_handler	= proc_dointvec,
>  	},
>  	{
> -		.procname	= "accept_ra_min_rtr_lft",
> -		.data		= &ipv6_devconf.accept_ra_min_rtr_lft,
> +		.procname	= "accept_ra_min_lft",
> +		.data		= &ipv6_devconf.accept_ra_min_lft,
>  		.maxlen		= sizeof(int),
>  		.mode		= 0644,
>  		.proc_handler	= proc_dointvec,
> diff --git a/net/ipv6/ndisc.c b/net/ipv6/ndisc.c
> index 29ddad1c1a2f..eeb60888187f 100644
> --- a/net/ipv6/ndisc.c
> +++ b/net/ipv6/ndisc.c
> @@ -1280,8 +1280,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>  	if (!ndisc_parse_options(skb->dev, opt, optlen, &ndopts))
>  		return SKB_DROP_REASON_IPV6_NDISC_BAD_OPTIONS;
>  
> -	lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
> -
>  	if (!ipv6_accept_ra(in6_dev)) {
>  		ND_PRINTK(2, info,
>  			  "RA: %s, did not accept ra for dev: %s\n",
> @@ -1289,13 +1287,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>  		goto skip_linkparms;
>  	}
>  
> -	if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_rtr_lft) {
> -		ND_PRINTK(2, info,
> -			  "RA: router lifetime (%ds) is too short: %s\n",
> -			  lifetime, skb->dev->name);
> -		goto skip_linkparms;
> -	}
> -
>  #ifdef CONFIG_IPV6_NDISC_NODETYPE
>  	/* skip link-specific parameters from interior routers */
>  	if (skb->ndisc_nodetype == NDISC_NODETYPE_NODEFAULT) {
> @@ -1336,6 +1327,14 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>  		goto skip_defrtr;
>  	}
>  
> +	lifetime = ntohs(ra_msg->icmph.icmp6_rt_lifetime);
> +	if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_lft) {
> +		ND_PRINTK(2, info,
> +			  "RA: router lifetime (%ds) is too short: %s\n",
> +			  lifetime, skb->dev->name);
> +		goto skip_defrtr;
> +	}
> +
>  	/* Do not accept RA with source-addr found on local machine unless
>  	 * accept_ra_from_local is set to true.
>  	 */
> @@ -1499,13 +1498,6 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>  		goto out;
>  	}
>  
> -	if (lifetime != 0 && lifetime < in6_dev->cnf.accept_ra_min_rtr_lft) {
> -		ND_PRINTK(2, info,
> -			  "RA: router lifetime (%ds) is too short: %s\n",
> -			  lifetime, skb->dev->name);
> -		goto out;
> -	}
> -

The commit mentioned in the Fixes was just applied and you are already
sending a follow up moving the same code around again.

>  #ifdef CONFIG_IPV6_ROUTE_INFO
>  	if (!in6_dev->cnf.accept_ra_from_local &&
>  	    ipv6_chk_addr(dev_net(in6_dev->dev), &ipv6_hdr(skb)->saddr,
> @@ -1530,6 +1522,9 @@ static enum skb_drop_reason ndisc_router_discovery(struct sk_buff *skb)
>  			if (ri->prefix_len == 0 &&
>  			    !in6_dev->cnf.accept_ra_defrtr)
>  				continue;
> +			if (ri->lifetime != 0 &&
> +			    ntohl(ri->lifetime) < in6_dev->cnf.accept_ra_min_lft)
> +				continue;
>  			if (ri->prefix_len < in6_dev->cnf.accept_ra_rt_info_min_plen)
>  				continue;
>  			if (ri->prefix_len > in6_dev->cnf.accept_ra_rt_info_max_plen)