[PATCH v3 net-next 0/2] Use correct sk->sk_prot for IPV6

[PATCH v3 net-next 0/2] 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. 

Changed since v2: 
- Dropped patch to fix IPV6_ADDRFORM setsockopt
There was some disagreement about the correct way of fixinig it,
and this series does not depend on it.

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 (2):
  net: Export tcpv6_prot
  tls: Use correct sk->sk_prot for IPV6

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

-- 
1.8.3.1

[PATCH v3 net-next 1/2] 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 38f76d8..60d0629 100644
--- a/net/ipv6/tcp_ipv6.c
+++ b/net/ipv6/tcp_ipv6.c
@@ -1948,6 +1948,7 @@ struct proto tcpv6_prot = {
 #endif
 	.diag_destroy		= tcp_abort,
 };
+EXPORT_SYMBOL_GPL(tcpv6_prot);
 
 /* thinking of making this const? Don't.
  * early_demux can change based on sysctl.
-- 
1.8.3.1

[PATCH v3 net-next 2/2] 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.

Fixes: 3c4d7559159b ('tls: kernel TLS support')
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..33c499e 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

Re: [v3,net-next,2/2] tls: Use correct sk->sk_prot for IPV6

[Guenter Roeck]

Hi Ilya,

On Mon, Sep 04, 2017 at 01:14:01PM +0300, Ilya Lesokhin wrote:
> 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.
> 

Do you plan to update this patch with the missing TCPv6 support ?
As far as I can see, the part that was accepted upstream does not fix
the TCPv6 protocol issue which triggers CVE-2018-5703.

If adding IPv6 support is not possible/acceptable, would it make sense
to limit TLS support to TCPv4, ie add something like

	if (sk->sk_prot != &tcp_prot)
		return -EINVAL;

to tls_init() ?

Thanks,
Guenter

> Fixes: 3c4d7559159b ('tls: kernel TLS support')
> 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..33c499e 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);
>  

Re: [v3,net-next,2/2] tls: Use correct sk->sk_prot for IPV6

[Boris Pismenny]

Hi Guenter,

On 2/23/2018 11:52 PM, Guenter Roeck wrote:
> Hi Ilya,
> 
> On Mon, Sep 04, 2017 at 01:14:01PM +0300, Ilya Lesokhin wrote:
>> 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.
>>
> 
> Do you plan to update this patch with the missing TCPv6 support ?

We'll re-spin a v4 by EOW.

> As far as I can see, the part that was accepted upstream does not fix
> the TCPv6 protocol issue which triggers CVE-2018-5703.
> 
> If adding IPv6 support is not possible/acceptable, would it make sense
> to limit TLS support to TCPv4, ie add something like
> 
> 	if (sk->sk_prot != &tcp_prot)
> 		return -EINVAL;
> 
> to tls_init() ?

AFAIK there are users of TLS over IPv6 who wouldn't find this acceptable.

Best,
Boris.

Re: [PATCH v3 net-next 0/2] Use correct sk->sk_prot for IPV6

[David Miller]

From: Ilya Lesokhin <ilyal@mellanox.com>
Date: Mon,  4 Sep 2017 13:13:59 +0300

> 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. 
> 
> Changed since v2: 
> - Dropped patch to fix IPV6_ADDRFORM setsockopt
> There was some disagreement about the correct way of fixinig it,
> and this series does not depend on it.
> 
> 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.                          

This is not acceptable.

Forcing such a huge piece of infrastructure like ipv6 to be statically
built just because someone wants to enable TLS is not going to pass.

Every distrubtion out there will enable all features, including TLS,
so effectively you are forcing every distribution to build ipv6
non-modules.

Sorry, you will have to fix this in a way that allows TLS and IPV6
to be modular.