[PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[Bram Bonné]

IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC behaves like the existing
IN6_ADDR_GEN_MODE_STABLE_PRIVACY mode, but uses the software-defined MAC
address (dev_addr) instead of the permanent, hardware-defined MAC
address (perm_addr) when generating IPv6 link-local addresses.

This mode allows the IPv6 link-local address to change in line with the
MAC address when per-network MAC address randomization is used. In this
case, the MAC address fulfills the role of both the Net_Iface and the
Network_ID parameters in RFC7217.

Signed-off-by: Bram Bonné <brambonne@google.com>
---
 include/uapi/linux/if_link.h |  1 +
 net/ipv6/addrconf.c          | 29 ++++++++++++++++++++++++-----
 2 files changed, 25 insertions(+), 5 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index a009365ad67b..0de71cfdcd84 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -240,6 +240,7 @@ enum in6_addr_gen_mode {
 	IN6_ADDR_GEN_MODE_NONE,
 	IN6_ADDR_GEN_MODE_STABLE_PRIVACY,
 	IN6_ADDR_GEN_MODE_RANDOM,
+	IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC,
 };
 
 /* Bridge section */
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index ab7e839753ae..02d999ca332c 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -142,6 +142,7 @@ static int ipv6_count_addresses(const struct inet6_dev *idev);
 static int ipv6_generate_stable_address(struct in6_addr *addr,
 					u8 dad_count,
 					const struct inet6_dev *idev);
+static bool ipv6_addr_gen_use_softmac(const struct inet6_dev *idev);
 
 #define IN6_ADDR_HSIZE_SHIFT	8
 #define IN6_ADDR_HSIZE		(1 << IN6_ADDR_HSIZE_SHIFT)
@@ -381,7 +382,8 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 	timer_setup(&ndev->rs_timer, addrconf_rs_timer, 0);
 	memcpy(&ndev->cnf, dev_net(dev)->ipv6.devconf_dflt, sizeof(ndev->cnf));
 
-	if (ndev->cnf.stable_secret.initialized)
+	if (ndev->cnf.stable_secret.initialized &&
+	    !ipv6_addr_gen_use_softmac(ndev))
 		ndev->cnf.addr_gen_mode = IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
 
 	ndev->cnf.mtu6 = dev->mtu;
@@ -2540,6 +2542,8 @@ static void manage_tempaddrs(struct inet6_dev *idev,
 static bool is_addr_mode_generate_stable(struct inet6_dev *idev)
 {
 	return idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY ||
+	       idev->cnf.addr_gen_mode ==
+		       IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC ||
 	       idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_RANDOM;
 }
 
@@ -3191,6 +3195,12 @@ static bool ipv6_reserved_interfaceid(struct in6_addr address)
 	return false;
 }
 
+static inline bool ipv6_addr_gen_use_softmac(const struct inet6_dev *idev)
+{
+	return idev->cnf.addr_gen_mode ==
+	    IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC;
+}
+
 static int ipv6_generate_stable_address(struct in6_addr *address,
 					u8 dad_count,
 					const struct inet6_dev *idev)
@@ -3212,6 +3222,7 @@ static int ipv6_generate_stable_address(struct in6_addr *address,
 	struct in6_addr secret;
 	struct in6_addr temp;
 	struct net *net = dev_net(idev->dev);
+	unsigned char *hwaddr;
 
 	BUILD_BUG_ON(sizeof(data.__data) != sizeof(data));
 
@@ -3222,13 +3233,16 @@ static int ipv6_generate_stable_address(struct in6_addr *address,
 	else
 		return -1;
 
+	hwaddr = ipv6_addr_gen_use_softmac(idev) ?
+			idev->dev->dev_addr : idev->dev->perm_addr;
+
 retry:
 	spin_lock_bh(&lock);
 
 	sha_init(digest);
 	memset(&data, 0, sizeof(data));
 	memset(workspace, 0, sizeof(workspace));
-	memcpy(data.hwaddr, idev->dev->perm_addr, idev->dev->addr_len);
+	memcpy(data.hwaddr, hwaddr, idev->dev->addr_len);
 	data.prefix[0] = address->s6_addr32[0];
 	data.prefix[1] = address->s6_addr32[1];
 	data.secret = secret;
@@ -3283,6 +3297,7 @@ static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 		ipv6_gen_mode_random_init(idev);
 		fallthrough;
 	case IN6_ADDR_GEN_MODE_STABLE_PRIVACY:
+	case IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC:
 		if (!ipv6_generate_stable_address(&addr, 0, idev))
 			addrconf_add_linklocal(idev, &addr,
 					       IFA_F_STABLE_PRIVACY);
@@ -5726,6 +5741,7 @@ static int check_addr_gen_mode(int mode)
 	if (mode != IN6_ADDR_GEN_MODE_EUI64 &&
 	    mode != IN6_ADDR_GEN_MODE_NONE &&
 	    mode != IN6_ADDR_GEN_MODE_STABLE_PRIVACY &&
+	    mode != IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC &&
 	    mode != IN6_ADDR_GEN_MODE_RANDOM)
 		return -EINVAL;
 	return 1;
@@ -5734,7 +5750,8 @@ static int check_addr_gen_mode(int mode)
 static int check_stable_privacy(struct inet6_dev *idev, struct net *net,
 				int mode)
 {
-	if (mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY &&
+	if ((mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY ||
+	     mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY) &&
 	    !idev->cnf.stable_secret.initialized &&
 	    !net->ipv6.devconf_dflt->stable_secret.initialized)
 		return -EINVAL;
@@ -6355,7 +6372,7 @@ static int addrconf_sysctl_stable_secret(struct ctl_table *ctl, int write,
 		for_each_netdev(net, dev) {
 			struct inet6_dev *idev = __in6_dev_get(dev);
 
-			if (idev) {
+			if (idev && !ipv6_addr_gen_use_softmac(idev)) {
 				idev->cnf.addr_gen_mode =
 					IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
 			}
@@ -6363,7 +6380,9 @@ static int addrconf_sysctl_stable_secret(struct ctl_table *ctl, int write,
 	} else {
 		struct inet6_dev *idev = ctl->extra1;
 
-		idev->cnf.addr_gen_mode = IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
+		if (idev && !ipv6_addr_gen_use_softmac(idev))
+			idev->cnf.addr_gen_mode =
+				IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
 	}
 
 out:
-- 
2.26.2.761.g0e0b3e54be-goog

Re: [PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[Lorenzo Colitti]

On Tue, May 19, 2020 at 9:08 PM Bram Bonné <brambonne@google.com> wrote:
> @@ -381,7 +382,8 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
>         timer_setup(&ndev->rs_timer, addrconf_rs_timer, 0);
>         memcpy(&ndev->cnf, dev_net(dev)->ipv6.devconf_dflt, sizeof(ndev->cnf));
>
> -       if (ndev->cnf.stable_secret.initialized)
> +       if (ndev->cnf.stable_secret.initialized &&
> +           !ipv6_addr_gen_use_softmac(ndev))
>                 ndev->cnf.addr_gen_mode = IN6_ADDR_GEN_MODE_STABLE_PRIVACY;

Looks like if stable_secret is set, then when the interface is brought
up it defaults to stable privacy addresses. But if
ipv6_addr_gen_use_softmac(), then this remains unset (which means...
EUI-64?) Any reason you don't set it to
IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC in this case?

> @@ -6355,7 +6372,7 @@ static int addrconf_sysctl_stable_secret(struct ctl_table *ctl, int write,
>                 for_each_netdev(net, dev) {
>                         struct inet6_dev *idev = __in6_dev_get(dev);
>
> -                       if (idev) {
> +                       if (idev && !ipv6_addr_gen_use_softmac(idev)) {
>                                 idev->cnf.addr_gen_mode =
>                                         IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
>                         }
> @@ -6363,7 +6380,9 @@ static int addrconf_sysctl_stable_secret(struct ctl_table *ctl, int write,
>         } else {
>                 struct inet6_dev *idev = ctl->extra1;
>
> -               idev->cnf.addr_gen_mode = IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
> +               if (idev && !ipv6_addr_gen_use_softmac(idev))
> +                       idev->cnf.addr_gen_mode =
> +                               IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
>         }

... and in these two as well?

Since you haven't changed the netlink code, I assume that this address
is going to appear to userspace as IFA_F_STABLE_PRIVACY. I assume
that's what we want here? It's not really "stable", it's only as
stable as the MAC address. Does the text of the RFC support this
definition of "stable"?

Can it happen that the MAC address when the device is up and an IPv6
address already exists? If so, what happens to the address? Will the
system create a second stable privacy address when the next RA
arrives? That seems bad. But perhaps this cannot happen.

Re: [PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[Bram Bonné]

Thanks for your comments Lorenzo!

On Tue, May 19, 2020 at 6:11 PM Lorenzo Colitti <lorenzo@google.com> wrote:
>
> On Tue, May 19, 2020 at 9:08 PM Bram Bonné <brambonne@google.com> wrote:
> > @@ -381,7 +382,8 @@ static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
> >         timer_setup(&ndev->rs_timer, addrconf_rs_timer, 0);
> >         memcpy(&ndev->cnf, dev_net(dev)->ipv6.devconf_dflt, sizeof(ndev->cnf));
> >
> > -       if (ndev->cnf.stable_secret.initialized)
> > +       if (ndev->cnf.stable_secret.initialized &&
> > +           !ipv6_addr_gen_use_softmac(ndev))
> >                 ndev->cnf.addr_gen_mode = IN6_ADDR_GEN_MODE_STABLE_PRIVACY;
>
> Looks like if stable_secret is set, then when the interface is brought
> up it defaults to stable privacy addresses. But if
> ipv6_addr_gen_use_softmac(), then this remains unset (which means...
> EUI-64?) Any reason you don't set it to
> IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC in this case?

ipv6_addr_gen_use_softmac() means that addr_gen_mode is set to
IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC. I was debating making that
explicit here, instead of using the inline function. It sounds like
that might be a better idea to make the implementation clearer?

The intention here (and below) was to keep the existing default
behavior of using IN6_ADDR_GEN_MODE_STABLE_PRIVACY if stable_secret is
set, while allowing to override this behavior by setting add_gen_mode
to IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC. I tested this locally,
and it seems to work as expected, but please let me know if you think
the above approach does not make sense (or if you'd prefer
IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC to be the default instead).

> Since you haven't changed the netlink code, I assume that this address
> is going to appear to userspace as IFA_F_STABLE_PRIVACY. I assume
> that's what we want here? It's not really "stable", it's only as
> stable as the MAC address. Does the text of the RFC support this
> definition of "stable"?

The RFC considers the generated identifiers as: "stable for each
network interface within each subnet, but [changing] as a host moves
from one network to another". If the MAC address stays stable for a
specific subnet, this implementation fits that definition.
That being said, if you think it makes sense to define a separate
IFA_F_STABLE_PRIVACY_SOFTMAC flag (or some better name), I'm happy to
add that in.

> Can it happen that the MAC address when the device is up and an IPv6
> address already exists? If so, what happens to the address? Will the
> system create a second stable privacy address when the next RA
> arrives? That seems bad. But perhaps this cannot happen.

Trying to change the MAC when the device is up errors with EBUSY on my
machines, so I was under the assumption that the device needs to be
down to change the MAC. I could very well be wrong though.

Re: [PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[David Miller]

From: Bram Bonné <brambonne@google.com>
Date: Wed, 20 May 2020 15:16:53 +0200

> Trying to change the MAC when the device is up errors with EBUSY on my
> machines, so I was under the assumption that the device needs to be
> down to change the MAC. I could very well be wrong though.

Not all drivers/devices have this limitation, the generic code allows this
just fine.

Re: [PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[Bram Bonné]

On Wed, May 20, 2020 at 8:33 PM David Miller <davem@davemloft.net> wrote:
>
> From: Bram Bonné <brambonne@google.com>
> Date: Wed, 20 May 2020 15:16:53 +0200
>
> > Trying to change the MAC when the device is up errors with EBUSY on my
> > machines, so I was under the assumption that the device needs to be
> > down to change the MAC. I could very well be wrong though.
>
> Not all drivers/devices have this limitation, the generic code allows this
> just fine.
>

Thanks David. I was able to test the behavior of changing the MAC
while connected to a network. It does not seem to trigger address
generation, leaving the link-local address intact.

Do we know about any scenarios (apart from dev reconfiguration) that
would trigger address generation? My understanding based on the code
is that any other scenario would add an additional link-local address,
rather than removing the old one.

Re: [PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[Lorenzo Colitti]

On Wed, May 27, 2020 at 6:30 PM Bram Bonné <brambonne@google.com> wrote:
> Thanks David. I was able to test the behavior of changing the MAC
> while connected to a network. It does not seem to trigger address
> generation, leaving the link-local address intact.
>
> Do we know about any scenarios (apart from dev reconfiguration) that
> would trigger address generation? My understanding based on the code
> is that any other scenario would add an additional link-local address,
> rather than removing the old one.

I don't think the stack ever regenerates link-local addresses after
the first one. I think the question was what happens to global
addresses if the MAC address is changed and then an RA is received.
Will the stack create a new global IFA_F_STABLE_PRIVACY address, such
that there are now two stable privacy addresses on the same interface?

That seems strange, but still, I suppose you could say that the user
got what they asked for. They configured IPv6 addressing that is "as
stable as the MAC address", and then they changed the MAC address,
and, well, they got a new IPv6 address. Is there anything in RFC 7217
that prohibits or discourages this? If not, maybe it's fine.

Perhaps you can add a test for what happens by adding a test case here:

https://cs.android.com/android/platform/superproject/+/master:kernel/tests/net/test/multinetwork_test.py;l=796

I think you'll need to do that anyway in order to use this on Android.

Re: [PATCH] ipv6: Add IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC mode

[Lorenzo Colitti]

On Wed, May 20, 2020 at 10:17 PM Bram Bonné <brambonne@google.com> wrote:
> The intention here (and below) was to keep the existing default
> behavior of using IN6_ADDR_GEN_MODE_STABLE_PRIVACY if stable_secret is
> set, while allowing to override this behavior by setting add_gen_mode
> to IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC. I tested this locally,
> and it seems to work as expected, but please let me know if you think
> the above approach does not make sense (or if you'd prefer
> IN6_ADDR_GEN_MODE_STABLE_PRIVACY_SOFTMAC to be the default instead).

Yes, sorry, I think I was misreading the code.

> The RFC considers the generated identifiers as: "stable for each
> network interface within each subnet, but [changing] as a host moves
> from one network to another". If the MAC address stays stable for a
> specific subnet, this implementation fits that definition.

Makes sense to me.