| 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
| ||
|
Message-ID: <1760aa03c22.ec63ccb8820202.7945714352225076012@shytyi.net>
Date: Fri, 27 Nov 2020 17:54:02 +0100
From: Dmytro Shytyi <dmytro@...tyi.net>
To: "Hideaki Yoshifuji" <hideaki.yoshifuji@...aclelinux.com>
Cc: "Jakub Kicinski" <kuba@...nel.org>,
"yoshfuji" <yoshfuji@...ux-ipv6.org>,
"kuznet" <kuznet@....inr.ac.ru>,
"liuhangbin" <liuhangbin@...il.com>, "davem" <davem@...emloft.net>,
"netdev" <netdev@...r.kernel.org>,
"linux-kernel" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH net-next V7] net: Variable SLAAC: SLAAC with prefixes of
arbitrary length in PIO
Hello,
---- On Mon, 23 Nov 2020 14:26:27 +0100 Hideaki Yoshifuji <hideaki.yoshifuji@...aclelinux.com> wrote ----
> Hi,
>
> 2020年11月20日(金) 18:28 Dmytro Shytyi <dmytro@...tyi.net>:
> >
> > Variable SLAAC: SLAAC with prefixes of arbitrary length in PIO (randomly
> > generated hostID or stable privacy + privacy extensions).
> > The main problem is that SLAAC RA or PD allocates a /64 by the Wireless
> > carrier 4G, 5G to a mobile hotspot, however segmentation of the /64 via
> > SLAAC is required so that downstream interfaces can be further subnetted.
> > Example: uCPE device (4G + WI-FI enabled) receives /64 via Wireless, and
> > assigns /72 to VNF-Firewall, /72 to WIFI, /72 to VNF-Router, /72 to
> > Load-Balancer and /72 to wired connected devices.
> > IETF document that defines problem statement:
> > draft-mishra-v6ops-variable-slaac-problem-stmt
> > IETF document that specifies variable slaac:
> > draft-mishra-6man-variable-slaac
> >
> > Signed-off-by: Dmytro Shytyi <dmytro@...tyi.net>
> > ---
> > diff -rupN net-next-5.10.0-rc2/net/ipv6/addrconf.c net-next-patch-5.10.0-rc2/net/ipv6/addrconf.c
> > --- net-next-5.10.0-rc2/net/ipv6/addrconf.c 2020-11-10 08:46:01.075193379 +0100
> > +++ net-next-patch-5.10.0-rc2/net/ipv6/addrconf.c 2020-11-19 21:26:39.770872898 +0100
> > @@ -142,7 +142,6 @@ static int ipv6_count_addresses(const st
> > static int ipv6_generate_stable_address(struct in6_addr *addr,
> > u8 dad_count,
> > const struct inet6_dev *idev);
> > -
>
> Do not remove this line.
[Dmytro]
Understood.
> > #define IN6_ADDR_HSIZE_SHIFT 8
> > #define IN6_ADDR_HSIZE (1 << IN6_ADDR_HSIZE_SHIFT)
> > /*
> > @@ -1315,6 +1314,7 @@ static int ipv6_create_tempaddr(struct i
> > struct ifa6_config cfg;
> > long max_desync_factor;
> > struct in6_addr addr;
> > + struct in6_addr temp;
> > int ret = 0;
> >
> > write_lock_bh(&idev->lock);
> > @@ -1340,9 +1340,16 @@ retry:
> > goto out;
> > }
> > in6_ifa_hold(ifp);
> > - memcpy(addr.s6_addr, ifp->addr.s6_addr, 8);
> > - ipv6_gen_rnd_iid(&addr);
> >
> > + if (ifp->prefix_len == 64) {
> > + memcpy(addr.s6_addr, ifp->addr.s6_addr, 8);
> > + ipv6_gen_rnd_iid(&addr);
> > + } else if (ifp->prefix_len > 0 && ifp->prefix_len <= 128) {
> > + memcpy(addr.s6_addr32, ifp->addr.s6_addr, 16);
> > + get_random_bytes(temp.s6_addr32, 16);
> > + ipv6_addr_prefix_copy(&temp, &addr, ifp->prefix_len);
> > + memcpy(addr.s6_addr, temp.s6_addr, 16);
> > + }
>
> I do not understand why you are copying many times.
> ipv6_addr_copy(), get_random_bytes(), and then ipv6_addr_prefix_copy
> is enough, no?
[Dmytro]
I do not see any definition of ipv6_addr_copy() in v5.10-rc5.
Indeed we can try do "if case" something like this:
if (ifp->prefix_len > 0 && ifp->prefix_len <= 128) {
get_random_bytes(addr.s6_addr, 16);
ipv6_addr_prefix_copy(&addr, &ifp->addr, ifp->prefix_len);
}
> > age = (now - ifp->tstamp) / HZ;
> >
> > regen_advance = idev->cnf.regen_max_retry *
> > @@ -2569,6 +2576,41 @@ static bool is_addr_mode_generate_stable
> > idev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_RANDOM;
> > }
> >
> > +static struct inet6_ifaddr *ipv6_cmp_rcvd_prsnt_prfxs(struct inet6_ifaddr *ifp,
> > + struct inet6_dev *in6_dev,
> > + struct net *net,
> > + const struct prefix_info *pinfo)
> > +{
> > + struct inet6_ifaddr *result_base = NULL;
> > + struct inet6_ifaddr *result = NULL;
> > + struct in6_addr curr_net_prfx;
> > + struct in6_addr net_prfx;
> > + bool prfxs_equal;
> > +
> > + result_base = result;
> > + rcu_read_lock();
> > + list_for_each_entry_rcu(ifp, &in6_dev->addr_list, if_list) {
> > + if (!net_eq(dev_net(ifp->idev->dev), net))
> > + continue;
> > + ipv6_addr_prefix_copy(&net_prfx, &pinfo->prefix, pinfo->prefix_len);
> > + ipv6_addr_prefix_copy(&curr_net_prfx, &ifp->addr, pinfo->prefix_len);
> > + prfxs_equal =
> > + ipv6_prefix_equal(&net_prfx, &curr_net_prfx, pinfo->prefix_len);
> > + if (prfxs_equal && pinfo->prefix_len == ifp->prefix_len) {
> > + result = ifp;
> > + in6_ifa_hold(ifp);
> > + break;
> > + }
>
> I guess we can compare them with ipv6_prefix_equal()
> directly because the code assumes "pinfo->prefix_len" and ifp->prefix_len are
> equal.
[Dmytro]
Understood.
> > + }
> > + rcu_read_unlock();
> > + if (result_base != result)
> > + ifp = result;
> > + else
> > + ifp = NULL;
> > +
> > + return ifp;
> > +}
> > +
> > int addrconf_prefix_rcv_add_addr(struct net *net, struct net_device *dev,
> > const struct prefix_info *pinfo,
> > struct inet6_dev *in6_dev,
> > @@ -2576,9 +2618,16 @@ int addrconf_prefix_rcv_add_addr(struct
> > u32 addr_flags, bool sllao, bool tokenized,
> > __u32 valid_lft, u32 prefered_lft)
> > {
> > - struct inet6_ifaddr *ifp = ipv6_get_ifaddr(net, addr, dev, 1);
> > + struct inet6_ifaddr *ifp = NULL;
> > + int plen = pinfo->prefix_len;
> > int create = 0;
> >
> > + if (plen > 0 && plen <= 128 && plen != 64 &&
> > + in6_dev->cnf.addr_gen_mode != IN6_ADDR_GEN_MODE_STABLE_PRIVACY)
> > + ifp = ipv6_cmp_rcvd_prsnt_prfxs(ifp, in6_dev, net, pinfo);
> > + else
> > + ifp = ipv6_get_ifaddr(net, addr, dev, 1);
> > +
> > if (!ifp && valid_lft) {
> > int max_addresses = in6_dev->cnf.max_addresses;
> > struct ifa6_config cfg = {
>
> I am wondering if we should enable this feature by default at this moment
> because the spec is personal internet draft and some test suites might
> consider this feature violates standards.
[Dmytro]
1. By default the /64 plen is send by the router in RA.
plen ==/64 is default behaviour for me.
We are NOT replacing plen == /64 with this patch.
Variable SLAAC is more like an additional functionality.
If and only IF router sends plen !=64 the patch functionality MAY be activated otherwise it is "dormant".
2. After a discussion with my colleague we come up with the next ideas:
- the implementation of IIDs whose length is arbitrary. The RFC7217, as
implemented here optionally, allows for IIDs of any length. The IETF
consensus status of that RFC is on the Standards Track,
and the status is "PROPOSED STANDARD" (the next consensus level that
this RFC could head for is DRAFT STANDARD; preceding levels through
which the document already went successfully: are Individual Draft
submission, WG adoption, Last Call, AUTH48, published).
- the linux kernel supports IPv6 NAT in the mainline - 'masquerading'.
The feature IPv6 NAT is not supported at all by the IETF: the
consensus level is something like complete rejection. Yet it is fully
supported in the kernel.
- the openbsd (not freebsd) implementations already support RFC 7217
IIDs of arbitrary length: send an RA with plen 65 and the receiving
Host will form an IID of length 63 and an IPv6 address. Why linux would
not allow this?
3. Possible solutions:
3.a) enable this feature as additional functionality, only when plen !=64, and keep it "dormant" by default.
3.b) Add sysctl net.ipv6.conf.enp0s3.variable_slaac = 1
3.c) A possibility is that this feature will be present in the make menuconfig.
> > @@ -2657,6 +2706,91 @@ int addrconf_prefix_rcv_add_addr(struct
> > }
> > EXPORT_SYMBOL_GPL(addrconf_prefix_rcv_add_addr);
> >
> > +static bool ipv6_reserved_interfaceid(struct in6_addr address)
> > +{
> > + if ((address.s6_addr32[2] | address.s6_addr32[3]) == 0)
> > + return true;
> > +
> > + if (address.s6_addr32[2] == htonl(0x02005eff) &&
> > + ((address.s6_addr32[3] & htonl(0xfe000000)) == htonl(0xfe000000)))
> > + return true;
> > +
> > + if (address.s6_addr32[2] == htonl(0xfdffffff) &&
> > + ((address.s6_addr32[3] & htonl(0xffffff80)) == htonl(0xffffff80)))
> > + return true;
> > +
> > + return false;
> > +}
> > +
> > +static int ipv6_gen_addr_var_plen(struct in6_addr *address,
> > + u8 dad_count,
> > + const struct inet6_dev *idev,
> > + unsigned int rcvd_prfx_len,
> > + bool stable_privacy_mode)
> > +{
> > + static union {
> > + char __data[SHA1_BLOCK_SIZE];
> > + struct {
> > + struct in6_addr secret;
> > + __be32 prefix[2];
> > + unsigned char hwaddr[MAX_ADDR_LEN];
> > + u8 dad_count;
> > + } __packed;
> > + } data;
> > + static __u32 workspace[SHA1_WORKSPACE_WORDS];
> > + static __u32 digest[SHA1_DIGEST_WORDS];
> > + struct net *net = dev_net(idev->dev);
> > + static DEFINE_SPINLOCK(lock);
> > + struct in6_addr secret;
> > + struct in6_addr temp;
> > +
> > + BUILD_BUG_ON(sizeof(data.__data) != sizeof(data));
> > +
> > + if (stable_privacy_mode) {
> > + if (idev->cnf.stable_secret.initialized)
> > + secret = idev->cnf.stable_secret.secret;
> > + else if (net->ipv6.devconf_dflt->stable_secret.initialized)
> > + secret = net->ipv6.devconf_dflt->stable_secret.secret;
> > + else
> > + return -1;
> > + }
> > +
> > +retry:
> > + spin_lock_bh(&lock);
> > + if (stable_privacy_mode) {
> > + sha1_init(digest);
> > + memset(&data, 0, sizeof(data));
> > + memset(workspace, 0, sizeof(workspace));
> > + memcpy(data.hwaddr, idev->dev->perm_addr, idev->dev->addr_len);
> > + data.prefix[0] = address->s6_addr32[0];
> > + data.prefix[1] = address->s6_addr32[1];
> > + data.secret = secret;
> > + data.dad_count = dad_count;
> > +
> > + sha1_transform(digest, data.__data, workspace);
> > +
> > + temp = *address;
> > + temp.s6_addr32[0] = (__force __be32)digest[0];
> > + temp.s6_addr32[1] = (__force __be32)digest[1];
> > + temp.s6_addr32[2] = (__force __be32)digest[2];
> > + temp.s6_addr32[3] = (__force __be32)digest[3];
>
> I do not understand why you copy *address and then overwrite
> by digest?
[Dmytro]
Originally it comes from "ipv6_generate_stable_address()".
it is present there because only 64bits of temp are replaced by digest.
In this case, we replace 128 bits thus I agree: it is misleading. I will fix that.
> > + } else {
> > + temp = *address;
> > + get_random_bytes(temp.s6_addr32, 16);
> > + }
> > + spin_unlock_bh(&lock);
> > +
> > + if (ipv6_reserved_interfaceid(temp)) {
> > + dad_count++;
> > + if (dad_count > dev_net(idev->dev)->ipv6.sysctl.idgen_retries)
> > + return -1;
> > + goto retry;
> > + }
> > + ipv6_addr_prefix_copy(&temp, address, rcvd_prfx_len);
> > + *address = temp;
> > + return 0;
> > +}
> > +
> > void addrconf_prefix_rcv(struct net_device *dev, u8 *opt, int len, bool sllao)
> > {
> > struct prefix_info *pinfo;
> > @@ -2781,9 +2915,33 @@ void addrconf_prefix_rcv(struct net_devi
> > dev_addr_generated = true;
> > }
> > goto ok;
> > + } else if (pinfo->prefix_len != 64 &&
> > + pinfo->prefix_len > 0 && pinfo->prefix_len <= 128) {
> > + /* SLAAC with prefixes of arbitrary length (Variable SLAAC).
> > + * draft-mishra-6man-variable-slaac
> > + * draft-mishra-v6ops-variable-slaac-problem-stmt
> > + */
> > + memcpy(&addr, &pinfo->prefix, 16);
> > + if (in6_dev->cnf.addr_gen_mode == IN6_ADDR_GEN_MODE_STABLE_PRIVACY) {
> > + if (!ipv6_gen_addr_var_plen(&addr,
> > + 0,
> > + in6_dev,
> > + pinfo->prefix_len,
> > + true)) {
> > + addr_flags |= IFA_F_STABLE_PRIVACY;
> > + goto ok;
> > + }
> > + } else if (!ipv6_gen_addr_var_plen(&addr,
> > + 0,
> > + in6_dev,
> > + pinfo->prefix_len,
> > + false)) {
> > + goto ok;
> > + }
> > + } else {
> > + net_dbg_ratelimited("IPv6: Prefix with unexpected length %d\n",
> > + pinfo->prefix_len);
> > }
> > - net_dbg_ratelimited("IPv6 addrconf: prefix with wrong length %d\n",
> > - pinfo->prefix_len);
> > goto put;
> >
> > ok:
> > @@ -3186,22 +3344,6 @@ void addrconf_add_linklocal(struct inet6
> > }
> > EXPORT_SYMBOL_GPL(addrconf_add_linklocal);
> >
> > -static bool ipv6_reserved_interfaceid(struct in6_addr address)
> > -{
> > - if ((address.s6_addr32[2] | address.s6_addr32[3]) == 0)
> > - return true;
> > -
> > - if (address.s6_addr32[2] == htonl(0x02005eff) &&
> > - ((address.s6_addr32[3] & htonl(0xfe000000)) == htonl(0xfe000000)))
> > - return true;
> > -
> > - if (address.s6_addr32[2] == htonl(0xfdffffff) &&
> > - ((address.s6_addr32[3] & htonl(0xffffff80)) == htonl(0xffffff80)))
> > - return true;
> > -
> > - return false;
> > -}
> > -
> > static int ipv6_generate_stable_address(struct in6_addr *address,
> > u8 dad_count,
> > const struct inet6_dev *idev)
>
Powered by blists - more mailing lists