netdev - Re: [PATCH net-next] net: ipv6: add tokenized interface identifier support

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: <20130408222154.GA25696@order.stressinduktion.org>
Date:	Tue, 9 Apr 2013 00:21:54 +0200
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Daniel Borkmann <dborkman@...hat.com>
Cc:	davem@...emloft.net, netdev@...r.kernel.org,
	YOSHIFUJI Hideaki <yoshfuji@...ux-ipv6.org>,
	Thomas Graf <tgraf@...g.ch>
Subject: Re: [PATCH net-next] net: ipv6: add tokenized interface identifier support

Sorry, I was a bit busy and just had more time to look at your patch
again. Perhaps you could look into my comments. (A new patch would be
needed as it already landed in net-next).

On Mon, Apr 08, 2013 at 04:01:30PM +0200, Daniel Borkmann wrote:
> diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
> index a33b157..65d8139 100644
> --- a/net/ipv6/addrconf.c
> +++ b/net/ipv6/addrconf.c
> @@ -422,6 +422,7 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
>  		ipv6_regen_rndid((unsigned long) ndev);
>  	}
>  #endif
> +	memset(ndev->token.s6_addr, 0, sizeof(ndev->token.s6_addr));

ndev is allocated with __GFP_ZERO so no need to clear it. Otherwise I would do

ndev->token = in6addr_any;

to make the check in addrconf_prefix_rcv more clear.

> +static int inet6_set_iftoken(struct inet6_dev *idev, struct in6_addr *token)
> +{
> +	struct in6_addr ll_addr;
> +	struct inet6_ifaddr *ifp;
> +	struct net_device *dev = idev->dev;
> +
> +	if (token == NULL)
> +		return -EINVAL;
> +	if (ipv6_addr_any(token))
> +		return -EINVAL;
> +	if (dev->flags & (IFF_LOOPBACK | IFF_NOARP))
> +		return -EINVAL;
> +	if (idev->dead || !(idev->if_flags & IF_READY))
> +		return -EINVAL;
> +	if (!ipv6_accept_ra(idev))
> +		return -EINVAL;
> +	if (idev->cnf.rtr_solicits <= 0)
> +		return -EINVAL;

IF_READY marks an interface which is already up. So we are not allowed to set
a token on an interface which is down? I would drop this requirement, seems
like a usability issue. ;)

> +
> +	write_lock_bh(&idev->lock);
> +
> +	BUILD_BUG_ON(sizeof(token->s6_addr) != 16);
> +	memcpy(idev->token.s6_addr + 8, token->s6_addr + 8, 8);
> +
> +	write_unlock_bh(&idev->lock);
> +

{
> +	ipv6_get_lladdr(dev, &ll_addr, IFA_F_TENTATIVE | IFA_F_OPTIMISTIC);
> +	ndisc_send_rs(dev, &ll_addr, &in6addr_linklocal_allrouters);
> +
> +	write_lock_bh(&idev->lock);
> +	idev->if_flags |= IF_RS_SENT;
}

This should then be only called if IF_READY is set. Otherwise normal ifup
handling will send out the rs. If one day there is the possibility to add more
than one token we would actually have to check the minimum solicitation
intervals. I think this does not matter now.

> +
> +	/* Well, that's kinda nasty ... */
> +	list_for_each_entry(ifp, &idev->addr_list, if_list) {
> +		spin_lock(&ifp->lock);
> +		if (ipv6_addr_src_scope(&ifp->addr) ==
> +		    IPV6_ADDR_SCOPE_GLOBAL) {
> +			ifp->valid_lft = 0;
> +			ifp->prefered_lft = 0;
> +		}
> +		spin_unlock(&ifp->lock);
> +	}

As I understand this logic it also does deprecate current statically configured ip
addresses? Perhaps another per-inet6_ifaddr flag to mark the ip address as
token configured and just clean these address.

The flag would have to be set in addrconf_prefix_rcv if tokens are active.

Thanks, 

  Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html