diff mbox

[RFC] ipv6: use a random ifid for headerless devices

Message ID 87d1ukk9ce.fsf@nemi.mork.no
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Bjørn Mork Dec. 5, 2015, 7:02 p.m. UTC 
Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
> On Thu, Dec 3, 2015, at 20:29, Bjørn Mork wrote:
>
>> After looking more at addrconf, I started wondering if we couldn't abuse
>> ipv6_generate_stable_address() for this purpose?  We could add a new
>> addr_gen_mode which would trigger automatic generation of a secret if
>> stable_secret is uninitialized.  This would be good enough to ensure
>> stability until the interface is destroyed.  And it would still allow
>> the adminstrator to select IN6_ADDR_GEN_MODE_STABLE_PRIVACY by entering
>> a new secret.
>
> I am fine with your proposal but I would really like to see it only
> happen on the per-interface stable_secret instance.

Do you think something like the patch below will be OK?

Or would it be better to drop the additional mode and just generate a
random secret if the mode is IN6_ADDR_GEN_MODE_STABLE_PRIVACY and the
secrets are missing?  Or would that be changing the userspace ABI?  This
is not clear to me...

Bjørn

Comments

Hannes Frederic Sowa Dec. 8, 2015, 1:44 p.m. UTC | #1 
On 05.12.2015 20:02, Bjørn Mork wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
>> On Thu, Dec 3, 2015, at 20:29, Bjørn Mork wrote:
>>
>>> After looking more at addrconf, I started wondering if we couldn't abuse
>>> ipv6_generate_stable_address() for this purpose?  We could add a new
>>> addr_gen_mode which would trigger automatic generation of a secret if
>>> stable_secret is uninitialized.  This would be good enough to ensure
>>> stability until the interface is destroyed.  And it would still allow
>>> the adminstrator to select IN6_ADDR_GEN_MODE_STABLE_PRIVACY by entering
>>> a new secret.
>>
>> I am fine with your proposal but I would really like to see it only
>> happen on the per-interface stable_secret instance.
> 
> Do you think something like the patch below will be OK?

I wouldn't call it IN6_ADDR_GEN_MODE_AUTO, this doesn't say anything.
But the idea is already good.

> Or would it be better to drop the additional mode and just generate a
> random secret if the mode is IN6_ADDR_GEN_MODE_STABLE_PRIVACY and the
> secrets are missing?  Or would that be changing the userspace ABI?  This
> is not clear to me...

I would not like to do that somehow. The problem is that the stable
secrets get written by user space probably during boot-up, but we don't
know when. That's why I would also not set the ->initialized flag, so
user can overwrite it to the final secret later on. We block it otherwise.

My proposal would be to use the stable privacy generator in case the
device does not have a device address for EUI-48 generation with a
secret which we simply generate on the stack. Let's factor out the part
of the generator which depends on the inet6_dev and cnf bits for that.

What do you think?

Bye,
Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Dec. 8, 2015, 6:57 p.m. UTC | #2 
Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
> On 05.12.2015 20:02, Bjørn Mork wrote:
>> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
>>> On Thu, Dec 3, 2015, at 20:29, Bjørn Mork wrote:
>>>
>>>> After looking more at addrconf, I started wondering if we couldn't abuse
>>>> ipv6_generate_stable_address() for this purpose?  We could add a new
>>>> addr_gen_mode which would trigger automatic generation of a secret if
>>>> stable_secret is uninitialized.  This would be good enough to ensure
>>>> stability until the interface is destroyed.  And it would still allow
>>>> the adminstrator to select IN6_ADDR_GEN_MODE_STABLE_PRIVACY by entering
>>>> a new secret.
>>>
>>> I am fine with your proposal but I would really like to see it only
>>> happen on the per-interface stable_secret instance.
>> 
>> Do you think something like the patch below will be OK?
>
> I wouldn't call it IN6_ADDR_GEN_MODE_AUTO, this doesn't say anything.
> But the idea is already good.

No, I didn't like that name either.  I just couldn't come up with
anything descriptive, short and non-redundant. "random", "generated",
"stable" are even worse.  And that's about where my imagination ended.
We need a child here :)

>> Or would it be better to drop the additional mode and just generate a
>> random secret if the mode is IN6_ADDR_GEN_MODE_STABLE_PRIVACY and the
>> secrets are missing?  Or would that be changing the userspace ABI?  This
>> is not clear to me...
>
> I would not like to do that somehow. The problem is that the stable
> secrets get written by user space probably during boot-up, but we don't
> know when. That's why I would also not set the ->initialized flag, so
> user can overwrite it to the final secret later on. We block it otherwise.

