[PATCH v2 net-next 0/3] Use correct sk->sk_prot for IPV6

[PATCH v2 net-next 0/3] Use correct sk->sk_prot for IPV6

[Ilya Lesokhin]

The tls ulp overrides sk->prot with a new tls specific proto structs.
The tls specific structs were previously based on the ipv4 specific
tcp_prot sturct.
As a result, attaching the tls ulp to an ipv6 tcp socket replaced
some ipv6 callback with the ipv4 equivalents.

This patch adds ipv6 tls proto structs and uses them when
attached to ipv6 sockets.

Changes since v1:
- TLS now dependes on IPV6
This fixes complication issues when TLS is built-in and IPV6 is a module.
The downside should be small as it is unlikely that there are kernel TLS 
users who can't afford to include IPV6 in thier kernel.
- tls_init now checks sk->sk_prot directly
This is somewhat safer then checking indirectly through sk->sk_family

Ilya Lesokhin (3):
  ipv6: Prevent unexpected sk->sk_prot changes
  net: Export tcpv6_prot
  tls: Use correct sk->sk_prot for IPV6

 net/ipv6/ipv6_sockglue.c | 12 ++++++++++++
 net/ipv6/tcp_ipv6.c      |  1 +
 net/tls/Kconfig          |  1 +
 net/tls/tls_main.c       | 50 ++++++++++++++++++++++++++++++++++++------------
 4 files changed, 52 insertions(+), 12 deletions(-)

-- 
1.8.3.1

[PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot changes

[Ilya Lesokhin]

With this patch IPV6 code ensure that only sockets with the
expected sk->sk_prot are converted to IPV4.

Signed-off-by: Boris Pismenny <borisp@mellanox.com>
---
 net/ipv6/ipv6_sockglue.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 02d795f..318cd344 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -174,6 +174,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 		if (val == PF_INET) {
 			struct ipv6_txoptions *opt;
 			struct sk_buff *pktopt;
+			struct proto *expected_prot;
 
 			if (sk->sk_type == SOCK_RAW)
 				break;
@@ -199,6 +200,17 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 				break;
 			}
 
+			if (sk->sk_protocol == IPPROTO_TCP &&
+			    sk->sk_prot != &tcpv6_prot)
+				break;
+
+			expected_prot = &udpv6_prot;
+			if (sk->sk_protocol == IPPROTO_UDPLITE)
+				expected_prot = &udplitev6_prot;
+
+			if (sk->sk_prot != expected_prot)
+				break;
+
 			fl6_free_socklist(sk);
 			__ipv6_sock_mc_close(sk);
 
-- 
1.8.3.1

Re: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot changes

[Eric Dumazet]

On Tue, 2017-08-15 at 14:08 +0300, Ilya Lesokhin wrote:
> With this patch IPV6 code ensure that only sockets with the
> expected sk->sk_prot are converted to IPV4.

It looks like you fix a bug added by a recent commit (in net-next, not
in net tree) ?

Please provide Fixes: tag to ease code review and maintenance.

Thanks.

Re: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot changes

[Eric Dumazet]

On Tue, 2017-08-15 at 14:08 +0300, Ilya Lesokhin wrote:
> With this patch IPV6 code ensure that only sockets with the
> expected sk->sk_prot are converted to IPV4.
> 
> Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> ---
>  net/ipv6/ipv6_sockglue.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> index 02d795f..318cd344 100644
> --- a/net/ipv6/ipv6_sockglue.c
> +++ b/net/ipv6/ipv6_sockglue.c
> @@ -174,6 +174,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>  		if (val == PF_INET) {
>  			struct ipv6_txoptions *opt;
>  			struct sk_buff *pktopt;
> +			struct proto *expected_prot;
>  
>  			if (sk->sk_type == SOCK_RAW)
>  				break;
> @@ -199,6 +200,17 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>  				break;
>  			}
>  
> +			if (sk->sk_protocol == IPPROTO_TCP &&
> +			    sk->sk_prot != &tcpv6_prot)
> +				break;
> +
> +			expected_prot = &udpv6_prot;
> +			if (sk->sk_protocol == IPPROTO_UDPLITE)
> +				expected_prot = &udplitev6_prot;
> +
> +			if (sk->sk_prot != expected_prot)
> +				break;
> +
>  			fl6_free_socklist(sk);
>  			__ipv6_sock_mc_close(sk);
>  

I am afraid I do not understand this patch at all.

Direct references to tcpv6_prot, udpv6_prot, and udplitev6_prot in
net/ipv6/ipv6_sockglue.c looks completely broken.

Please provide something cleaner, maybe by adding a new method
(implementation would then be provided in TCP / UDP code )

RE: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot changes

[Boris Pismenny]

Hi Eric,

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Tuesday, August 15, 2017 14:59
> To: Ilya Lesokhin <ilyal@mellanox.com>
> Cc: netdev@vger.kernel.org; davem@davemloft.net; davejwatson@fb.com;
> Aviad Yehezkel <aviadye@mellanox.com>; Boris Pismenny
> <borisp@mellanox.com>
> Subject: Re: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot
> changes
> 
> On Tue, 2017-08-15 at 14:08 +0300, Ilya Lesokhin wrote:
> > With this patch IPV6 code ensure that only sockets with the
> > expected sk->sk_prot are converted to IPV4.
> >
> > Signed-off-by: Boris Pismenny <borisp@mellanox.com>
> > ---
> >  net/ipv6/ipv6_sockglue.c | 12 ++++++++++++
> >  1 file changed, 12 insertions(+)
> >
> > diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
> > index 02d795f..318cd344 100644
> > --- a/net/ipv6/ipv6_sockglue.c
> > +++ b/net/ipv6/ipv6_sockglue.c
> > @@ -174,6 +174,7 @@ static int do_ipv6_setsockopt(struct sock *sk, int
> level, int optname,
> >  		if (val == PF_INET) {
> >  			struct ipv6_txoptions *opt;
> >  			struct sk_buff *pktopt;
> > +			struct proto *expected_prot;
> >
> >  			if (sk->sk_type == SOCK_RAW)
> >  				break;
> > @@ -199,6 +200,17 @@ static int do_ipv6_setsockopt(struct sock *sk, int
> level, int optname,
> >  				break;
> >  			}
> >
> > +			if (sk->sk_protocol == IPPROTO_TCP &&
> > +			    sk->sk_prot != &tcpv6_prot)
> > +				break;
> > +
> > +			expected_prot = &udpv6_prot;
> > +			if (sk->sk_protocol == IPPROTO_UDPLITE)
> > +				expected_prot = &udplitev6_prot;
> > +
> > +			if (sk->sk_prot != expected_prot)
> > +				break;
> > +
> >  			fl6_free_socklist(sk);
> >  			__ipv6_sock_mc_close(sk);
> >
> 
> I am afraid I do not understand this patch at all.
> 
> Direct references to tcpv6_prot, udpv6_prot, and udplitev6_prot in
> net/ipv6/ipv6_sockglue.c looks completely broken.
> 
> Please provide something cleaner, maybe by adding a new method
> (implementation would then be provided in TCP / UDP code )
> 
> 

The IPV6_ADDRFORM socket option assumes that when 
(sk->sk_protocol == IPPROTO_TCP)
then the sk_proto is set to tcpv6_prot and it replaces it with tcp_prot.

This patch ensures that the IPV6_ADDRFORM socket option doesn't replace
the socket's sk_prot to tcp when it is not expected. For example, TLS sockets
also replace sk_prot, and we need to prevent IPV6_ADDRFORM from
overriding these.

Are you suggesting that each socket protocol will provide a method that
converts it from IPv6 to IPv4?

Re: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot changes

[Eric Dumazet]

On Tue, 2017-08-15 at 13:08 +0000, Boris Pismenny wrote:
> Hi Eric,


> 
> The IPV6_ADDRFORM socket option assumes that when 
> (sk->sk_protocol == IPPROTO_TCP)
> then the sk_proto is set to tcpv6_prot and it replaces it with tcp_prot.
> 
> This patch ensures that the IPV6_ADDRFORM socket option doesn't replace
> the socket's sk_prot to tcp when it is not expected. For example, TLS sockets
> also replace sk_prot, and we need to prevent IPV6_ADDRFORM from
> overriding these.
> 
> Are you suggesting that each socket protocol will provide a method that
> converts it from IPv6 to IPv4?

I am just saying IPV6_ADDRFORM is becoming spaghetti code, and maybe
this is time to make it modular.

The UDP or TCP magic really should be implemented in TCP or UDP files.

(Who knows maybe one day SCTP or DCCP will support IPVADDRFORM)

RE: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot changes

[Ilya Lesokhin]

> -----Original Message-----
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> Sent: Tuesday, August 15, 2017 5:46 PM
> To: Boris Pismenny <borisp@mellanox.com>
> Cc: Ilya Lesokhin <ilyal@mellanox.com>; netdev@vger.kernel.org;
> davem@davemloft.net; davejwatson@fb.com; Aviad Yehezkel
> <aviadye@mellanox.com>
> Subject: Re: [PATCH v2 net-next 1/3] ipv6: Prevent unexpected sk->sk_prot
> changes
> 
> I am just saying IPV6_ADDRFORM is becoming spaghetti code, and maybe
> this is time to make it modular. 
> 

Hi Eric,
I understand your concern, but I would like to postpone implementing this
feature until the ulp infrastructure stabilizes.
I don't even know how many users want to use IPV6_ADDRFORM on sockets with ulp.
I think that simply returning an error for ulp sockets in the meantime is a reasonable 
compromise.

what do you think about the patch below?

Subject: [PATCH 1/1] ipv6: Disable IPV6_ADDRFORM on sockets with ulp attached

The IPV6_ADDRFORM setsockopt works by overriding sk->sk_prot.
The current KTLS ulp also overrides sk->sk_prot.
Consequently, using IPV6_ADDRFORM when there is a ulp attached is
unsafe and this patch disables it.

Fixes: 3c4d7559159b ('tls: kernel TLS support')
Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com>
---
 net/ipv6/ipv6_sockglue.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
index 02d795f..d935948 100644
--- a/net/ipv6/ipv6_sockglue.c
+++ b/net/ipv6/ipv6_sockglue.c
@@ -185,8 +185,12 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
 					retv = -EBUSY;
 					break;
 				}
-			} else if (sk->sk_protocol != IPPROTO_TCP)
+			} else if (sk->sk_protocol == IPPROTO_TCP) {
+				if (inet_csk(sk)->icsk_ulp_ops)
+					break;
+			} else {
 				break;
+			}
 
 			if (sk->sk_state != TCP_ESTABLISHED) {
 				retv = -ENOTCONN;


Thanks,
Ilya

[PATCH v2 net-next 2/3] net: Export tcpv6_prot

[Ilya Lesokhin]

Want to be able to use these in TLS.

Signed-off-by: Boris Pismenny <borisp@mellanox.com>
---
 net/ipv6/tcp_ipv6.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/ipv6/tcp_ipv6.c b/net/ipv6/tcp_ipv6.c
index 2521690..ef8d5b4 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1944,6 +1944,7 @@ struct proto tcpv6_prot = {
 #endif
 	.diag_destroy		= tcp_abort,
 };
+EXPORT_SYMBOL_GPL(tcpv6_prot);
 
 static struct inet6_protocol tcpv6_protocol = {
 	.early_demux	=	tcp_v6_early_demux,
-- 
1.8.3.1

[PATCH v2 net-next 3/3] tls: Use correct sk->sk_prot for IPV6

[Ilya Lesokhin]

The tls ulp overrides sk->prot with a new tls specific proto structs.
The tls specific structs were previously based on the ipv4 specific
tcp_prot sturct.
As a result, attaching the tls ulp to an ipv6 tcp socket replaced
some ipv6 callback with the ipv4 equivalents.

This patch adds ipv6 tls proto structs and uses them when
attached to ipv6 sockets.

Signed-off-by: Boris Pismenny <borisp@mellanox.com>
Signed-off-by: Ilya Lesokhin <ilyal@mellanox.com>
---
 net/tls/Kconfig    |  1 +
 net/tls/tls_main.c | 50 ++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 39 insertions(+), 12 deletions(-)

diff --git a/net/tls/Kconfig b/net/tls/Kconfig
index eb58303..7e9cf8b 100644
--- a/net/tls/Kconfig
+++ b/net/tls/Kconfig
@@ -7,6 +7,7 @@ config TLS
 	select CRYPTO
 	select CRYPTO_AES
 	select CRYPTO_GCM
+	select IPV6
 	default n
 	---help---
 	Enable kernel support for TLS protocol. This allows symmetric
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 60aff60..9caad11 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -40,13 +40,25 @@
 #include <linux/sched/signal.h>
 
 #include <net/tls.h>
+#include <net/transp_v6.h>
 
 MODULE_AUTHOR("Mellanox Technologies");
 MODULE_DESCRIPTION("Transport Layer Security Support");
 MODULE_LICENSE("Dual BSD/GPL");
 
-static struct proto tls_base_prot;
-static struct proto tls_sw_prot;
+enum {
+	TLSV4,
+	TLSV6,
+	TLS_NUM_PROTS,
+};
+
+enum {
+	TLS_BASE_TX,
+	TLS_SW_TX,
+	TLS_NUM_CONFIG,
+};
+
+static struct proto tls_prots[TLS_NUM_PROTS][TLS_NUM_CONFIG];
 
 int wait_on_pending_writer(struct sock *sk, long *timeo)
 {
@@ -342,6 +354,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
 	struct tls_context *ctx = tls_get_ctx(sk);
 	struct proto *prot = NULL;
 	int rc = 0;
+	int ip_ver = sk->sk_family == AF_INET6 ? TLSV6 : TLSV4;
 
 	if (!optval || (optlen < sizeof(*crypto_info))) {
 		rc = -EINVAL;
@@ -396,7 +409,7 @@ static int do_tls_setsockopt_tx(struct sock *sk, char __user *optval,
 
 	/* currently SW is default, we will have ethtool in future */
 	rc = tls_set_sw_offload(sk, ctx);
-	prot = &tls_sw_prot;
+	prot = &tls_prots[ip_ver][TLS_SW_TX];
 	if (rc)
 		goto err_crypto_info;
 
@@ -443,6 +456,12 @@ static int tls_init(struct sock *sk)
 	struct inet_connection_sock *icsk = inet_csk(sk);
 	struct tls_context *ctx;
 	int rc = 0;
+	int ip_ver = TLSV4;
+
+	if (sk->sk_prot == &tcpv6_prot)
+		ip_ver = TLSV6;
+	else if (sk->sk_prot != &tcp_prot)
+		return -EINVAL;
 
 	/* allocate tls context */
 	ctx = kzalloc(sizeof(*ctx), GFP_KERNEL);
@@ -453,7 +472,8 @@ static int tls_init(struct sock *sk)
 	icsk->icsk_ulp_data = ctx;
 	ctx->setsockopt = sk->sk_prot->setsockopt;
 	ctx->getsockopt = sk->sk_prot->getsockopt;
-	sk->sk_prot = &tls_base_prot;
+
+	sk->sk_prot = &tls_prots[ip_ver][TLS_BASE_TX];
 out:
 	return rc;
 }
@@ -464,16 +484,22 @@ static int tls_init(struct sock *sk)
 	.init			= tls_init,
 };
 
+static void build_protos(struct proto *prot, struct proto *base)
+{
+	prot[TLS_BASE_TX] = *base;
+	prot[TLS_BASE_TX].setsockopt = tls_setsockopt;
+	prot[TLS_BASE_TX].getsockopt = tls_getsockopt;
+
+	prot[TLS_SW_TX]		= prot[TLS_BASE_TX];
+	prot[TLS_SW_TX].close	= tls_sk_proto_close;
+	prot[TLS_SW_TX].sendmsg		= tls_sw_sendmsg;
+	prot[TLS_SW_TX].sendpage	= tls_sw_sendpage;
+}
+
 static int __init tls_register(void)
 {
-	tls_base_prot			= tcp_prot;
-	tls_base_prot.setsockopt	= tls_setsockopt;
-	tls_base_prot.getsockopt	= tls_getsockopt;
-
-	tls_sw_prot			= tls_base_prot;
-	tls_sw_prot.sendmsg		= tls_sw_sendmsg;
-	tls_sw_prot.sendpage            = tls_sw_sendpage;
-	tls_sw_prot.close               = tls_sk_proto_close;
+	build_protos(tls_prots[TLSV4], &tcp_prot);
+	build_protos(tls_prots[TLSV6], &tcpv6_prot);
 
 	tcp_register_ulp(&tcp_tls_ulp_ops);
 
-- 
1.8.3.1