| 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: <CADvbK_c6gbV-F9Lv6aiT6JbGGJD96ExWxTj_SWerJsvwvzOoXQ@mail.gmail.com>
Date: Sat, 3 Oct 2020 17:41:58 +0800
From: Xin Long <lucien.xin@...il.com>
To: Nicolas Dichtel <nicolas.dichtel@...nd.com>
Cc: Steffen Klassert <steffen.klassert@...unet.com>,
David Miller <davem@...emloft.net>,
Herbert Xu <herbert@...dor.apana.org.au>,
network dev <netdev@...r.kernel.org>
Subject: Re: [PATCH 10/19] xfrm: interface: support IP6IP6 and IP6IP tunnels
processing with .cb_handler
On Fri, Oct 2, 2020 at 10:44 PM Nicolas Dichtel
<nicolas.dichtel@...nd.com> wrote:
>
> Le 30/07/2020 à 07:41, Steffen Klassert a écrit :
> > From: Xin Long <lucien.xin@...il.com>
> >
> > Similar to ip6_vti, IP6IP6 and IP6IP tunnels processing can easily
> > be done with .cb_handler for xfrm interface.
> >
> > v1->v2:
> > - no change.
> > v2-v3:
> > - enable it only when CONFIG_INET6_XFRM_TUNNEL is defined, to fix
> > the build error, reported by kbuild test robot.
> >
> > Signed-off-by: Xin Long <lucien.xin@...il.com>
> > Signed-off-by: Steffen Klassert <steffen.klassert@...unet.com>
> > ---
>
> This patch broke standard IP tunnels. With this setup:
> + ip link set ntfp2 up
> + ip addr add 10.125.0.2/24 dev ntfp2
> + ip tunnel add tun1 mode ipip ttl 64 local 10.125.0.2 remote 10.125.0.1 dev ntfp2
> + ip addr add 192.168.0.2/32 peer 192.168.0.1/32 dev tun1
> + ip link set dev tun1 up
>
> incoming packets are dropped:
> $ cat /proc/net/xfrm_stat
> ...
> XfrmInNoStates 31
> ...
>
> Xin, can you have a look?
Sure.
When xfrmi processes the ipip packets, it does the state lookup and xfrmi
device lookup both in xfrm_input(). When either of them fails, instead of
returning err and continuing the next .handler in tunnel4_rcv(), it would
drop the packet and return 0.
It's kinda the same as xfrm_tunnel_rcv() and xfrm6_tunnel_rcv().
So the safe fix is to lower the priority of xfrmi .handler but it should
still be higher than xfrm_tunnel_rcv() and xfrm6_tunnel_rcv(). Having
xfrmi loaded will only break IPCOMP, and it's expected. I'll post a fix:
diff --git a/net/ipv4/xfrm4_tunnel.c b/net/ipv4/xfrm4_tunnel.c
index dc19aff7c2e0..fb0648e7fb32 100644
--- a/net/ipv4/xfrm4_tunnel.c
+++ b/net/ipv4/xfrm4_tunnel.c
@@ -64,14 +64,14 @@ static int xfrm_tunnel_err(struct sk_buff *skb, u32 info)
static struct xfrm_tunnel xfrm_tunnel_handler __read_mostly = {
.handler = xfrm_tunnel_rcv,
.err_handler = xfrm_tunnel_err,
- .priority = 3,
+ .priority = 4,
};
#if IS_ENABLED(CONFIG_IPV6)
static struct xfrm_tunnel xfrm64_tunnel_handler __read_mostly = {
.handler = xfrm_tunnel_rcv,
.err_handler = xfrm_tunnel_err,
- .priority = 2,
+ .priority = 3,
};
#endif
diff --git a/net/ipv6/xfrm6_tunnel.c b/net/ipv6/xfrm6_tunnel.c
index 25b7ebda2fab..f696d46e6910 100644
--- a/net/ipv6/xfrm6_tunnel.c
+++ b/net/ipv6/xfrm6_tunnel.c
@@ -303,13 +303,13 @@ static const struct xfrm_type xfrm6_tunnel_type = {
static struct xfrm6_tunnel xfrm6_tunnel_handler __read_mostly = {
.handler = xfrm6_tunnel_rcv,
.err_handler = xfrm6_tunnel_err,
- .priority = 2,
+ .priority = 3,
};
static struct xfrm6_tunnel xfrm46_tunnel_handler __read_mostly = {
.handler = xfrm6_tunnel_rcv,
.err_handler = xfrm6_tunnel_err,
- .priority = 2,
+ .priority = 3,
};
static int __net_init xfrm6_tunnel_net_init(struct net *net)
diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
index a8f66112c52b..0bb7963b9f6b 100644
--- a/net/xfrm/xfrm_interface.c
+++ b/net/xfrm/xfrm_interface.c
@@ -830,14 +830,14 @@ static struct xfrm6_tunnel xfrmi_ipv6_handler
__read_mostly = {
.handler = xfrmi6_rcv_tunnel,
.cb_handler = xfrmi_rcv_cb,
.err_handler = xfrmi6_err,
- .priority = -1,
+ .priority = 2,
};
static struct xfrm6_tunnel xfrmi_ip6ip_handler __read_mostly = {
.handler = xfrmi6_rcv_tunnel,
.cb_handler = xfrmi_rcv_cb,
.err_handler = xfrmi6_err,
- .priority = -1,
+ .priority = 2,
};
#endif
@@ -875,14 +875,14 @@ static struct xfrm_tunnel xfrmi_ipip_handler
__read_mostly = {
.handler = xfrmi4_rcv_tunnel,
.cb_handler = xfrmi_rcv_cb,
.err_handler = xfrmi4_err,
- .priority = -1,
+ .priority = 3,
};
static struct xfrm_tunnel xfrmi_ipip6_handler __read_mostly = {
.handler = xfrmi4_rcv_tunnel,
.cb_handler = xfrmi_rcv_cb,
.err_handler = xfrmi4_err,
- .priority = -1,
+ .priority = 2,
};
#endif
> > net/xfrm/xfrm_interface.c | 38 ++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 38 insertions(+)
> >
> > diff --git a/net/xfrm/xfrm_interface.c b/net/xfrm/xfrm_interface.c
> > index c407ecbc5d46..b9ef496d3d7c 100644
> > --- a/net/xfrm/xfrm_interface.c
> > +++ b/net/xfrm/xfrm_interface.c
> > @@ -798,6 +798,26 @@ static struct xfrm6_protocol xfrmi_ipcomp6_protocol __read_mostly = {
> > .priority = 10,
> > };
> >
> > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL)
> > +static int xfrmi6_rcv_tunnel(struct sk_buff *skb)
> > +{
> > + const xfrm_address_t *saddr;
> > + __be32 spi;
> > +
> > + saddr = (const xfrm_address_t *)&ipv6_hdr(skb)->saddr;
> > + spi = xfrm6_tunnel_spi_lookup(dev_net(skb->dev), saddr);
> > +
> > + return xfrm6_rcv_spi(skb, IPPROTO_IPV6, spi, NULL);
> > +}
> > +
> > +static struct xfrm6_tunnel xfrmi_ipv6_handler __read_mostly = {
> > + .handler = xfrmi6_rcv_tunnel,
> > + .cb_handler = xfrmi_rcv_cb,
> > + .err_handler = xfrmi6_err,
> > + .priority = -1,
> > +};
> > +#endif
> > +
> > static struct xfrm4_protocol xfrmi_esp4_protocol __read_mostly = {
> > .handler = xfrm4_rcv,
> > .input_handler = xfrm_input,
> > @@ -866,9 +886,23 @@ static int __init xfrmi6_init(void)
> > err = xfrm6_protocol_register(&xfrmi_ipcomp6_protocol, IPPROTO_COMP);
> > if (err < 0)
> > goto xfrm_proto_comp_failed;
> > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL)
> > + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET6);
> > + if (err < 0)
> > + goto xfrm_tunnel_ipv6_failed;
> > + err = xfrm6_tunnel_register(&xfrmi_ipv6_handler, AF_INET);
> > + if (err < 0)
> > + goto xfrm_tunnel_ip6ip_failed;
> > +#endif
> >
> > return 0;
> >
> > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL)
> > +xfrm_tunnel_ip6ip_failed:
> > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6);
> > +xfrm_tunnel_ipv6_failed:
> > + xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP);
> > +#endif
> > xfrm_proto_comp_failed:
> > xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH);
> > xfrm_proto_ah_failed:
> > @@ -879,6 +913,10 @@ static int __init xfrmi6_init(void)
> >
> > static void xfrmi6_fini(void)
> > {
> > +#if IS_ENABLED(CONFIG_INET6_XFRM_TUNNEL)
> > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET);
> > + xfrm6_tunnel_deregister(&xfrmi_ipv6_handler, AF_INET6);
> > +#endif
> > xfrm6_protocol_deregister(&xfrmi_ipcomp6_protocol, IPPROTO_COMP);
> > xfrm6_protocol_deregister(&xfrmi_ah6_protocol, IPPROTO_AH);
> > xfrm6_protocol_deregister(&xfrmi_esp6_protocol, IPPROTO_ESP);
> >
Powered by blists - more mailing lists