I am not sure I follow...  There is nothing preventing userspace from
initializing the secret before or after generation of the random secret.
Writing to /proc/sys/net/ipv6/conf/<iface>/stable_secret will update the
secret and set the mode to IN6_ADDR_GEN_MODE_STABLE_PRIVACY as before,
even if we have generated a random secret first.  I have verified that
this part works as expected.

I guess we should check &net->ipv6.devconf_dflt->stable_secret too
before choosing the default mode.  IN6_ADDR_GEN_MODE_STABLE_PRIVACY is a
more approproate default if a default secret is set.  IMHO, this should
really be the case without the proposed change too, but it isn't. The
current behaviour confuses me: Setting 'default' changes all existing
interfaces, but does not change the default for new interfaces. Is that
right?

> My proposal would be to use the stable privacy generator in case the
> device does not have a device address for EUI-48 generation with a
> secret which we simply generate on the stack. Let's factor out the part
> of the generator which depends on the inet6_dev and cnf bits for that.

Not sure I get this part either.  The point was to have stable addresses
for the lifetime of the netdev.  We can generate the secret on the
stack, but we will still need to stash it somewhere.  That could of
course be to a new field.  But I don't see the point since there is no
way you can combine this mode with IN6_ADDR_GEN_MODE_STABLE_PRIVACY.
Only one mode can be active at, and that mode can then own the secret.

As long as we can manage to introduce this without changing any existing
behaviour, of course.



Bjørn
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Hannes Frederic Sowa Dec. 14, 2015, 9:30 p.m. UTC | #3 
Hello,

On 08.12.2015 19:57, Bjørn Mork wrote:
> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
>> On 05.12.2015 20:02, Bjørn Mork wrote:
>>> Hannes Frederic Sowa <hannes@stressinduktion.org> writes:
>>>> On Thu, Dec 3, 2015, at 20:29, Bjørn Mork wrote:
>>>>
>>>>> After looking more at addrconf, I started wondering if we couldn't abuse
>>>>> ipv6_generate_stable_address() for this purpose?  We could add a new
>>>>> addr_gen_mode which would trigger automatic generation of a secret if
>>>>> stable_secret is uninitialized.  This would be good enough to ensure
>>>>> stability until the interface is destroyed.  And it would still allow
>>>>> the adminstrator to select IN6_ADDR_GEN_MODE_STABLE_PRIVACY by entering
>>>>> a new secret.
>>>>
>>>> I am fine with your proposal but I would really like to see it only
>>>> happen on the per-interface stable_secret instance.
>>>
>>> Do you think something like the patch below will be OK?
>>
>> I wouldn't call it IN6_ADDR_GEN_MODE_AUTO, this doesn't say anything.
>> But the idea is already good.
> 
> No, I didn't like that name either.  I just couldn't come up with
> anything descriptive, short and non-redundant. "random", "generated",
> "stable" are even worse.  And that's about where my imagination ended.
> We need a child here :)

Sorry for answering so late...

What do you think about simply using IN6_ADDR_GEN_MODE_RANDOM?

>>> Or would it be better to drop the additional mode and just generate a
>>> random secret if the mode is IN6_ADDR_GEN_MODE_STABLE_PRIVACY and the
>>> secrets are missing?  Or would that be changing the userspace ABI?  This
>>> is not clear to me...
>>
>> I would not like to do that somehow. The problem is that the stable
>> secrets get written by user space probably during boot-up, but we don't
>> know when. That's why I would also not set the ->initialized flag, so
>> user can overwrite it to the final secret later on. We block it otherwise.
> 
> I am not sure I follow...  There is nothing preventing userspace from
> initializing the secret before or after generation of the random secret.

I actually missed that. Shortly before sending the patch I decided to
allow to reinitialize the stable_secret. Before I had a check in there
to not being able to rewrite the stable_secret after it became
initialized. So we are good here. Sorry for the confusion.

> Writing to /proc/sys/net/ipv6/conf/<iface>/stable_secret will update the
> secret and set the mode to IN6_ADDR_GEN_MODE_STABLE_PRIVACY as before,
> even if we have generated a random secret first.  I have verified that
> this part works as expected.

Thanks!

> I guess we should check &net->ipv6.devconf_dflt->stable_secret too
> before choosing the default mode.  IN6_ADDR_GEN_MODE_STABLE_PRIVACY is a
> more approproate default if a default secret is set.  IMHO, this should
> really be the case without the proposed change too, but it isn't. The
> current behaviour confuses me: Setting 'default' changes all existing
> interfaces, but does not change the default for new interfaces. Is that
> right?

Nope, that is a good point. I think we should do that unconditionally.
If we have a stable secret set, which we can use, we always should use
this address generation mode. Can you send the addition of this as a
separate patch so we can propose it for stable? Otherwise I can do that,
too.

