netdev - Re: [PATCH net-next] ipv6: allow userspace to add IFA_F

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, 20 Feb 2018 10:25:41 -0700
From:   David Ahern <dsahern@...il.com>
To:     Sabrina Dubroca <sd@...asysnail.net>, netdev@...r.kernel.org
Subject: Re: [PATCH net-next] ipv6: allow userspace to add IFA_F_OPTIMISTIC
 addresses

On 2/20/18 9:43 AM, Sabrina Dubroca wrote:
> According to RFC 4429 (section 3.1), adding new IPv6 addresses as
> optimistic addresses is acceptable, as long as the implementation
> follows some rules:
> 
>    * Optimistic DAD SHOULD only be used when the implementation is aware
>         that the address is based on a most likely unique interface
>         identifier (such as in [RFC2464]), generated randomly [RFC3041],
>         or by a well-distributed hash function [RFC3972] or assigned by
>         Dynamic Host Configuration Protocol for IPv6 (DHCPv6) [RFC3315].
>         Optimistic DAD SHOULD NOT be used for manually entered
>         addresses.

That last line suggests this patch should not be allowed.

But if it is ...

> 
> Thus, it seems reasonable to allow userspace to set the optimistic flag
> when adding new addresses.
> 
> We must not let userspace set NODAD + OPTIMISTIC, since if the kernel is
> not performing DAD we would never clear the optimistic flag. We must
> also ignore userspace's request to add OPTIMISTIC flag to addresses that
> have already completed DAD.
> 
> Then we also need to clear the OPTIMISTIC flag on permanent addresses
> when DAD fails. Otherwise, IFA_F_OPTIMISTIC addresses added by userspace
> can still be used after DAD has failed, because in
> ipv6_chk_addr_and_flags(), IFA_F_OPTIMISTIC overrides IFA_F_TENTATIVE.
> 
> Signed-off-by: Sabrina Dubroca <sd@...asysnail.net>
> ---
>  net/ipv6/addrconf.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index 4facfe0b1888..652285bae801 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -1968,6 +1968,7 @@ static void addrconf_dad_stop(struct inet6_ifaddr *ifp, int dad_failed)
>  		spin_lock_bh(&ifp->lock);
>  		addrconf_del_dad_work(ifp);
>  		ifp->flags |= IFA_F_TENTATIVE;
> +		ifp->flags &= ~IFA_F_OPTIMISTIC;
>  		spin_unlock_bh(&ifp->lock);
>  		if (dad_failed)
>  			ipv6_ifa_notify(0, ifp);
> @@ -4501,6 +4502,9 @@ static int inet6_addr_modify(struct inet6_ifaddr *ifp, u32 ifa_flags,
>  	    (ifp->flags & IFA_F_TEMPORARY || ifp->prefix_len != 64))
>  		return -EINVAL;
>  
> +	if (!(ifp->flags & (IFA_F_TENTATIVE | IFA_F_DADFAILED)))
> +		ifa_flags &= ~IFA_F_OPTIMISTIC;
> +
>  	timeout = addrconf_timeout_fixup(valid_lft, HZ);
>  	if (addrconf_finite_timeout(timeout)) {
>  		expires = jiffies_to_clock_t(timeout * HZ);
> @@ -4607,7 +4611,10 @@ inet6_rtm_newaddr(struct sk_buff *skb, struct nlmsghdr *nlh,
>  
>  	/* We ignore other flags so far. */
>  	ifa_flags &= IFA_F_NODAD | IFA_F_HOMEADDRESS | IFA_F_MANAGETEMPADDR |
> -		     IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN;
> +		     IFA_F_NOPREFIXROUTE | IFA_F_MCAUTOJOIN | IFA_F_OPTIMISTIC;
> +
> +	if (ifa_flags & IFA_F_NODAD && ifa_flags & IFA_F_OPTIMISTIC)
> +		return -EINVAL;

... add an extack message telling users nodad and optimistic are
mutually exclusive.


Also, it seems like this feature needs to be wrapped in
CONFIG_IPV6_OPTIMISTIC_DAD and optimistic checks for linklocal and
autoconf are wrapped in sysctl checks. Why shouldn't manual addresses
follow suit?

>  
>  	ifa = ipv6_get_ifaddr(net, pfx, dev, 1);
>  	if (!ifa) {
>