>> My proposal would be to use the stable privacy generator in case the
>> device does not have a device address for EUI-48 generation with a
>> secret which we simply generate on the stack. Let's factor out the part
>> of the generator which depends on the inet6_dev and cnf bits for that.
> 
> Not sure I get this part either.  The point was to have stable addresses
> for the lifetime of the netdev.  We can generate the secret on the
> stack, but we will still need to stash it somewhere.  That could of
> course be to a new field.  But I don't see the point since there is no
> way you can combine this mode with IN6_ADDR_GEN_MODE_STABLE_PRIVACY.
> Only one mode can be active at, and that mode can then own the secret.

Ok, your argument makes sense.

> As long as we can manage to introduce this without changing any existing
> behaviour, of course.

Besides the naming I think your patch looks fine.

Thank you,
Hannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bjørn Mork Dec. 14, 2015, 9:43 p.m. UTC | #4 
Hannes Frederic Sowa <hannes@stressinduktion.org> writes:

> Sorry for answering so late...

No problem.  There is no rush here AFAICS.  Thanks for taking the time
to look at this.

> What do you think about simply using IN6_ADDR_GEN_MODE_RANDOM?

Yes, that's fine with me (actually what I first used :)

>> I guess we should check &net->ipv6.devconf_dflt->stable_secret too
>> before choosing the default mode.  IN6_ADDR_GEN_MODE_STABLE_PRIVACY is a
>> more approproate default if a default secret is set.  IMHO, this should
>> really be the case without the proposed change too, but it isn't. The
>> current behaviour confuses me: Setting 'default' changes all existing
>> interfaces, but does not change the default for new interfaces. Is that
>> right?
>
> Nope, that is a good point. I think we should do that unconditionally.
> If we have a stable secret set, which we can use, we always should use
> this address generation mode. Can you send the addition of this as a
> separate patch so we can propose it for stable? Otherwise I can do that,
> too.

I can do that if it can wait for whenever I get around to actually
submit this.  No guarantee that will be in time for v4.5.


>>> My proposal would be to use the stable privacy generator in case the
>>> device does not have a device address for EUI-48 generation with a
>>> secret which we simply generate on the stack. Let's factor out the part
>>> of the generator which depends on the inet6_dev and cnf bits for that.
>> 
>> Not sure I get this part either.  The point was to have stable addresses
>> for the lifetime of the netdev.  We can generate the secret on the
>> stack, but we will still need to stash it somewhere.  That could of
>> course be to a new field.  But I don't see the point since there is no
>> way you can combine this mode with IN6_ADDR_GEN_MODE_STABLE_PRIVACY.
>> Only one mode can be active at, and that mode can then own the secret.
>
> Ok, your argument makes sense.
>
>> As long as we can manage to introduce this without changing any existing
>> behaviour, of course.
>
> Besides the naming I think your patch looks fine.

Thanks!  Will fixup that and formally submit when I find some time.


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

Patch

From 099c0a856cc4d06d2da6eb7f1f684adff9820f65 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Bj=C3=B8rn=20Mork?= <bjorn@mork.no>
Date: Fri, 4 Dec 2015 13:30:13 +0100
Subject: [PATCH] ipv6: addrconf: use stable address generator for ARPHRD_NONE
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Add a new address generator mode, using the stable address
generator with an automatically generated secret. Set it up
as the default address generator for ARPHRD_NONE interfaces,
which cannot use the EUI generator.

A random secret is generated on first link up event, allowing
a link local address to be added:

5: tun0: <POINTOPOINT,MULTICAST,NOARP> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 500
    link/none  promiscuity 0
    tun addrgenmode auto
cat: /proc/sys/net/ipv6/conf/tun0/stable_secret: Input/output error
b269:f3cc:e285:3de3:d9e5:ac1c:bf76:a7ca
5: tun0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN group default qlen 500
    link/none  promiscuity 0
    tun
    inet6 fe80::19d4:c711:c8d3:451d/64 scope link flags 800
       valid_lft forever preferred_lft forever

Manually configuring a secret changes the mode to
stable-privacy:

A:~# echo :: > /proc/sys/net/ipv6/conf/tun0/stable_secret
A:~# ip -d link show dev tun0
5: tun0: <POINTOPOINT,MULTICAST,NOARP,UP,LOWER_UP> mtu 1500 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 500
    link/none  promiscuity 0
    tun addrgenmode stable-privacy

Changing the mode to auto will not change an existing secret.

Signed-off-by: Bjørn Mork <bjorn@mork.no>
---
 include/uapi/linux/if_link.h |  1 +
 net/ipv6/addrconf.c          | 43 +++++++++++++++++++++++++++++++++++++------
 2 files changed, 38 insertions(+), 6 deletions(-)

diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index 5ad5737..999d765 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -218,6 +218,7 @@  enum in6_addr_gen_mode {
 	IN6_ADDR_GEN_MODE_EUI64,
 	IN6_ADDR_GEN_MODE_NONE,
 	IN6_ADDR_GEN_MODE_STABLE_PRIVACY,
+	IN6_ADDR_GEN_MODE_AUTO,
 };
 
 /* Bridge section */
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index d84742f..1b12ff3 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -393,6 +393,9 @@  static struct inet6_dev *ipv6_add_dev(struct net_device *dev)
 		ndev->cnf.rtr_solicits = 0;
 	}
 #endif
+	/* this device type does not support the default EUI mode */
+	if (dev->type == ARPHRD_NONE)
+		ndev->addr_gen_mode = IN6_ADDR_GEN_MODE_AUTO;
 
 	INIT_LIST_HEAD(&ndev->tempaddr_list);
 	setup_timer(&ndev->regen_timer, ipv6_regen_rndid, (unsigned long)ndev);
@@ -2314,6 +2317,12 @@  static void manage_tempaddrs(struct inet6_dev *idev,
 	}
 }
 
+static bool is_addr_mode_generate_stable(struct inet6_dev *idev)
+{
+	return idev->addr_gen_mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY ||
+	       idev->addr_gen_mode == IN6_ADDR_GEN_MODE_AUTO;
+}
+
 void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 {
 	struct prefix_info *pinfo;
@@ -2427,8 +2436,7 @@  void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
 				       in6_dev->token.s6_addr + 8, 8);
 				read_unlock_bh(&in6_dev->lock);
 				tokenized = true;
-			} else if (in6_dev->addr_gen_mode ==
-				   IN6_ADDR_GEN_MODE_STABLE_PRIVACY &&
+			} else if (is_addr_mode_generate_stable(in6_dev) &&
 				   !ipv6_generate_stable_address(&addr, 0,
 								 in6_dev)) {
 				addr_flags |= IFA_F_STABLE_PRIVACY;
@@ -3028,6 +3036,17 @@  retry:
 	return 0;
 }
 
+static void ipv6_gen_mode_auto_init(struct inet6_dev *idev)
+{
+	struct ipv6_stable_secret *s = &idev->cnf.stable_secret;
+
+	if (s->initialized)
+		return;
+	s = &idev->cnf.stable_secret;
+	get_random_bytes(&s->secret, sizeof(s->secret));
+	s->initialized = true;
+}
+
 static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 {
 	struct in6_addr addr;
@@ -3038,13 +3057,18 @@  static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 
 	ipv6_addr_set(&addr, htonl(0xFE800000), 0, 0, 0);
 
-	if (idev->addr_gen_mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY) {
+	switch (idev->addr_gen_mode) {
+	case IN6_ADDR_GEN_MODE_AUTO:
+		ipv6_gen_mode_auto_init(idev);
+		/* fallthrough */
+	case IN6_ADDR_GEN_MODE_STABLE_PRIVACY:
 		if (!ipv6_generate_stable_address(&addr, 0, idev))
 			addrconf_add_linklocal(idev, &addr,
 					       IFA_F_STABLE_PRIVACY);
 		else if (prefix_route)
 			addrconf_prefix_route(&addr, 64, idev->dev, 0, 0);
-	} else if (idev->addr_gen_mode == IN6_ADDR_GEN_MODE_EUI64) {
+		break;
+	case IN6_ADDR_GEN_MODE_EUI64:
 		/* addrconf_add_linklocal also adds a prefix_route and we
 		 * only need to care about prefix routes if ipv6_generate_eui64
 		 * couldn't generate one.
@@ -3053,6 +3077,11 @@  static void addrconf_addr_gen(struct inet6_dev *idev, bool prefix_route)
 			addrconf_add_linklocal(idev, &addr, 0);
 		else if (prefix_route)
 			addrconf_prefix_route(&addr, 64, idev->dev, 0, 0);
+		break;
+	case IN6_ADDR_GEN_MODE_NONE:
+	default:
+		/* will not add any link local address */
+		break;
 	}
 }
 
@@ -3069,7 +3098,8 @@  static void addrconf_dev_config(struct net_device *dev)
 	    (dev->type != ARPHRD_IEEE802154) &&
 	    (dev->type != ARPHRD_IEEE1394) &&
 	    (dev->type != ARPHRD_TUNNEL6) &&
-	    (dev->type != ARPHRD_6LOWPAN)) {
+	    (dev->type != ARPHRD_6LOWPAN) &&
+	    (dev->type != ARPHRD_NONE)) {
 		/* Alas, we support only Ethernet autoconfiguration. */
 		return;
 	}
@@ -4921,7 +4951,8 @@  static int inet6_set_link_af(struct net_device *dev, const struct nlattr *nla)
 
 		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 &&
+		    mode != IN6_ADDR_GEN_MODE_AUTO)
 			return -EINVAL;
 
 		if (mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY &&
-- 
2.1.4