[PATCH net-next 0/5] tls: implement key updates for TLS1.3

[PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Sabrina Dubroca]

This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
[1]). A sender transmits a KeyUpdate message and then changes its TX
key. The receiver should react by updating its RX key before
processing the next message.

This patchset implements key updates by:
 1. pausing decryption when a KeyUpdate message is received, to avoid
    attempting to use the old key to decrypt a record encrypted with
    the new key
 2. returning -EKEYEXPIRED to syscalls that cannot receive the
    KeyUpdate message, until the rekey has been performed by userspace
 3. passing the KeyUpdate message to userspace as a control message
 4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
    setsockopts

This API has been tested with gnutls to make sure that it allows
userspace libraries to implement key updates [2]. Thanks to Frantisek
Krenzelok <fkrenzel@redhat.com> for providing the implementation in
gnutls and testing the kernel patches.

Note: in a future series, I'll clean up tls_set_sw_offload and
eliminate the per-cipher copy-paste using tls_cipher_size_desc.

[1] https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3
[2] https://gitlab.com/gnutls/gnutls/-/merge_requests/1625

Sabrina Dubroca (5):
  tls: remove tls_context argument from tls_set_sw_offload
  tls: block decryption when a rekey is pending
  tls: implement rekey for TLS1.3
  selftests: tls: add key_generation argument to tls_crypto_info_init
  selftests: tls: add rekey tests

 include/net/tls.h                 |   4 +
 net/tls/tls.h                     |   3 +-
 net/tls/tls_device.c              |   2 +-
 net/tls/tls_main.c                |  32 +++-
 net/tls/tls_sw.c                  | 169 +++++++++++++++----
 tools/testing/selftests/net/tls.c | 272 +++++++++++++++++++++++++++++-
 6 files changed, 434 insertions(+), 48 deletions(-)

-- 
2.38.1

[PATCH net-next 1/5] tls: remove tls_context argument from tls_set_sw_offload

[Sabrina Dubroca]

It's not really needed since we end up refetching it as tls_ctx. We
can also remove the NULL check, since we have already dereferenced ctx
in do_tls_setsockopt_conf.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Tested-by: Frantisek Krenzelok <fkrenzel@redhat.com>
---
 net/tls/tls.h        |  2 +-
 net/tls/tls_device.c |  2 +-
 net/tls/tls_main.c   |  4 ++--
 net/tls/tls_sw.c     | 11 +++--------
 4 files changed, 7 insertions(+), 12 deletions(-)

diff --git a/net/tls/tls.h b/net/tls/tls.h
index 0e840a0c3437..34d0fe814600 100644
--- a/net/tls/tls.h
+++ b/net/tls/tls.h
@@ -90,7 +90,7 @@ int tls_sk_attach(struct sock *sk, int optname, char __user *optval,
 		  unsigned int optlen);
 void tls_err_abort(struct sock *sk, int err);
 
-int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx);
+int tls_set_sw_offload(struct sock *sk, int tx);
 void tls_update_rx_zc_capable(struct tls_context *tls_ctx);
 void tls_sw_strparser_arm(struct sock *sk, struct tls_context *ctx);
 void tls_sw_strparser_done(struct tls_context *tls_ctx);
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index 6c593788dc25..c149f36b42ee 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -1291,7 +1291,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 	context->resync_nh_reset = 1;
 
 	ctx->priv_ctx_rx = context;
-	rc = tls_set_sw_offload(sk, ctx, 0);
+	rc = tls_set_sw_offload(sk, 0);
 	if (rc)
 		goto release_ctx;
 
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index 3735cb00905d..fb1da1780f50 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -772,7 +772,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXDEVICE);
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
 		} else {
-			rc = tls_set_sw_offload(sk, ctx, 1);
+			rc = tls_set_sw_offload(sk, 1);
 			if (rc)
 				goto err_crypto_info;
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXSW);
@@ -786,7 +786,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXDEVICE);
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRRXDEVICE);
 		} else {
-			rc = tls_set_sw_offload(sk, ctx, 0);
+			rc = tls_set_sw_offload(sk, 0);
 			if (rc)
 				goto err_crypto_info;
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXSW);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 9ed978634125..238562f9081b 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2462,10 +2462,10 @@ void tls_update_rx_zc_capable(struct tls_context *tls_ctx)
 		tls_ctx->prot_info.version != TLS_1_3_VERSION;
 }
 
-int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
+int tls_set_sw_offload(struct sock *sk, int tx)
 {
-	struct tls_context *tls_ctx = tls_get_ctx(sk);
-	struct tls_prot_info *prot = &tls_ctx->prot_info;
+	struct tls_context *ctx = tls_get_ctx(sk);
+	struct tls_prot_info *prot = &ctx->prot_info;
 	struct tls_crypto_info *crypto_info;
 	struct tls_sw_context_tx *sw_ctx_tx = NULL;
 	struct tls_sw_context_rx *sw_ctx_rx = NULL;
@@ -2477,11 +2477,6 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
 	size_t keysize;
 	int rc = 0;
 
-	if (!ctx) {
-		rc = -EINVAL;
-		goto out;
-	}
-
 	if (tx) {
 		if (!ctx->priv_ctx_tx) {
 			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);
-- 
2.38.1

[PATCH net-next 2/5] tls: block decryption when a rekey is pending

[Sabrina Dubroca]

When a TLS handshake record carrying a KeyUpdate message is received,
all subsequent records will be encrypted with a new key. We need to
stop decrypting incoming records with the old key, and wait until
userspace provides a new key.

Make a note of this in the RX context just after decrypting that
record, and stop recvmsg/splice calls with EKEYEXPIRED until the new
key is available.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Tested-by: Frantisek Krenzelok <fkrenzel@redhat.com>
---
 include/net/tls.h |  4 ++++
 net/tls/tls_sw.c  | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/include/net/tls.h b/include/net/tls.h
index 154949c7b0c8..297732f23804 100644
--- a/include/net/tls.h
+++ b/include/net/tls.h
@@ -69,8 +69,11 @@ extern const struct tls_cipher_size_desc tls_cipher_size_desc[];
 
 #define TLS_CRYPTO_INFO_READY(info)	((info)->cipher_type)
 
+#define TLS_RECORD_TYPE_HANDSHAKE	0x16
 #define TLS_RECORD_TYPE_DATA		0x17
 
+#define TLS_HANDSHAKE_KEYUPDATE		24	/* rfc8446 B.3: Key update */
+
 #define TLS_AAD_SPACE_SIZE		13
 
 #define MAX_IV_SIZE			16
@@ -145,6 +148,7 @@ struct tls_sw_context_rx {
 
 	struct tls_strparser strp;
 
+	bool key_update_pending;
 	atomic_t decrypt_pending;
 	/* protect crypto_wait with decrypt_pending*/
 	spinlock_t decrypt_compl_lock;
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 238562f9081b..22efea224a04 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -1686,6 +1686,33 @@ tls_decrypt_device(struct sock *sk, struct msghdr *msg,
 	return 1;
 }
 
+static int tls_check_pending_rekey(struct sock *sk, struct sk_buff *skb)
+{
+	const struct tls_msg *tlm = tls_msg(skb);
+	const struct strp_msg *rxm = strp_msg(skb);
+
+	if (tlm->control == TLS_RECORD_TYPE_HANDSHAKE) {
+		char hs_type;
+		int err;
+
+		if (rxm->full_len < 1)
+			return -EINVAL;
+
+		err = skb_copy_bits(skb, rxm->offset, &hs_type, 1);
+		if (err < 0)
+			return err;
+
+		if (hs_type == TLS_HANDSHAKE_KEYUPDATE) {
+			struct tls_context *ctx = tls_get_ctx(sk);
+			struct tls_sw_context_rx *rx_ctx = ctx->priv_ctx_rx;
+
+			rx_ctx->key_update_pending = true;
+		}
+	}
+
+	return 0;
+}
+
 static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 			     struct tls_decrypt_arg *darg)
 {
@@ -1705,6 +1732,10 @@ static int tls_rx_one_record(struct sock *sk, struct msghdr *msg,
 	rxm->full_len -= prot->overhead_size;
 	tls_advance_record_sn(sk, prot, &tls_ctx->rx);
 
+	err = tls_check_pending_rekey(sk, darg->skb);
+	if (err < 0)
+		return err;
+
 	return 0;
 }
 
@@ -1956,6 +1987,12 @@ int tls_sw_recvmsg(struct sock *sk,
 		struct tls_decrypt_arg darg;
 		int to_decrypt, chunk;
 
+		/* a rekey is pending, let userspace deal with it */
+		if (unlikely(ctx->key_update_pending)) {
+			err = -EKEYEXPIRED;
+			break;
+		}
+
 		err = tls_rx_rec_wait(sk, psock, flags & MSG_DONTWAIT,
 				      released);
 		if (err <= 0) {
@@ -2140,6 +2177,12 @@ ssize_t tls_sw_splice_read(struct socket *sock,  loff_t *ppos,
 	if (err < 0)
 		return err;
 
+	/* a rekey is pending, let userspace deal with it */
+	if (unlikely(ctx->key_update_pending)) {
+		err = -EKEYEXPIRED;
+		goto splice_read_end;
+	}
+
 	if (!skb_queue_empty(&ctx->rx_list)) {
 		skb = __skb_dequeue(&ctx->rx_list);
 	} else {
@@ -2521,6 +2564,7 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 		skb_queue_head_init(&sw_ctx_rx->rx_list);
 		skb_queue_head_init(&sw_ctx_rx->async_hold);
 		aead = &sw_ctx_rx->aead_recv;
+		sw_ctx_rx->key_update_pending = false;
 	}
 
 	switch (crypto_info->cipher_type) {
-- 
2.38.1

[PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Sabrina Dubroca]

This adds the possibility to change the key and IV when using
TLS1.3. Changing the cipher or TLS version is not supported.

Once we have updated the RX key, we can unblock the receive side.

This change only affects tls_sw, since 1.3 offload isn't supported.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Tested-by: Frantisek Krenzelok <fkrenzel@redhat.com>
---
 net/tls/tls.h        |   3 +-
 net/tls/tls_device.c |   2 +-
 net/tls/tls_main.c   |  32 ++++++++++--
 net/tls/tls_sw.c     | 120 ++++++++++++++++++++++++++++++++-----------
 4 files changed, 120 insertions(+), 37 deletions(-)

diff --git a/net/tls/tls.h b/net/tls/tls.h
index 34d0fe814600..6f9c85eaa9c5 100644
--- a/net/tls/tls.h
+++ b/net/tls/tls.h
@@ -90,7 +90,8 @@ int tls_sk_attach(struct sock *sk, int optname, char __user *optval,
 		  unsigned int optlen);
 void tls_err_abort(struct sock *sk, int err);
 
-int tls_set_sw_offload(struct sock *sk, int tx);
+int tls_set_sw_offload(struct sock *sk, int tx,
+		       struct tls_crypto_info *new_crypto_info);
 void tls_update_rx_zc_capable(struct tls_context *tls_ctx);
 void tls_sw_strparser_arm(struct sock *sk, struct tls_context *ctx);
 void tls_sw_strparser_done(struct tls_context *tls_ctx);
diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
index c149f36b42ee..1ad50c253dfe 100644
--- a/net/tls/tls_device.c
+++ b/net/tls/tls_device.c
@@ -1291,7 +1291,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
 	context->resync_nh_reset = 1;
 
 	ctx->priv_ctx_rx = context;
-	rc = tls_set_sw_offload(sk, 0);
+	rc = tls_set_sw_offload(sk, 0, NULL);
 	if (rc)
 		goto release_ctx;
 
diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
index fb1da1780f50..9be82aecd13e 100644
--- a/net/tls/tls_main.c
+++ b/net/tls/tls_main.c
@@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
 static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 				  unsigned int optlen, int tx)
 {
+	union tls_crypto_context tmp = {};
+	struct tls_crypto_info *old_crypto_info = NULL;
 	struct tls_crypto_info *crypto_info;
 	struct tls_crypto_info *alt_crypto_info;
 	struct tls_context *ctx = tls_get_ctx(sk);
+	bool update = false;
 	size_t optsize;
 	int rc = 0;
 	int conf;
@@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 		alt_crypto_info = &ctx->crypto_send.info;
 	}
 
-	/* Currently we don't support set crypto info more than one time */
-	if (TLS_CRYPTO_INFO_READY(crypto_info))
-		return -EBUSY;
+	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
+		/* Currently we only support setting crypto info more
+		 * than one time for TLS 1.3
+		 */
+		if (crypto_info->version != TLS_1_3_VERSION)
+			return -EBUSY;
+
+		update = true;
+		old_crypto_info = crypto_info;
+		crypto_info = &tmp.info;
+	}
 
 	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
 	if (rc) {
@@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 		goto err_crypto_info;
 	}
 
+	if (update) {
+		/* Ensure that TLS version and ciphers are not modified */
+		if (crypto_info->version != old_crypto_info->version ||
+		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
+			rc = -EINVAL;
+			goto err_crypto_info;
+		}
+	}
+
 	/* Ensure that TLS version and ciphers are same in both directions */
 	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
 		if (alt_crypto_info->version != crypto_info->version ||
@@ -772,7 +792,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXDEVICE);
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
 		} else {
-			rc = tls_set_sw_offload(sk, 1);
+			rc = tls_set_sw_offload(sk, 1,
+						update ? crypto_info : NULL);
 			if (rc)
 				goto err_crypto_info;
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXSW);
@@ -786,7 +807,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXDEVICE);
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRRXDEVICE);
 		} else {
-			rc = tls_set_sw_offload(sk, 0);
+			rc = tls_set_sw_offload(sk, 0,
+						update ? crypto_info : NULL);
 			if (rc)
 				goto err_crypto_info;
 			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXSW);
diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
index 22efea224a04..310135aaa6e6 100644
--- a/net/tls/tls_sw.c
+++ b/net/tls/tls_sw.c
@@ -2505,11 +2505,19 @@ void tls_update_rx_zc_capable(struct tls_context *tls_ctx)
 		tls_ctx->prot_info.version != TLS_1_3_VERSION;
 }
 
-int tls_set_sw_offload(struct sock *sk, int tx)
+static void tls_finish_key_update(struct tls_context *tls_ctx)
+{
+	struct tls_sw_context_rx *ctx = tls_ctx->priv_ctx_rx;
+
+	ctx->key_update_pending = false;
+}
+
+int tls_set_sw_offload(struct sock *sk, int tx,
+		       struct tls_crypto_info *new_crypto_info)
 {
 	struct tls_context *ctx = tls_get_ctx(sk);
 	struct tls_prot_info *prot = &ctx->prot_info;
-	struct tls_crypto_info *crypto_info;
+	struct tls_crypto_info *crypto_info, *src_crypto_info;
 	struct tls_sw_context_tx *sw_ctx_tx = NULL;
 	struct tls_sw_context_rx *sw_ctx_rx = NULL;
 	struct cipher_context *cctx;
@@ -2517,9 +2525,28 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	u16 nonce_size, tag_size, iv_size, rec_seq_size, salt_size;
 	struct crypto_tfm *tfm;
 	char *iv, *rec_seq, *key, *salt, *cipher_name;
-	size_t keysize;
+	size_t keysize, crypto_info_size;
 	int rc = 0;
 
+	if (new_crypto_info) {
+		/* non-NULL new_crypto_info means rekey */
+		src_crypto_info = new_crypto_info;
+		if (tx) {
+			sw_ctx_tx = ctx->priv_ctx_tx;
+			crypto_info = &ctx->crypto_send.info;
+			cctx = &ctx->tx;
+			aead = &sw_ctx_tx->aead_send;
+			sw_ctx_tx = NULL;
+		} else {
+			sw_ctx_rx = ctx->priv_ctx_rx;
+			crypto_info = &ctx->crypto_recv.info;
+			cctx = &ctx->rx;
+			aead = &sw_ctx_rx->aead_recv;
+			sw_ctx_rx = NULL;
+		}
+		goto skip_init;
+	}
+
 	if (tx) {
 		if (!ctx->priv_ctx_tx) {
 			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);
@@ -2566,12 +2593,15 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 		aead = &sw_ctx_rx->aead_recv;
 		sw_ctx_rx->key_update_pending = false;
 	}
+	src_crypto_info = crypto_info;
 
+skip_init:
 	switch (crypto_info->cipher_type) {
 	case TLS_CIPHER_AES_GCM_128: {
 		struct tls12_crypto_info_aes_gcm_128 *gcm_128_info;
 
-		gcm_128_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_aes_gcm_128);
+		gcm_128_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_AES_GCM_128_IV_SIZE;
 		tag_size = TLS_CIPHER_AES_GCM_128_TAG_SIZE;
 		iv_size = TLS_CIPHER_AES_GCM_128_IV_SIZE;
@@ -2588,7 +2618,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_AES_GCM_256: {
 		struct tls12_crypto_info_aes_gcm_256 *gcm_256_info;
 
-		gcm_256_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_aes_gcm_256);
+		gcm_256_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
 		tag_size = TLS_CIPHER_AES_GCM_256_TAG_SIZE;
 		iv_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
@@ -2605,7 +2636,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_AES_CCM_128: {
 		struct tls12_crypto_info_aes_ccm_128 *ccm_128_info;
 
-		ccm_128_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_aes_ccm_128);
+		ccm_128_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_AES_CCM_128_IV_SIZE;
 		tag_size = TLS_CIPHER_AES_CCM_128_TAG_SIZE;
 		iv_size = TLS_CIPHER_AES_CCM_128_IV_SIZE;
@@ -2622,7 +2654,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_CHACHA20_POLY1305: {
 		struct tls12_crypto_info_chacha20_poly1305 *chacha20_poly1305_info;
 
-		chacha20_poly1305_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_chacha20_poly1305);
+		chacha20_poly1305_info = (void *)src_crypto_info;
 		nonce_size = 0;
 		tag_size = TLS_CIPHER_CHACHA20_POLY1305_TAG_SIZE;
 		iv_size = TLS_CIPHER_CHACHA20_POLY1305_IV_SIZE;
@@ -2639,7 +2672,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_SM4_GCM: {
 		struct tls12_crypto_info_sm4_gcm *sm4_gcm_info;
 
-		sm4_gcm_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_sm4_gcm);
+		sm4_gcm_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_SM4_GCM_IV_SIZE;
 		tag_size = TLS_CIPHER_SM4_GCM_TAG_SIZE;
 		iv_size = TLS_CIPHER_SM4_GCM_IV_SIZE;
@@ -2656,7 +2690,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_SM4_CCM: {
 		struct tls12_crypto_info_sm4_ccm *sm4_ccm_info;
 
-		sm4_ccm_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_sm4_ccm);
+		sm4_ccm_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_SM4_CCM_IV_SIZE;
 		tag_size = TLS_CIPHER_SM4_CCM_TAG_SIZE;
 		iv_size = TLS_CIPHER_SM4_CCM_IV_SIZE;
@@ -2673,7 +2708,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_ARIA_GCM_128: {
 		struct tls12_crypto_info_aria_gcm_128 *aria_gcm_128_info;
 
-		aria_gcm_128_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_aria_gcm_128);
+		aria_gcm_128_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_ARIA_GCM_128_IV_SIZE;
 		tag_size = TLS_CIPHER_ARIA_GCM_128_TAG_SIZE;
 		iv_size = TLS_CIPHER_ARIA_GCM_128_IV_SIZE;
@@ -2690,7 +2726,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	case TLS_CIPHER_ARIA_GCM_256: {
 		struct tls12_crypto_info_aria_gcm_256 *gcm_256_info;
 
-		gcm_256_info = (void *)crypto_info;
+		crypto_info_size = sizeof(struct tls12_crypto_info_aria_gcm_256);
+		gcm_256_info = (void *)src_crypto_info;
 		nonce_size = TLS_CIPHER_ARIA_GCM_256_IV_SIZE;
 		tag_size = TLS_CIPHER_ARIA_GCM_256_TAG_SIZE;
 		iv_size = TLS_CIPHER_ARIA_GCM_256_IV_SIZE;
@@ -2734,19 +2771,26 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 			      prot->tag_size + prot->tail_size;
 	prot->iv_size = iv_size;
 	prot->salt_size = salt_size;
-	cctx->iv = kmalloc(iv_size + salt_size, GFP_KERNEL);
-	if (!cctx->iv) {
-		rc = -ENOMEM;
-		goto free_priv;
+	if (!new_crypto_info) {
+		cctx->iv = kmalloc(iv_size + salt_size, GFP_KERNEL);
+		if (!cctx->iv) {
+			rc = -ENOMEM;
+			goto free_priv;
+		}
 	}
 	/* Note: 128 & 256 bit salt are the same size */
 	prot->rec_seq_size = rec_seq_size;
 	memcpy(cctx->iv, salt, salt_size);
 	memcpy(cctx->iv + salt_size, iv, iv_size);
-	cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
-	if (!cctx->rec_seq) {
-		rc = -ENOMEM;
-		goto free_iv;
+
+	if (!new_crypto_info) {
+		cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
+		if (!cctx->rec_seq) {
+			rc = -ENOMEM;
+			goto free_iv;
+		}
+	} else {
+		memcpy(cctx->rec_seq, rec_seq, rec_seq_size);
 	}
 
 	if (!*aead) {
@@ -2761,13 +2805,20 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	ctx->push_pending_record = tls_sw_push_pending_record;
 
 	rc = crypto_aead_setkey(*aead, key, keysize);
-
-	if (rc)
-		goto free_aead;
+	if (rc) {
+		if (new_crypto_info)
+			goto out;
+		else
+			goto free_aead;
+	}
 
 	rc = crypto_aead_setauthsize(*aead, prot->tag_size);
-	if (rc)
-		goto free_aead;
+	if (rc) {
+		if (new_crypto_info)
+			goto out;
+		else
+			goto free_aead;
+	}
 
 	if (sw_ctx_rx) {
 		tfm = crypto_aead_tfm(sw_ctx_rx->aead_recv);
@@ -2782,6 +2833,13 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 			goto free_aead;
 	}
 
+	if (new_crypto_info) {
+		memcpy(crypto_info, new_crypto_info, crypto_info_size);
+		memzero_explicit(new_crypto_info, crypto_info_size);
+		if (!tx)
+			tls_finish_key_update(ctx);
+	}
+
 	goto out;
 
 free_aead:
@@ -2794,12 +2852,14 @@ int tls_set_sw_offload(struct sock *sk, int tx)
 	kfree(cctx->iv);
 	cctx->iv = NULL;
 free_priv:
-	if (tx) {
-		kfree(ctx->priv_ctx_tx);
-		ctx->priv_ctx_tx = NULL;
-	} else {
-		kfree(ctx->priv_ctx_rx);
-		ctx->priv_ctx_rx = NULL;
+	if (!new_crypto_info) {
+		if (tx) {
+			kfree(ctx->priv_ctx_tx);
+			ctx->priv_ctx_tx = NULL;
+		} else {
+			kfree(ctx->priv_ctx_rx);
+			ctx->priv_ctx_rx = NULL;
+		}
 	}
 out:
 	return rc;
-- 
2.38.1

[PATCH net-next 4/5] selftests: tls: add key_generation argument to tls_crypto_info_init

[Sabrina Dubroca]

This allows us to generate different keys, so that we can test that
rekey is using the correct one.

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 tools/testing/selftests/net/tls.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 2cbb12736596..5f3adb28fee1 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -38,9 +38,11 @@ struct tls_crypto_info_keys {
 };
 
 static void tls_crypto_info_init(uint16_t tls_version, uint16_t cipher_type,
-				 struct tls_crypto_info_keys *tls12)
+				 struct tls_crypto_info_keys *tls12,
+				 char key_generation)
 {
-	memset(tls12, 0, sizeof(*tls12));
+	memset(tls12, key_generation, sizeof(*tls12));
+	memset(tls12, 0, sizeof(struct tls_crypto_info));
 
 	switch (cipher_type) {
 	case TLS_CIPHER_CHACHA20_POLY1305:
@@ -312,7 +314,7 @@ FIXTURE_SETUP(tls)
 	int ret;
 
 	tls_crypto_info_init(variant->tls_version, variant->cipher_type,
-			     &tls12);
+			     &tls12, 0);
 
 	ulp_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
 
@@ -1071,7 +1073,7 @@ TEST_F(tls, bidir)
 		struct tls_crypto_info_keys tls12;
 
 		tls_crypto_info_init(variant->tls_version, variant->cipher_type,
-				     &tls12);
+				     &tls12, 0);
 
 		ret = setsockopt(self->fd, SOL_TLS, TLS_RX, &tls12,
 				 tls12.len);
@@ -1479,7 +1481,7 @@ FIXTURE_SETUP(tls_err)
 	int ret;
 
 	tls_crypto_info_init(variant->tls_version, TLS_CIPHER_AES_GCM_128,
-			     &tls12);
+			     &tls12, 0);
 
 	ulp_sock_pair(_metadata, &self->fd, &self->cfd, &self->notls);
 	ulp_sock_pair(_metadata, &self->fd2, &self->cfd2, &self->notls);
@@ -1771,7 +1773,7 @@ TEST(tls_v6ops) {
 	int sfd, ret, fd;
 	socklen_t len, len2;
 
-	tls_crypto_info_init(TLS_1_2_VERSION, TLS_CIPHER_AES_GCM_128, &tls12);
+	tls_crypto_info_init(TLS_1_2_VERSION, TLS_CIPHER_AES_GCM_128, &tls12, 0);
 
 	addr.sin6_family = AF_INET6;
 	addr.sin6_addr = in6addr_any;
-- 
2.38.1

[PATCH net-next 5/5] selftests: tls: add rekey tests

[Sabrina Dubroca]

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
---
 tools/testing/selftests/net/tls.c | 258 ++++++++++++++++++++++++++++++
 1 file changed, 258 insertions(+)

diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
index 5f3adb28fee1..97c20e2246e1 100644
--- a/tools/testing/selftests/net/tls.c
+++ b/tools/testing/selftests/net/tls.c
@@ -1453,6 +1453,264 @@ TEST_F(tls, shutdown_reuse)
 	EXPECT_EQ(errno, EISCONN);
 }
 
+#define TLS_RECORD_TYPE_HANDSHAKE      0x16
+/* key_update, length 1, update_not_requested */
+static const char key_update_msg[] = "\x18\x00\x00\x01\x00";
+static void tls_send_keyupdate(struct __test_metadata *_metadata, int fd)
+{
+	size_t len = sizeof(key_update_msg);
+
+	EXPECT_EQ(tls_send_cmsg(fd, TLS_RECORD_TYPE_HANDSHAKE,
+				(char *)key_update_msg, len, 0),
+		  len);
+}
+
+static void tls_recv_keyupdate(struct __test_metadata *_metadata, int fd, int flags)
+{
+	char buf[100];
+
+	EXPECT_EQ(tls_recv_cmsg(_metadata, fd, TLS_RECORD_TYPE_HANDSHAKE, buf, sizeof(buf), flags),
+		  sizeof(key_update_msg));
+	EXPECT_EQ(memcmp(buf, key_update_msg, sizeof(key_update_msg)), 0);
+}
+
+/* set the key to 0 then 1 for RX, immediately to 1 for TX */
+TEST_F(tls_basic, rekey_rx)
+{
+	struct tls_crypto_info_keys tls12_0, tls12_1;
+	char const *test_str = "test_message";
+	int send_len = strlen(test_str) + 1;
+	char buf[20];
+	int ret;
+
+	if (self->notls)
+		return;
+
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
+			     &tls12_0, 0);
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
+			     &tls12_1, 1);
+
+
+	ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_1, tls12_1.len);
+	ASSERT_EQ(ret, 0);
+
+	ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_0, tls12_0.len);
+	ASSERT_EQ(ret, 0);
+
+	ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_1, tls12_1.len);
+	EXPECT_EQ(ret, 0);
+
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+}
+
+/* set the key to 0 then 1 for TX, immediately to 1 for RX */
+TEST_F(tls_basic, rekey_tx)
+{
+	struct tls_crypto_info_keys tls12_0, tls12_1;
+	char const *test_str = "test_message";
+	int send_len = strlen(test_str) + 1;
+	char buf[20];
+	int ret;
+
+	if (self->notls)
+		return;
+
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
+			     &tls12_0, 0);
+	tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
+			     &tls12_1, 1);
+
+
+	ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_0, tls12_0.len);
+	ASSERT_EQ(ret, 0);
+
+	ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_1, tls12_1.len);
+	ASSERT_EQ(ret, 0);
+
+	ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_1, tls12_1.len);
+	EXPECT_EQ(ret, 0);
+
+	EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
+	EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
+	EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
+}
+
+TEST_F(tls, rekey)
+{
+	char const *test_str_1 = "test_message_before_rekey";
+	char const *test_str_2 = "test_message_after_rekey";
+	struct tls_crypto_info_keys tls12;
+	int send_len;
+	char buf[100];
+
+	if (variant->tls_version != TLS_1_3_VERSION)
+		return;
+
+	/* initial send/recv */
+	send_len = strlen(test_str_1) + 1;
+	EXPECT_EQ(send(self->fd, test_str_1, send_len, 0), send_len);
+	EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
+	EXPECT_EQ(memcmp(buf, test_str_1, send_len), 0);
+
+	/* update TX key */
+	tls_send_keyupdate(_metadata, self->fd);
+	tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
+	EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
+
+	/* send after rekey */
+	send_len = strlen(test_str_2) + 1;
+	EXPECT_EQ(send(self->fd, test_str_2, send_len, 0), send_len);
+
+	/* can't receive the KeyUpdate without a control message */
+	EXPECT_EQ(recv(self->cfd, buf, send_len, 0), -1);
+
+	/* get KeyUpdate */
+	tls_recv_keyupdate(_metadata, self->cfd, 0);
+
+	/* recv blocking -> -EKEYEXPIRED */
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), 0), -1);
+	EXPECT_EQ(errno, EKEYEXPIRED);
+
+	/* recv non-blocking -> -EKEYEXPIRED */
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT), -1);
+	EXPECT_EQ(errno, EKEYEXPIRED);
+
+	/* update RX key */
+	EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
+
+	/* recv after rekey */
+	EXPECT_NE(recv(self->cfd, buf, send_len, 0), -1);
+	EXPECT_EQ(memcmp(buf, test_str_2, send_len), 0);
+}
+
+TEST_F(tls, rekey_peek)
+{
+	char const *test_str_1 = "test_message_before_rekey";
+	struct tls_crypto_info_keys tls12;
+	int send_len;
+	char buf[100];
+
+	if (variant->tls_version != TLS_1_3_VERSION)
+		return;
+
+	send_len = strlen(test_str_1) + 1;
+	EXPECT_EQ(send(self->fd, test_str_1, send_len, 0), send_len);
+
+	/* update TX key */
+	tls_send_keyupdate(_metadata, self->fd);
+	tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
+	EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
+
+	EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_PEEK), send_len);
+	EXPECT_EQ(memcmp(buf, test_str_1, send_len), 0);
+
+	EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
+	EXPECT_EQ(memcmp(buf, test_str_1, send_len), 0);
+
+	/* can't receive the KeyUpdate without a control message */
+	EXPECT_EQ(recv(self->cfd, buf, send_len, MSG_PEEK), -1);
+
+	/* peek KeyUpdate */
+	tls_recv_keyupdate(_metadata, self->cfd, MSG_PEEK);
+
+	/* get KeyUpdate */
+	tls_recv_keyupdate(_metadata, self->cfd, 0);
+
+	/* update RX key */
+	EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
+}
+
+TEST_F(tls, splice_rekey)
+{
+	int send_len = TLS_PAYLOAD_MAX_LEN / 2;
+	char mem_send[TLS_PAYLOAD_MAX_LEN];
+	char mem_recv[TLS_PAYLOAD_MAX_LEN];
+	struct tls_crypto_info_keys tls12;
+	int p[2];
+
+	if (variant->tls_version != TLS_1_3_VERSION)
+		return;
+
+	memrnd(mem_send, sizeof(mem_send));
+
+	ASSERT_GE(pipe(p), 0);
+	EXPECT_EQ(send(self->fd, mem_send, send_len, 0), send_len);
+
+	/* update TX key */
+	tls_send_keyupdate(_metadata, self->fd);
+	tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
+	EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
+
+	EXPECT_EQ(send(self->fd, mem_send, send_len, 0), send_len);
+
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), send_len);
+	EXPECT_EQ(read(p[0], mem_recv, send_len), send_len);
+	EXPECT_EQ(memcmp(mem_send, mem_recv, send_len), 0);
+
+	/* can't splice the KeyUpdate */
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), -1);
+	EXPECT_EQ(errno, EINVAL);
+
+	/* peek KeyUpdate */
+	tls_recv_keyupdate(_metadata, self->cfd, MSG_PEEK);
+
+	/* get KeyUpdate */
+	tls_recv_keyupdate(_metadata, self->cfd, 0);
+
+	/* can't splice before updating the key */
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), -1);
+	EXPECT_EQ(errno, EKEYEXPIRED);
+
+	/* update RX key */
+	EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
+
+	EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), send_len);
+	EXPECT_EQ(read(p[0], mem_recv, send_len), send_len);
+	EXPECT_EQ(memcmp(mem_send, mem_recv, send_len), 0);
+}
+
+TEST_F(tls, rekey_getsockopt)
+{
+	struct tls_crypto_info_keys tls12;
+	struct tls_crypto_info_keys tls12_get;
+	socklen_t len;
+
+	tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 0);
+
+	len = tls12.len;
+	EXPECT_EQ(getsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_get, &len), 0);
+	EXPECT_EQ(len, tls12.len);
+	EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
+
+	len = tls12.len;
+	EXPECT_EQ(getsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_get, &len), 0);
+	EXPECT_EQ(len, tls12.len);
+	EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
+
+	if (variant->tls_version != TLS_1_3_VERSION)
+		return;
+
+	tls_send_keyupdate(_metadata, self->fd);
+	tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
+	EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
+
+	tls_recv_keyupdate(_metadata, self->cfd, 0);
+	EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
+
+	len = tls12.len;
+	EXPECT_EQ(getsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_get, &len), 0);
+	EXPECT_EQ(len, tls12.len);
+	EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
+
+	len = tls12.len;
+	EXPECT_EQ(getsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_get, &len), 0);
+	EXPECT_EQ(len, tls12.len);
+	EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
+}
+
 FIXTURE(tls_err)
 {
 	int fd, cfd;
-- 
2.38.1

Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Kuniyuki Iwashima]

Hi,

Thanks for posting this series!
We were working on the same feature.
CC Apoorv from s2n team.

From:   Sabrina Dubroca <sd@queasysnail.net>
Date:   Tue, 17 Jan 2023 14:45:29 +0100
> This adds the possibility to change the key and IV when using
> TLS1.3. Changing the cipher or TLS version is not supported.
> 
> Once we have updated the RX key, we can unblock the receive side.
> 
> This change only affects tls_sw, since 1.3 offload isn't supported.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Tested-by: Frantisek Krenzelok <fkrenzel@redhat.com>
> ---
>  net/tls/tls.h        |   3 +-
>  net/tls/tls_device.c |   2 +-
>  net/tls/tls_main.c   |  32 ++++++++++--
>  net/tls/tls_sw.c     | 120 ++++++++++++++++++++++++++++++++-----------
>  4 files changed, 120 insertions(+), 37 deletions(-)
> 
> diff --git a/net/tls/tls.h b/net/tls/tls.h
> index 34d0fe814600..6f9c85eaa9c5 100644
> --- a/net/tls/tls.h
> +++ b/net/tls/tls.h
> @@ -90,7 +90,8 @@ int tls_sk_attach(struct sock *sk, int optname, char __user *optval,
>  		  unsigned int optlen);
>  void tls_err_abort(struct sock *sk, int err);
>  
> -int tls_set_sw_offload(struct sock *sk, int tx);
> +int tls_set_sw_offload(struct sock *sk, int tx,
> +		       struct tls_crypto_info *new_crypto_info);
>  void tls_update_rx_zc_capable(struct tls_context *tls_ctx);
>  void tls_sw_strparser_arm(struct sock *sk, struct tls_context *ctx);
>  void tls_sw_strparser_done(struct tls_context *tls_ctx);
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index c149f36b42ee..1ad50c253dfe 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -1291,7 +1291,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
>  	context->resync_nh_reset = 1;
>  
>  	ctx->priv_ctx_rx = context;
> -	rc = tls_set_sw_offload(sk, 0);
> +	rc = tls_set_sw_offload(sk, 0, NULL);
>  	if (rc)
>  		goto release_ctx;
>  
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index fb1da1780f50..9be82aecd13e 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
>  static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>  				  unsigned int optlen, int tx)
>  {
> +	union tls_crypto_context tmp = {};
> +	struct tls_crypto_info *old_crypto_info = NULL;
>  	struct tls_crypto_info *crypto_info;
>  	struct tls_crypto_info *alt_crypto_info;
>  	struct tls_context *ctx = tls_get_ctx(sk);
> +	bool update = false;
>  	size_t optsize;
>  	int rc = 0;
>  	int conf;
> @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>  		alt_crypto_info = &ctx->crypto_send.info;
>  	}
>  
> -	/* Currently we don't support set crypto info more than one time */
> -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> -		return -EBUSY;
> +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> +		/* Currently we only support setting crypto info more
> +		 * than one time for TLS 1.3
> +		 */
> +		if (crypto_info->version != TLS_1_3_VERSION)
> +			return -EBUSY;
> +

Should we check this ?

                if (!tx && !key_update_pending)
                        return -EBUSY;

Otherwise we can set a new RX key even if the other end has not sent
KeyUpdateRequest.


> +		update = true;
> +		old_crypto_info = crypto_info;
> +		crypto_info = &tmp.info;
> +	}
>  
>  	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
>  	if (rc) {
> @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>  		goto err_crypto_info;
>  	}
>  
> +	if (update) {
> +		/* Ensure that TLS version and ciphers are not modified */
> +		if (crypto_info->version != old_crypto_info->version ||
> +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> +			rc = -EINVAL;
> +			goto err_crypto_info;
> +		}
> +	}
> +
>  	/* Ensure that TLS version and ciphers are same in both directions */
>  	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {

We can change this to else-if.


>  		if (alt_crypto_info->version != crypto_info->version ||
> @@ -772,7 +792,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>  			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXDEVICE);
>  			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
>  		} else {
> -			rc = tls_set_sw_offload(sk, 1);
> +			rc = tls_set_sw_offload(sk, 1,
> +						update ? crypto_info : NULL);
>  			if (rc)
>  				goto err_crypto_info;
>  			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXSW);
> @@ -786,7 +807,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>  			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXDEVICE);
>  			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRRXDEVICE);
>  		} else {
> -			rc = tls_set_sw_offload(sk, 0);
> +			rc = tls_set_sw_offload(sk, 0,
> +						update ? crypto_info : NULL);
>  			if (rc)
>  				goto err_crypto_info;
>  			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXSW);
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 22efea224a04..310135aaa6e6 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2505,11 +2505,19 @@ void tls_update_rx_zc_capable(struct tls_context *tls_ctx)
>  		tls_ctx->prot_info.version != TLS_1_3_VERSION;
>  }
>  
> -int tls_set_sw_offload(struct sock *sk, int tx)
> +static void tls_finish_key_update(struct tls_context *tls_ctx)
> +{
> +	struct tls_sw_context_rx *ctx = tls_ctx->priv_ctx_rx;
> +
> +	ctx->key_update_pending = false;
> +}
> +
> +int tls_set_sw_offload(struct sock *sk, int tx,
> +		       struct tls_crypto_info *new_crypto_info)
>  {
>  	struct tls_context *ctx = tls_get_ctx(sk);
>  	struct tls_prot_info *prot = &ctx->prot_info;
> -	struct tls_crypto_info *crypto_info;
> +	struct tls_crypto_info *crypto_info, *src_crypto_info;
>  	struct tls_sw_context_tx *sw_ctx_tx = NULL;
>  	struct tls_sw_context_rx *sw_ctx_rx = NULL;
>  	struct cipher_context *cctx;
> @@ -2517,9 +2525,28 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	u16 nonce_size, tag_size, iv_size, rec_seq_size, salt_size;
>  	struct crypto_tfm *tfm;
>  	char *iv, *rec_seq, *key, *salt, *cipher_name;
> -	size_t keysize;
> +	size_t keysize, crypto_info_size;
>  	int rc = 0;
>  
> +	if (new_crypto_info) {
> +		/* non-NULL new_crypto_info means rekey */
> +		src_crypto_info = new_crypto_info;
> +		if (tx) {
> +			sw_ctx_tx = ctx->priv_ctx_tx;
> +			crypto_info = &ctx->crypto_send.info;
> +			cctx = &ctx->tx;
> +			aead = &sw_ctx_tx->aead_send;
> +			sw_ctx_tx = NULL;

sw_ctx_tx is already initialised.


> +		} else {
> +			sw_ctx_rx = ctx->priv_ctx_rx;
> +			crypto_info = &ctx->crypto_recv.info;
> +			cctx = &ctx->rx;
> +			aead = &sw_ctx_rx->aead_recv;
> +			sw_ctx_rx = NULL;

Same here.


> +		}
> +		goto skip_init;
> +	}
> +
>  	if (tx) {
>  		if (!ctx->priv_ctx_tx) {
>  			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);
> @@ -2566,12 +2593,15 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  		aead = &sw_ctx_rx->aead_recv;
>  		sw_ctx_rx->key_update_pending = false;
>  	}
> +	src_crypto_info = crypto_info;
>  
> +skip_init:
>  	switch (crypto_info->cipher_type) {
>  	case TLS_CIPHER_AES_GCM_128: {
>  		struct tls12_crypto_info_aes_gcm_128 *gcm_128_info;
>  
> -		gcm_128_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aes_gcm_128);
> +		gcm_128_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_AES_GCM_128_IV_SIZE;
>  		tag_size = TLS_CIPHER_AES_GCM_128_TAG_SIZE;
>  		iv_size = TLS_CIPHER_AES_GCM_128_IV_SIZE;
> @@ -2588,7 +2618,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_AES_GCM_256: {
>  		struct tls12_crypto_info_aes_gcm_256 *gcm_256_info;
>  
> -		gcm_256_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aes_gcm_256);
> +		gcm_256_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
>  		tag_size = TLS_CIPHER_AES_GCM_256_TAG_SIZE;
>  		iv_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
> @@ -2605,7 +2636,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_AES_CCM_128: {
>  		struct tls12_crypto_info_aes_ccm_128 *ccm_128_info;
>  
> -		ccm_128_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aes_ccm_128);
> +		ccm_128_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_AES_CCM_128_IV_SIZE;
>  		tag_size = TLS_CIPHER_AES_CCM_128_TAG_SIZE;
>  		iv_size = TLS_CIPHER_AES_CCM_128_IV_SIZE;
> @@ -2622,7 +2654,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_CHACHA20_POLY1305: {
>  		struct tls12_crypto_info_chacha20_poly1305 *chacha20_poly1305_info;
>  
> -		chacha20_poly1305_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_chacha20_poly1305);
> +		chacha20_poly1305_info = (void *)src_crypto_info;
>  		nonce_size = 0;
>  		tag_size = TLS_CIPHER_CHACHA20_POLY1305_TAG_SIZE;
>  		iv_size = TLS_CIPHER_CHACHA20_POLY1305_IV_SIZE;
> @@ -2639,7 +2672,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_SM4_GCM: {
>  		struct tls12_crypto_info_sm4_gcm *sm4_gcm_info;
>  
> -		sm4_gcm_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_sm4_gcm);
> +		sm4_gcm_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_SM4_GCM_IV_SIZE;
>  		tag_size = TLS_CIPHER_SM4_GCM_TAG_SIZE;
>  		iv_size = TLS_CIPHER_SM4_GCM_IV_SIZE;
> @@ -2656,7 +2690,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_SM4_CCM: {
>  		struct tls12_crypto_info_sm4_ccm *sm4_ccm_info;
>  
> -		sm4_ccm_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_sm4_ccm);
> +		sm4_ccm_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_SM4_CCM_IV_SIZE;
>  		tag_size = TLS_CIPHER_SM4_CCM_TAG_SIZE;
>  		iv_size = TLS_CIPHER_SM4_CCM_IV_SIZE;
> @@ -2673,7 +2708,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_ARIA_GCM_128: {
>  		struct tls12_crypto_info_aria_gcm_128 *aria_gcm_128_info;
>  
> -		aria_gcm_128_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aria_gcm_128);
> +		aria_gcm_128_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_ARIA_GCM_128_IV_SIZE;
>  		tag_size = TLS_CIPHER_ARIA_GCM_128_TAG_SIZE;
>  		iv_size = TLS_CIPHER_ARIA_GCM_128_IV_SIZE;
> @@ -2690,7 +2726,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	case TLS_CIPHER_ARIA_GCM_256: {
>  		struct tls12_crypto_info_aria_gcm_256 *gcm_256_info;
>  
> -		gcm_256_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aria_gcm_256);
> +		gcm_256_info = (void *)src_crypto_info;
>  		nonce_size = TLS_CIPHER_ARIA_GCM_256_IV_SIZE;
>  		tag_size = TLS_CIPHER_ARIA_GCM_256_TAG_SIZE;
>  		iv_size = TLS_CIPHER_ARIA_GCM_256_IV_SIZE;
> @@ -2734,19 +2771,26 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  			      prot->tag_size + prot->tail_size;
>  	prot->iv_size = iv_size;
>  	prot->salt_size = salt_size;
> -	cctx->iv = kmalloc(iv_size + salt_size, GFP_KERNEL);
> -	if (!cctx->iv) {
> -		rc = -ENOMEM;
> -		goto free_priv;
> +	if (!new_crypto_info) {
> +		cctx->iv = kmalloc(iv_size + salt_size, GFP_KERNEL);
> +		if (!cctx->iv) {
> +			rc = -ENOMEM;
> +			goto free_priv;
> +		}
>  	}
>  	/* Note: 128 & 256 bit salt are the same size */
>  	prot->rec_seq_size = rec_seq_size;
>  	memcpy(cctx->iv, salt, salt_size);
>  	memcpy(cctx->iv + salt_size, iv, iv_size);
> -	cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
> -	if (!cctx->rec_seq) {
> -		rc = -ENOMEM;
> -		goto free_iv;
> +
> +	if (!new_crypto_info) {
> +		cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
> +		if (!cctx->rec_seq) {
> +			rc = -ENOMEM;
> +			goto free_iv;
> +		}
> +	} else {
> +		memcpy(cctx->rec_seq, rec_seq, rec_seq_size);
>  	}
>  
>  	if (!*aead) {
> @@ -2761,13 +2805,20 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	ctx->push_pending_record = tls_sw_push_pending_record;
>  
>  	rc = crypto_aead_setkey(*aead, key, keysize);
> -
> -	if (rc)
> -		goto free_aead;
> +	if (rc) {
> +		if (new_crypto_info)
> +			goto out;
> +		else
> +			goto free_aead;
> +	}
>  
>  	rc = crypto_aead_setauthsize(*aead, prot->tag_size);
> -	if (rc)
> -		goto free_aead;
> +	if (rc) {
> +		if (new_crypto_info)
> +			goto out;
> +		else
> +			goto free_aead;
> +	}
>  
>  	if (sw_ctx_rx) {
>  		tfm = crypto_aead_tfm(sw_ctx_rx->aead_recv);
> @@ -2782,6 +2833,13 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  			goto free_aead;
>  	}
>  
> +	if (new_crypto_info) {
> +		memcpy(crypto_info, new_crypto_info, crypto_info_size);
> +		memzero_explicit(new_crypto_info, crypto_info_size);
> +		if (!tx)
> +			tls_finish_key_update(ctx);
> +	}
> +
>  	goto out;
>  
>  free_aead:
> @@ -2794,12 +2852,14 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>  	kfree(cctx->iv);
>  	cctx->iv = NULL;
>  free_priv:
> -	if (tx) {
> -		kfree(ctx->priv_ctx_tx);
> -		ctx->priv_ctx_tx = NULL;
> -	} else {
> -		kfree(ctx->priv_ctx_rx);
> -		ctx->priv_ctx_rx = NULL;
> +	if (!new_crypto_info) {
> +		if (tx) {
> +			kfree(ctx->priv_ctx_tx);
> +			ctx->priv_ctx_tx = NULL;
> +		} else {
> +			kfree(ctx->priv_ctx_rx);
> +			ctx->priv_ctx_rx = NULL;
> +		}
>  	}
>  out:
>  	return rc;
> -- 
> 2.38.1

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Jakub Kicinski]

Please CC all the maintainers.

On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:
> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> [1]). A sender transmits a KeyUpdate message and then changes its TX
> key. The receiver should react by updating its RX key before
> processing the next message.
> 
> This patchset implements key updates by:
>  1. pausing decryption when a KeyUpdate message is received, to avoid
>     attempting to use the old key to decrypt a record encrypted with
>     the new key
>  2. returning -EKEYEXPIRED to syscalls that cannot receive the
>     KeyUpdate message, until the rekey has been performed by userspace

Why? We return to user space after hitting a cmsg, don't we?
If the user space wants to keep reading with the old key - 🤷️

>  3. passing the KeyUpdate message to userspace as a control message
>  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
>     setsockopts
> 
> This API has been tested with gnutls to make sure that it allows
> userspace libraries to implement key updates [2]. Thanks to Frantisek
> Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> gnutls and testing the kernel patches.

Please explain why - the kernel TLS is not faster than user space, 
the point of it is primarily to enable offload. And you don't add
offload support here.
 
> Note: in a future series, I'll clean up tls_set_sw_offload and
> eliminate the per-cipher copy-paste using tls_cipher_size_desc.

Yeah, I think it's on Vadim's TODO list as well.

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Sabrina Dubroca]

2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> Please CC all the maintainers.

Sorry.

> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:
> > This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> > [1]). A sender transmits a KeyUpdate message and then changes its TX
> > key. The receiver should react by updating its RX key before
> > processing the next message.
> > 
> > This patchset implements key updates by:
> >  1. pausing decryption when a KeyUpdate message is received, to avoid
> >     attempting to use the old key to decrypt a record encrypted with
> >     the new key
> >  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> >     KeyUpdate message, until the rekey has been performed by userspace
> 
> Why? We return to user space after hitting a cmsg, don't we?
> If the user space wants to keep reading with the old key - 🤷️

But they won't be able to read anything. Either we don't pause
decryption, and the socket is just broken when we look at the next
record, or we pause, and there's nothing to read until the rekey is
done. I think that -EKEYEXPIRED is better than breaking the socket
just because a read snuck in between getting the cmsg and setting the
new key.

> >  3. passing the KeyUpdate message to userspace as a control message
> >  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> >     setsockopts
> > 
> > This API has been tested with gnutls to make sure that it allows
> > userspace libraries to implement key updates [2]. Thanks to Frantisek
> > Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> > gnutls and testing the kernel patches.
> 
> Please explain why - the kernel TLS is not faster than user space, 
> the point of it is primarily to enable offload. And you don't add
> offload support here.

Well, TLS1.3 support was added 4 years ago, and yet the offload still
doesn't support 1.3 at all.

IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
kernel can't claim to support 1.3, independent of offloading.

Some folks did tests with and without kTLS using nbdcopy and found a
small but noticeable performance improvement (around 8-10%).

> > Note: in a future series, I'll clean up tls_set_sw_offload and
> > eliminate the per-cipher copy-paste using tls_cipher_size_desc.
> 
> Yeah, I think it's on Vadim's TODO list as well.

I've already done most of the work as I was working on this, I'll
submit it later.

-- 
Sabrina

Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Sabrina Dubroca]

2023-01-17, 15:16:33 -0800, Kuniyuki Iwashima wrote:
> Hi,
> 
> Thanks for posting this series!
> We were working on the same feature.
> CC Apoorv from s2n team.

Ah, cool. Does the behavior in those patches match what your
implementation?

[...]
> > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > index fb1da1780f50..9be82aecd13e 100644
> > --- a/net/tls/tls_main.c
> > +++ b/net/tls/tls_main.c
> > @@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
> >  static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >  				  unsigned int optlen, int tx)
> >  {
> > +	union tls_crypto_context tmp = {};
> > +	struct tls_crypto_info *old_crypto_info = NULL;
> >  	struct tls_crypto_info *crypto_info;
> >  	struct tls_crypto_info *alt_crypto_info;
> >  	struct tls_context *ctx = tls_get_ctx(sk);
> > +	bool update = false;
> >  	size_t optsize;
> >  	int rc = 0;
> >  	int conf;
> > @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >  		alt_crypto_info = &ctx->crypto_send.info;
> >  	}
> >  
> > -	/* Currently we don't support set crypto info more than one time */
> > -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> > -		return -EBUSY;
> > +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> > +		/* Currently we only support setting crypto info more
> > +		 * than one time for TLS 1.3
> > +		 */
> > +		if (crypto_info->version != TLS_1_3_VERSION)
> > +			return -EBUSY;
> > +
> 
> Should we check this ?
> 
>                 if (!tx && !key_update_pending)
>                         return -EBUSY;
> 
> Otherwise we can set a new RX key even if the other end has not sent
> KeyUpdateRequest.

Maybe. My thinking was "let userspace shoot itself in the foot if it
wants".

> > +		update = true;
> > +		old_crypto_info = crypto_info;
> > +		crypto_info = &tmp.info;
> > +	}
> >  
> >  	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
> >  	if (rc) {
> > @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >  		goto err_crypto_info;
> >  	}
> >  
> > +	if (update) {
> > +		/* Ensure that TLS version and ciphers are not modified */
> > +		if (crypto_info->version != old_crypto_info->version ||
> > +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> > +			rc = -EINVAL;
> > +			goto err_crypto_info;
> > +		}
> > +	}
> > +
> >  	/* Ensure that TLS version and ciphers are same in both directions */
> >  	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
> 
> We can change this to else-if.

Ok.

> >  		if (alt_crypto_info->version != crypto_info->version ||
[...]
> > @@ -2517,9 +2525,28 @@ int tls_set_sw_offload(struct sock *sk, int tx)
> >  	u16 nonce_size, tag_size, iv_size, rec_seq_size, salt_size;
> >  	struct crypto_tfm *tfm;
> >  	char *iv, *rec_seq, *key, *salt, *cipher_name;
> > -	size_t keysize;
> > +	size_t keysize, crypto_info_size;
> >  	int rc = 0;
> >  
> > +	if (new_crypto_info) {
> > +		/* non-NULL new_crypto_info means rekey */
> > +		src_crypto_info = new_crypto_info;
> > +		if (tx) {
> > +			sw_ctx_tx = ctx->priv_ctx_tx;
> > +			crypto_info = &ctx->crypto_send.info;
> > +			cctx = &ctx->tx;
> > +			aead = &sw_ctx_tx->aead_send;
> > +			sw_ctx_tx = NULL;
> 
> sw_ctx_tx is already initialised.

No, it was NULL at the beginning of the function, but then I used it
to set aead on the previous line, so I need to clear it again. I could
use a temp variable instead if you think it's better.

> > +		} else {
> > +			sw_ctx_rx = ctx->priv_ctx_rx;
> > +			crypto_info = &ctx->crypto_recv.info;
> > +			cctx = &ctx->rx;
> > +			aead = &sw_ctx_rx->aead_recv;
> > +			sw_ctx_rx = NULL;
> 
> Same here.
> 
> 
> > +		}
> > +		goto skip_init;
> > +	}
> > +
> >  	if (tx) {
> >  		if (!ctx->priv_ctx_tx) {
> >  			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);

Thanks for the comments.

-- 
Sabrina

Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Vadim Fedorenko]

On 17.01.2023 13:45, Sabrina Dubroca wrote:
> This adds the possibility to change the key and IV when using
> TLS1.3. Changing the cipher or TLS version is not supported.
> 
> Once we have updated the RX key, we can unblock the receive side.
> 
> This change only affects tls_sw, since 1.3 offload isn't supported.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Tested-by: Frantisek Krenzelok <fkrenzel@redhat.com>
> ---
>   net/tls/tls.h        |   3 +-
>   net/tls/tls_device.c |   2 +-
>   net/tls/tls_main.c   |  32 ++++++++++--
>   net/tls/tls_sw.c     | 120 ++++++++++++++++++++++++++++++++-----------
>   4 files changed, 120 insertions(+), 37 deletions(-)
> 
> diff --git a/net/tls/tls.h b/net/tls/tls.h
> index 34d0fe814600..6f9c85eaa9c5 100644
> --- a/net/tls/tls.h
> +++ b/net/tls/tls.h
> @@ -90,7 +90,8 @@ int tls_sk_attach(struct sock *sk, int optname, char __user *optval,
>   		  unsigned int optlen);
>   void tls_err_abort(struct sock *sk, int err);
>   
> -int tls_set_sw_offload(struct sock *sk, int tx);
> +int tls_set_sw_offload(struct sock *sk, int tx,
> +		       struct tls_crypto_info *new_crypto_info);
>   void tls_update_rx_zc_capable(struct tls_context *tls_ctx);
>   void tls_sw_strparser_arm(struct sock *sk, struct tls_context *ctx);
>   void tls_sw_strparser_done(struct tls_context *tls_ctx);
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index c149f36b42ee..1ad50c253dfe 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -1291,7 +1291,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
>   	context->resync_nh_reset = 1;
>   
>   	ctx->priv_ctx_rx = context;
> -	rc = tls_set_sw_offload(sk, 0);
> +	rc = tls_set_sw_offload(sk, 0, NULL);
>   	if (rc)
>   		goto release_ctx;
>   
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index fb1da1780f50..9be82aecd13e 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
>   static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   				  unsigned int optlen, int tx)
>   {
> +	union tls_crypto_context tmp = {};
> +	struct tls_crypto_info *old_crypto_info = NULL;
>   	struct tls_crypto_info *crypto_info;
>   	struct tls_crypto_info *alt_crypto_info;
>   	struct tls_context *ctx = tls_get_ctx(sk);
> +	bool update = false;
>   	size_t optsize;
>   	int rc = 0;
>   	int conf;
> @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   		alt_crypto_info = &ctx->crypto_send.info;
>   	}
>   
> -	/* Currently we don't support set crypto info more than one time */
> -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> -		return -EBUSY;
> +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> +		/* Currently we only support setting crypto info more
> +		 * than one time for TLS 1.3
> +		 */
> +		if (crypto_info->version != TLS_1_3_VERSION)
> +			return -EBUSY;
> +
> +		update = true;
> +		old_crypto_info = crypto_info;
> +		crypto_info = &tmp.info;
> +	}
>   
>   	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
>   	if (rc) {
> @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   		goto err_crypto_info;
>   	}
>   
> +	if (update) {
> +		/* Ensure that TLS version and ciphers are not modified */
> +		if (crypto_info->version != old_crypto_info->version ||
> +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> +			rc = -EINVAL;
> +			goto err_crypto_info;
> +		}
> +	}
> +

looks like these checks can be moved up to TLS_CRYPTO_INFO_READY scope and there 
will be no need for extra variables.

>   	/* Ensure that TLS version and ciphers are same in both directions */
>   	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
>   		if (alt_crypto_info->version != crypto_info->version ||
> @@ -772,7 +792,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXDEVICE);
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
>   		} else {
> -			rc = tls_set_sw_offload(sk, 1);
> +			rc = tls_set_sw_offload(sk, 1,
> +						update ? crypto_info : NULL);
>   			if (rc)
>   				goto err_crypto_info;
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXSW);
> @@ -786,7 +807,8 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXDEVICE);
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRRXDEVICE);
>   		} else {
> -			rc = tls_set_sw_offload(sk, 0);
> +			rc = tls_set_sw_offload(sk, 0,
> +						update ? crypto_info : NULL);
>   			if (rc)
>   				goto err_crypto_info;
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXSW);
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 22efea224a04..310135aaa6e6 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2505,11 +2505,19 @@ void tls_update_rx_zc_capable(struct tls_context *tls_ctx)
>   		tls_ctx->prot_info.version != TLS_1_3_VERSION;
>   }
>   
> -int tls_set_sw_offload(struct sock *sk, int tx)
> +static void tls_finish_key_update(struct tls_context *tls_ctx)
> +{
> +	struct tls_sw_context_rx *ctx = tls_ctx->priv_ctx_rx;
> +
> +	ctx->key_update_pending = false;
> +}
> +
> +int tls_set_sw_offload(struct sock *sk, int tx,
> +		       struct tls_crypto_info *new_crypto_info)
>   {
>   	struct tls_context *ctx = tls_get_ctx(sk);
>   	struct tls_prot_info *prot = &ctx->prot_info;
> -	struct tls_crypto_info *crypto_info;
> +	struct tls_crypto_info *crypto_info, *src_crypto_info;
>   	struct tls_sw_context_tx *sw_ctx_tx = NULL;
>   	struct tls_sw_context_rx *sw_ctx_rx = NULL;
>   	struct cipher_context *cctx;
> @@ -2517,9 +2525,28 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	u16 nonce_size, tag_size, iv_size, rec_seq_size, salt_size;
>   	struct crypto_tfm *tfm;
>   	char *iv, *rec_seq, *key, *salt, *cipher_name;
> -	size_t keysize;
> +	size_t keysize, crypto_info_size;
>   	int rc = 0;
>   
> +	if (new_crypto_info) {
> +		/* non-NULL new_crypto_info means rekey */
> +		src_crypto_info = new_crypto_info;
> +		if (tx) {
> +			sw_ctx_tx = ctx->priv_ctx_tx;
> +			crypto_info = &ctx->crypto_send.info;
> +			cctx = &ctx->tx;
> +			aead = &sw_ctx_tx->aead_send;
> +			sw_ctx_tx = NULL;
> +		} else {
> +			sw_ctx_rx = ctx->priv_ctx_rx;
> +			crypto_info = &ctx->crypto_recv.info;
> +			cctx = &ctx->rx;
> +			aead = &sw_ctx_rx->aead_recv;
> +			sw_ctx_rx = NULL;
> +		}
> +		goto skip_init;
> +	}
> +
>   	if (tx) {
>   		if (!ctx->priv_ctx_tx) {
>   			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);
> @@ -2566,12 +2593,15 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   		aead = &sw_ctx_rx->aead_recv;
>   		sw_ctx_rx->key_update_pending = false;
>   	}
> +	src_crypto_info = crypto_info;
>   
> +skip_init:
>   	switch (crypto_info->cipher_type) {
>   	case TLS_CIPHER_AES_GCM_128: {
>   		struct tls12_crypto_info_aes_gcm_128 *gcm_128_info;
>   
> -		gcm_128_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aes_gcm_128);
> +		gcm_128_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_AES_GCM_128_IV_SIZE;
>   		tag_size = TLS_CIPHER_AES_GCM_128_TAG_SIZE;
>   		iv_size = TLS_CIPHER_AES_GCM_128_IV_SIZE;
> @@ -2588,7 +2618,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_AES_GCM_256: {
>   		struct tls12_crypto_info_aes_gcm_256 *gcm_256_info;
>   
> -		gcm_256_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aes_gcm_256);
> +		gcm_256_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
>   		tag_size = TLS_CIPHER_AES_GCM_256_TAG_SIZE;
>   		iv_size = TLS_CIPHER_AES_GCM_256_IV_SIZE;
> @@ -2605,7 +2636,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_AES_CCM_128: {
>   		struct tls12_crypto_info_aes_ccm_128 *ccm_128_info;
>   
> -		ccm_128_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aes_ccm_128);
> +		ccm_128_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_AES_CCM_128_IV_SIZE;
>   		tag_size = TLS_CIPHER_AES_CCM_128_TAG_SIZE;
>   		iv_size = TLS_CIPHER_AES_CCM_128_IV_SIZE;
> @@ -2622,7 +2654,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_CHACHA20_POLY1305: {
>   		struct tls12_crypto_info_chacha20_poly1305 *chacha20_poly1305_info;
>   
> -		chacha20_poly1305_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_chacha20_poly1305);
> +		chacha20_poly1305_info = (void *)src_crypto_info;
>   		nonce_size = 0;
>   		tag_size = TLS_CIPHER_CHACHA20_POLY1305_TAG_SIZE;
>   		iv_size = TLS_CIPHER_CHACHA20_POLY1305_IV_SIZE;
> @@ -2639,7 +2672,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_SM4_GCM: {
>   		struct tls12_crypto_info_sm4_gcm *sm4_gcm_info;
>   
> -		sm4_gcm_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_sm4_gcm);
> +		sm4_gcm_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_SM4_GCM_IV_SIZE;
>   		tag_size = TLS_CIPHER_SM4_GCM_TAG_SIZE;
>   		iv_size = TLS_CIPHER_SM4_GCM_IV_SIZE;
> @@ -2656,7 +2690,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_SM4_CCM: {
>   		struct tls12_crypto_info_sm4_ccm *sm4_ccm_info;
>   
> -		sm4_ccm_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_sm4_ccm);
> +		sm4_ccm_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_SM4_CCM_IV_SIZE;
>   		tag_size = TLS_CIPHER_SM4_CCM_TAG_SIZE;
>   		iv_size = TLS_CIPHER_SM4_CCM_IV_SIZE;
> @@ -2673,7 +2708,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_ARIA_GCM_128: {
>   		struct tls12_crypto_info_aria_gcm_128 *aria_gcm_128_info;
>   
> -		aria_gcm_128_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aria_gcm_128);
> +		aria_gcm_128_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_ARIA_GCM_128_IV_SIZE;
>   		tag_size = TLS_CIPHER_ARIA_GCM_128_TAG_SIZE;
>   		iv_size = TLS_CIPHER_ARIA_GCM_128_IV_SIZE;
> @@ -2690,7 +2726,8 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	case TLS_CIPHER_ARIA_GCM_256: {
>   		struct tls12_crypto_info_aria_gcm_256 *gcm_256_info;
>   
> -		gcm_256_info = (void *)crypto_info;
> +		crypto_info_size = sizeof(struct tls12_crypto_info_aria_gcm_256);
> +		gcm_256_info = (void *)src_crypto_info;
>   		nonce_size = TLS_CIPHER_ARIA_GCM_256_IV_SIZE;
>   		tag_size = TLS_CIPHER_ARIA_GCM_256_TAG_SIZE;
>   		iv_size = TLS_CIPHER_ARIA_GCM_256_IV_SIZE;
> @@ -2734,19 +2771,26 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   			      prot->tag_size + prot->tail_size;
>   	prot->iv_size = iv_size;
>   	prot->salt_size = salt_size;
> -	cctx->iv = kmalloc(iv_size + salt_size, GFP_KERNEL);
> -	if (!cctx->iv) {
> -		rc = -ENOMEM;
> -		goto free_priv;
> +	if (!new_crypto_info) {
> +		cctx->iv = kmalloc(iv_size + salt_size, GFP_KERNEL);
> +		if (!cctx->iv) {
> +			rc = -ENOMEM;
> +			goto free_priv;
> +		}
>   	}
>   	/* Note: 128 & 256 bit salt are the same size */
>   	prot->rec_seq_size = rec_seq_size;
>   	memcpy(cctx->iv, salt, salt_size);
>   	memcpy(cctx->iv + salt_size, iv, iv_size);
> -	cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
> -	if (!cctx->rec_seq) {
> -		rc = -ENOMEM;
> -		goto free_iv;
> +
> +	if (!new_crypto_info) {
> +		cctx->rec_seq = kmemdup(rec_seq, rec_seq_size, GFP_KERNEL);
> +		if (!cctx->rec_seq) {
> +			rc = -ENOMEM;
> +			goto free_iv;
> +		}
> +	} else {
> +		memcpy(cctx->rec_seq, rec_seq, rec_seq_size);
>   	}
>   
>   	if (!*aead) {
> @@ -2761,13 +2805,20 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	ctx->push_pending_record = tls_sw_push_pending_record;
>   
>   	rc = crypto_aead_setkey(*aead, key, keysize);
> -
> -	if (rc)
> -		goto free_aead;
> +	if (rc) {
> +		if (new_crypto_info)
> +			goto out;
> +		else
> +			goto free_aead;
> +	}
>   
>   	rc = crypto_aead_setauthsize(*aead, prot->tag_size);
> -	if (rc)
> -		goto free_aead;
> +	if (rc) {
> +		if (new_crypto_info)
> +			goto out;
> +		else
> +			goto free_aead;
> +	}
>   
>   	if (sw_ctx_rx) {
>   		tfm = crypto_aead_tfm(sw_ctx_rx->aead_recv);
> @@ -2782,6 +2833,13 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   			goto free_aead;
>   	}
>   
> +	if (new_crypto_info) {
> +		memcpy(crypto_info, new_crypto_info, crypto_info_size);
> +		memzero_explicit(new_crypto_info, crypto_info_size);
> +		if (!tx)
> +			tls_finish_key_update(ctx);
> +	}
> +
>   	goto out;
>   
>   free_aead:
> @@ -2794,12 +2852,14 @@ int tls_set_sw_offload(struct sock *sk, int tx)
>   	kfree(cctx->iv);
>   	cctx->iv = NULL;
>   free_priv:
> -	if (tx) {
> -		kfree(ctx->priv_ctx_tx);
> -		ctx->priv_ctx_tx = NULL;
> -	} else {
> -		kfree(ctx->priv_ctx_rx);
> -		ctx->priv_ctx_rx = NULL;
> +	if (!new_crypto_info) {
> +		if (tx) {
> +			kfree(ctx->priv_ctx_tx);
> +			ctx->priv_ctx_tx = NULL;
> +		} else {
> +			kfree(ctx->priv_ctx_rx);
> +			ctx->priv_ctx_rx = NULL;
> +		}
>   	}
>   out:
>   	return rc;

I think we can avoid extra parameter and extra level of if{} constructions by 
checking if iv and rec_seq is already allocated and adjust init part the same 
way. I don't think we have to have separate error path because in case of any 
error during rekey procedure the connection becomes useless and application 
should indicate error to the other end. The code copies new crypto info to the 
current storage, so it assumes that all fields a properly filled and that means 
that this copy can be done earlier and use the same code path as first init code.

Re: [PATCH net-next 1/5] tls: remove tls_context argument from tls_set_sw_offload

[Vadim Fedorenko]

On 17.01.2023 13:45, Sabrina Dubroca wrote:
> It's not really needed since we end up refetching it as tls_ctx. We
> can also remove the NULL check, since we have already dereferenced ctx
> in do_tls_setsockopt_conf.
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Tested-by: Frantisek Krenzelok <fkrenzel@redhat.com>
> ---
>   net/tls/tls.h        |  2 +-
>   net/tls/tls_device.c |  2 +-
>   net/tls/tls_main.c   |  4 ++--
>   net/tls/tls_sw.c     | 11 +++--------
>   4 files changed, 7 insertions(+), 12 deletions(-)
> 
> diff --git a/net/tls/tls.h b/net/tls/tls.h
> index 0e840a0c3437..34d0fe814600 100644
> --- a/net/tls/tls.h
> +++ b/net/tls/tls.h
> @@ -90,7 +90,7 @@ int tls_sk_attach(struct sock *sk, int optname, char __user *optval,
>   		  unsigned int optlen);
>   void tls_err_abort(struct sock *sk, int err);
>   
> -int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx);
> +int tls_set_sw_offload(struct sock *sk, int tx);
>   void tls_update_rx_zc_capable(struct tls_context *tls_ctx);
>   void tls_sw_strparser_arm(struct sock *sk, struct tls_context *ctx);
>   void tls_sw_strparser_done(struct tls_context *tls_ctx);
> diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c
> index 6c593788dc25..c149f36b42ee 100644
> --- a/net/tls/tls_device.c
> +++ b/net/tls/tls_device.c
> @@ -1291,7 +1291,7 @@ int tls_set_device_offload_rx(struct sock *sk, struct tls_context *ctx)
>   	context->resync_nh_reset = 1;
>   
>   	ctx->priv_ctx_rx = context;
> -	rc = tls_set_sw_offload(sk, ctx, 0);
> +	rc = tls_set_sw_offload(sk, 0);
>   	if (rc)
>   		goto release_ctx;
>   
> diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> index 3735cb00905d..fb1da1780f50 100644
> --- a/net/tls/tls_main.c
> +++ b/net/tls/tls_main.c
> @@ -772,7 +772,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXDEVICE);
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRTXDEVICE);
>   		} else {
> -			rc = tls_set_sw_offload(sk, ctx, 1);
> +			rc = tls_set_sw_offload(sk, 1);
>   			if (rc)
>   				goto err_crypto_info;
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSTXSW);
> @@ -786,7 +786,7 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXDEVICE);
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSCURRRXDEVICE);
>   		} else {
> -			rc = tls_set_sw_offload(sk, ctx, 0);
> +			rc = tls_set_sw_offload(sk, 0);
>   			if (rc)
>   				goto err_crypto_info;
>   			TLS_INC_STATS(sock_net(sk), LINUX_MIB_TLSRXSW);
> diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c
> index 9ed978634125..238562f9081b 100644
> --- a/net/tls/tls_sw.c
> +++ b/net/tls/tls_sw.c
> @@ -2462,10 +2462,10 @@ void tls_update_rx_zc_capable(struct tls_context *tls_ctx)
>   		tls_ctx->prot_info.version != TLS_1_3_VERSION;
>   }
>   
> -int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
> +int tls_set_sw_offload(struct sock *sk, int tx)
>   {
> -	struct tls_context *tls_ctx = tls_get_ctx(sk);
> -	struct tls_prot_info *prot = &tls_ctx->prot_info;
> +	struct tls_context *ctx = tls_get_ctx(sk);
> +	struct tls_prot_info *prot = &ctx->prot_info;

nit: while you are here it's good idea to rearrange variables to follow reverse 
xmas tree rule

>   	struct tls_crypto_info *crypto_info;
>   	struct tls_sw_context_tx *sw_ctx_tx = NULL;
>   	struct tls_sw_context_rx *sw_ctx_rx = NULL;
> @@ -2477,11 +2477,6 @@ int tls_set_sw_offload(struct sock *sk, struct tls_context *ctx, int tx)
>   	size_t keysize;
>   	int rc = 0;
>   
> -	if (!ctx) {
> -		rc = -EINVAL;
> -		goto out;
> -	}
> -
>   	if (tx) {
>   		if (!ctx->priv_ctx_tx) {
>   			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);

we may consider changing tls_set_device_offload{,_rx} the same for consistency.

Reviewed-by: Vadim Fedorenko <vfedorenko@novek.ru>

Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Apoorv Kothari]

> 2023-01-17, 15:16:33 -0800, Kuniyuki Iwashima wrote:
> > Hi,
> > 
> > Thanks for posting this series!
> > We were working on the same feature.
> > CC Apoorv from s2n team.
> 
> Ah, cool. Does the behavior in those patches match what your
> implementation?

Thanks for submitting this, it looks great! We are working on testing this now.

> 
> [...]
> > > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > > index fb1da1780f50..9be82aecd13e 100644
> > > --- a/net/tls/tls_main.c
> > > +++ b/net/tls/tls_main.c
> > > @@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
> > >  static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > >  				  unsigned int optlen, int tx)
> > >  {
> > > +	union tls_crypto_context tmp = {};
> > > +	struct tls_crypto_info *old_crypto_info = NULL;
> > >  	struct tls_crypto_info *crypto_info;
> > >  	struct tls_crypto_info *alt_crypto_info;
> > >  	struct tls_context *ctx = tls_get_ctx(sk);
> > > +	bool update = false;
> > >  	size_t optsize;
> > >  	int rc = 0;
> > >  	int conf;
> > > @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > >  		alt_crypto_info = &ctx->crypto_send.info;
> > >  	}
> > >  
> > > -	/* Currently we don't support set crypto info more than one time */
> > > -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> > > -		return -EBUSY;
> > > +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> > > +		/* Currently we only support setting crypto info more
> > > +		 * than one time for TLS 1.3
> > > +		 */
> > > +		if (crypto_info->version != TLS_1_3_VERSION)
> > > +			return -EBUSY;
> > > +
> > 
> > Should we check this ?
> > 
> >                 if (!tx && !key_update_pending)
> >                         return -EBUSY;
> > 
> > Otherwise we can set a new RX key even if the other end has not sent
> > KeyUpdateRequest.
> 
> Maybe. My thinking was "let userspace shoot itself in the foot if it
> wants".

I feel avoiding foot-guns is probably the correct thing to do. The RFC also has
a requirement that re-key(process messages with new key) should only happen after
a KeyUpdate is received so it would be nice if the kTLS implemention can help
enforce this.

Based on the RFC https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3:
   Additionally, both sides MUST enforce that a KeyUpdate
   with the old key is received before accepting any messages encrypted
   with the new key.  Failure to do so may allow message truncation
   attacks.

> 
> > > +		update = true;
> > > +		old_crypto_info = crypto_info;
> > > +		crypto_info = &tmp.info;
> > > +	}
> > >  
> > >  	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
> > >  	if (rc) {
> > > @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > >  		goto err_crypto_info;
> > >  	}
> > >  
> > > +	if (update) {
> > > +		/* Ensure that TLS version and ciphers are not modified */
> > > +		if (crypto_info->version != old_crypto_info->version ||
> > > +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> > > +			rc = -EINVAL;
> > > +			goto err_crypto_info;
> > > +		}
> > > +	}
> > > +
> > >  	/* Ensure that TLS version and ciphers are same in both directions */
> > >  	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
> > 
> > We can change this to else-if.
> 
> Ok.
> 
> > >  		if (alt_crypto_info->version != crypto_info->version ||
> [...]
> > > @@ -2517,9 +2525,28 @@ int tls_set_sw_offload(struct sock *sk, int tx)
> > >  	u16 nonce_size, tag_size, iv_size, rec_seq_size, salt_size;
> > >  	struct crypto_tfm *tfm;
> > >  	char *iv, *rec_seq, *key, *salt, *cipher_name;
> > > -	size_t keysize;
> > > +	size_t keysize, crypto_info_size;
> > >  	int rc = 0;
> > >  
> > > +	if (new_crypto_info) {
> > > +		/* non-NULL new_crypto_info means rekey */
> > > +		src_crypto_info = new_crypto_info;
> > > +		if (tx) {
> > > +			sw_ctx_tx = ctx->priv_ctx_tx;
> > > +			crypto_info = &ctx->crypto_send.info;
> > > +			cctx = &ctx->tx;
> > > +			aead = &sw_ctx_tx->aead_send;
> > > +			sw_ctx_tx = NULL;
> > 
> > sw_ctx_tx is already initialised.
> 
> No, it was NULL at the beginning of the function, but then I used it
> to set aead on the previous line, so I need to clear it again. I could
> use a temp variable instead if you think it's better.
> 
> > > +		} else {
> > > +			sw_ctx_rx = ctx->priv_ctx_rx;
> > > +			crypto_info = &ctx->crypto_recv.info;
> > > +			cctx = &ctx->rx;
> > > +			aead = &sw_ctx_rx->aead_recv;
> > > +			sw_ctx_rx = NULL;
> > 
> > Same here.
> > 
> > 
> > > +		}
> > > +		goto skip_init;
> > > +	}
> > > +
> > >  	if (tx) {
> > >  		if (!ctx->priv_ctx_tx) {
> > >  			sw_ctx_tx = kzalloc(sizeof(*sw_ctx_tx), GFP_KERNEL);
> 
> Thanks for the comments.
> 
> -- 
> Sabrina

--
Apoorv

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Apoorv Kothari]

> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> > Please CC all the maintainers.
> 
> Sorry.
> 
> > On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:
> > > This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> > > [1]). A sender transmits a KeyUpdate message and then changes its TX
> > > key. The receiver should react by updating its RX key before
> > > processing the next message.
> > > 
> > > This patchset implements key updates by:
> > >  1. pausing decryption when a KeyUpdate message is received, to avoid
> > >     attempting to use the old key to decrypt a record encrypted with
> > >     the new key
> > >  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> > >     KeyUpdate message, until the rekey has been performed by userspace
> > 
> > Why? We return to user space after hitting a cmsg, don't we?
> > If the user space wants to keep reading with the old key - 🤷️
> 
> But they won't be able to read anything. Either we don't pause
> decryption, and the socket is just broken when we look at the next
> record, or we pause, and there's nothing to read until the rekey is
> done. I think that -EKEYEXPIRED is better than breaking the socket
> just because a read snuck in between getting the cmsg and setting the
> new key.

Pausing also better aligns with the RFC also since all subsequent messages
should be encrypted with the new key.

From the RFC https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3
   After sending a KeyUpdate message, the sender SHALL send all
   its traffic using the next generation of keys, computed as described
   in Section 7.2.  Upon receiving a KeyUpdate, the receiver MUST update
   its receiving keys.

> 
> > >  3. passing the KeyUpdate message to userspace as a control message
> > >  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> > >     setsockopts
> > > 
> > > This API has been tested with gnutls to make sure that it allows
> > > userspace libraries to implement key updates [2]. Thanks to Frantisek
> > > Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> > > gnutls and testing the kernel patches.
> > 
> > Please explain why - the kernel TLS is not faster than user space, 
> > the point of it is primarily to enable offload. And you don't add
> > offload support here.
> 
> Well, TLS1.3 support was added 4 years ago, and yet the offload still
> doesn't support 1.3 at all.
> 
> IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
> kernel can't claim to support 1.3, independent of offloading.
> 
> Some folks did tests with and without kTLS using nbdcopy and found a
> small but noticeable performance improvement (around 8-10%).
> 
> > > Note: in a future series, I'll clean up tls_set_sw_offload and
> > > eliminate the per-cipher copy-paste using tls_cipher_size_desc.
> > 
> > Yeah, I think it's on Vadim's TODO list as well.
> 
> I've already done most of the work as I was working on this, I'll
> submit it later.
> 
> -- 
> Sabrina

--
Apoorv

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Jakub Kicinski]

On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> > On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
> > > This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> > > [1]). A sender transmits a KeyUpdate message and then changes its TX
> > > key. The receiver should react by updating its RX key before
> > > processing the next message.
> > > 
> > > This patchset implements key updates by:
> > >  1. pausing decryption when a KeyUpdate message is received, to avoid
> > >     attempting to use the old key to decrypt a record encrypted with
> > >     the new key
> > >  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> > >     KeyUpdate message, until the rekey has been performed by userspace  
> > 
> > Why? We return to user space after hitting a cmsg, don't we?
> > If the user space wants to keep reading with the old key - 🤷️  
> 
> But they won't be able to read anything. Either we don't pause
> decryption, and the socket is just broken when we look at the next
> record, or we pause, and there's nothing to read until the rekey is
> done. I think that -EKEYEXPIRED is better than breaking the socket
> just because a read snuck in between getting the cmsg and setting the
> new key.

IDK, we don't interpret any other content types/cmsgs, and for well
behaved user space there should be no problem (right?).
I'm weakly against, if nobody agrees with me you can keep as is.

> > >  3. passing the KeyUpdate message to userspace as a control message
> > >  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> > >     setsockopts
> > > 
> > > This API has been tested with gnutls to make sure that it allows
> > > userspace libraries to implement key updates [2]. Thanks to Frantisek
> > > Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> > > gnutls and testing the kernel patches.  
> > 
> > Please explain why - the kernel TLS is not faster than user space, 
> > the point of it is primarily to enable offload. And you don't add
> > offload support here.  
> 
> Well, TLS1.3 support was added 4 years ago, and yet the offload still
> doesn't support 1.3 at all.

I'm pretty sure some devices support it. None of the vendors could 
be bothered to plumb in the kernel support, yet, tho.
I don't know of anyone supporting rekeying.

> IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
> kernel can't claim to support 1.3, independent of offloading.

The problem is that we will not be able to rekey offloaded connections.
For Tx it's a non-trivial problem given the current architecture.
The offload is supposed to be transparent, we can't fail the rekey just
because the TLS gotten offloaded.

> Some folks did tests with and without kTLS using nbdcopy and found a
> small but noticeable performance improvement (around 8-10%).

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Gal Pressman]

On 19/01/2023 4:55, Jakub Kicinski wrote:
> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
>> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
>>> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
>>>> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
>>>> [1]). A sender transmits a KeyUpdate message and then changes its TX
>>>> key. The receiver should react by updating its RX key before
>>>> processing the next message.
>>>>
>>>> This patchset implements key updates by:
>>>>  1. pausing decryption when a KeyUpdate message is received, to avoid
>>>>     attempting to use the old key to decrypt a record encrypted with
>>>>     the new key
>>>>  2. returning -EKEYEXPIRED to syscalls that cannot receive the
>>>>     KeyUpdate message, until the rekey has been performed by userspace  
>>>
>>> Why? We return to user space after hitting a cmsg, don't we?
>>> If the user space wants to keep reading with the old key - 🤷️  
>>
>> But they won't be able to read anything. Either we don't pause
>> decryption, and the socket is just broken when we look at the next
>> record, or we pause, and there's nothing to read until the rekey is
>> done. I think that -EKEYEXPIRED is better than breaking the socket
>> just because a read snuck in between getting the cmsg and setting the
>> new key.
> 
> IDK, we don't interpret any other content types/cmsgs, and for well
> behaved user space there should be no problem (right?).
> I'm weakly against, if nobody agrees with me you can keep as is.
> 
>>>>  3. passing the KeyUpdate message to userspace as a control message
>>>>  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
>>>>     setsockopts
>>>>
>>>> This API has been tested with gnutls to make sure that it allows
>>>> userspace libraries to implement key updates [2]. Thanks to Frantisek
>>>> Krenzelok <fkrenzel@redhat.com> for providing the implementation in
>>>> gnutls and testing the kernel patches.  
>>>
>>> Please explain why - the kernel TLS is not faster than user space, 
>>> the point of it is primarily to enable offload. And you don't add
>>> offload support here.  
>>
>> Well, TLS1.3 support was added 4 years ago, and yet the offload still
>> doesn't support 1.3 at all.
> 
> I'm pretty sure some devices support it. None of the vendors could 
> be bothered to plumb in the kernel support, yet, tho.

Our device supports TLS 1.3, it's in our plans to add driver/kernel support.

> I don't know of anyone supporting rekeying.

Boris, Tariq, do you know?

Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Sabrina Dubroca]

2023-01-18, 23:10:18 +0000, Vadim Fedorenko wrote:
> On 17.01.2023 13:45, Sabrina Dubroca wrote:
> > @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >   		alt_crypto_info = &ctx->crypto_send.info;
> >   	}
> > -	/* Currently we don't support set crypto info more than one time */
> > -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> > -		return -EBUSY;
> > +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> > +		/* Currently we only support setting crypto info more
> > +		 * than one time for TLS 1.3
> > +		 */
> > +		if (crypto_info->version != TLS_1_3_VERSION)
> > +			return -EBUSY;
> > +
> > +		update = true;
> > +		old_crypto_info = crypto_info;
> > +		crypto_info = &tmp.info;
> > +	}
> >   	rc = copy_from_sockptr(crypto_info, optval, sizeof(*crypto_info));
> >   	if (rc) {
> > @@ -704,6 +715,15 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> >   		goto err_crypto_info;
> >   	}
> > +	if (update) {
> > +		/* Ensure that TLS version and ciphers are not modified */
> > +		if (crypto_info->version != old_crypto_info->version ||
> > +		    crypto_info->cipher_type != old_crypto_info->cipher_type) {
> > +			rc = -EINVAL;
> > +			goto err_crypto_info;
> > +		}
> > +	}
> > +
> 
> looks like these checks can be moved up to TLS_CRYPTO_INFO_READY scope and
> there will be no need for extra variables.

I don't see how to do that cleanly. I'd have to duplicate the
copy_from_sockptr, which IMHO looks a lot worse. Is there another way?

> >   	/* Ensure that TLS version and ciphers are same in both directions */
> >   	if (TLS_CRYPTO_INFO_READY(alt_crypto_info)) {
> >   		if (alt_crypto_info->version != crypto_info->version ||

[...]
> > @@ -2794,12 +2852,14 @@ int tls_set_sw_offload(struct sock *sk, int tx)
> >   	kfree(cctx->iv);
> >   	cctx->iv = NULL;
> >   free_priv:
> > -	if (tx) {
> > -		kfree(ctx->priv_ctx_tx);
> > -		ctx->priv_ctx_tx = NULL;
> > -	} else {
> > -		kfree(ctx->priv_ctx_rx);
> > -		ctx->priv_ctx_rx = NULL;
> > +	if (!new_crypto_info) {
> > +		if (tx) {
> > +			kfree(ctx->priv_ctx_tx);
> > +			ctx->priv_ctx_tx = NULL;
> > +		} else {
> > +			kfree(ctx->priv_ctx_rx);
> > +			ctx->priv_ctx_rx = NULL;
> > +		}
> >   	}
> >   out:
> >   	return rc;
> 
> I think we can avoid extra parameter and extra level of if{} constructions
> by checking if iv and rec_seq is already allocated and adjust init part the
> same way. I don't think we have to have separate error path because in case
> of any error during rekey procedure the connection becomes useless and
> application should indicate error to the other end. The code copies new
> crypto info to the current storage, so it assumes that all fields a properly
> filled and that means that this copy can be done earlier and use the same
> code path as first init code.

Rekey could fail because of memory allocation failures during
crypto_aead_setkey. Userspace could choose to retry the key update,
and we shouldn't necessarily kill off the connection in that case.

I think we need to keep the init/update distinction in the error paths
for tls_set_sw_offload and do_tls_setsockopt_conf, otherwise we clear
the crypto_info from the context and a new attempt to do the rekey
will run through the full init path instead of the rekey path.

We could set crypto_info in tls_context before calling
tls_set_sw_offload, but do_tls_setsockopt_conf would still have some
differences since we need to validate that the version/cipher hasn't
changed. I'll give that a try and see how much that improves
things. It should reduce the churn a bit.

Thanks

-- 
Sabrina

Re: [PATCH net-next 3/5] tls: implement rekey for TLS1.3

[Sabrina Dubroca]

2023-01-18, 17:25:46 -0800, Apoorv Kothari wrote:
> > 2023-01-17, 15:16:33 -0800, Kuniyuki Iwashima wrote:
> > > Hi,
> > > 
> > > Thanks for posting this series!
> > > We were working on the same feature.
> > > CC Apoorv from s2n team.
> > 
> > Ah, cool. Does the behavior in those patches match what your
> > implementation?
> 
> Thanks for submitting this, it looks great! We are working on testing this now.
> 
> > 
> > [...]
> > > > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c
> > > > index fb1da1780f50..9be82aecd13e 100644
> > > > --- a/net/tls/tls_main.c
> > > > +++ b/net/tls/tls_main.c
> > > > @@ -669,9 +669,12 @@ static int tls_getsockopt(struct sock *sk, int level, int optname,
> > > >  static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > > >  				  unsigned int optlen, int tx)
> > > >  {
> > > > +	union tls_crypto_context tmp = {};
> > > > +	struct tls_crypto_info *old_crypto_info = NULL;
> > > >  	struct tls_crypto_info *crypto_info;
> > > >  	struct tls_crypto_info *alt_crypto_info;
> > > >  	struct tls_context *ctx = tls_get_ctx(sk);
> > > > +	bool update = false;
> > > >  	size_t optsize;
> > > >  	int rc = 0;
> > > >  	int conf;
> > > > @@ -687,9 +690,17 @@ static int do_tls_setsockopt_conf(struct sock *sk, sockptr_t optval,
> > > >  		alt_crypto_info = &ctx->crypto_send.info;
> > > >  	}
> > > >  
> > > > -	/* Currently we don't support set crypto info more than one time */
> > > > -	if (TLS_CRYPTO_INFO_READY(crypto_info))
> > > > -		return -EBUSY;
> > > > +	if (TLS_CRYPTO_INFO_READY(crypto_info)) {
> > > > +		/* Currently we only support setting crypto info more
> > > > +		 * than one time for TLS 1.3
> > > > +		 */
> > > > +		if (crypto_info->version != TLS_1_3_VERSION)
> > > > +			return -EBUSY;
> > > > +
> > > 
> > > Should we check this ?
> > > 
> > >                 if (!tx && !key_update_pending)
> > >                         return -EBUSY;
> > > 
> > > Otherwise we can set a new RX key even if the other end has not sent
> > > KeyUpdateRequest.
> > 
> > Maybe. My thinking was "let userspace shoot itself in the foot if it
> > wants".
> 
> I feel avoiding foot-guns is probably the correct thing to do. The RFC also has
> a requirement that re-key(process messages with new key) should only happen after
> a KeyUpdate is received so it would be nice if the kTLS implemention can help
> enforce this.
> 
> Based on the RFC https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3:
>    Additionally, both sides MUST enforce that a KeyUpdate
>    with the old key is received before accepting any messages encrypted
>    with the new key.  Failure to do so may allow message truncation
>    attacks.

Ok. I'll add that in v2, unless someone is strongly against it.

Thanks.

-- 
Sabrina

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Sabrina Dubroca]

2023-01-18, 18:55:22 -0800, Jakub Kicinski wrote:
> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
> > 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> > > On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
> > > > This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> > > > [1]). A sender transmits a KeyUpdate message and then changes its TX
> > > > key. The receiver should react by updating its RX key before
> > > > processing the next message.
> > > > 
> > > > This patchset implements key updates by:
> > > >  1. pausing decryption when a KeyUpdate message is received, to avoid
> > > >     attempting to use the old key to decrypt a record encrypted with
> > > >     the new key
> > > >  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> > > >     KeyUpdate message, until the rekey has been performed by userspace  
> > > 
> > > Why? We return to user space after hitting a cmsg, don't we?
> > > If the user space wants to keep reading with the old key - 🤷️  
> > 
> > But they won't be able to read anything. Either we don't pause
> > decryption, and the socket is just broken when we look at the next
> > record, or we pause, and there's nothing to read until the rekey is
> > done. I think that -EKEYEXPIRED is better than breaking the socket
> > just because a read snuck in between getting the cmsg and setting the
> > new key.
> 
> IDK, we don't interpret any other content types/cmsgs, and for well
> behaved user space there should be no problem (right?).
> I'm weakly against, if nobody agrees with me you can keep as is.

I was concerned (I don't know if it's realistic) about a userspace
application with two threads:


  Thread A            Thread B
  --------            --------

  read cmsg

                      read some data (still on the old key)

  sets the new key


I guess one could claim that's a userspace bug.

František's implementation in gnutls doesn't seem to rely on this.

Apoorv, since you were also looking into key updates, do you have an
opinion on pausing decryption/reads until userspace has provides the
new key?

> > > >  3. passing the KeyUpdate message to userspace as a control message
> > > >  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> > > >     setsockopts
> > > > 
> > > > This API has been tested with gnutls to make sure that it allows
> > > > userspace libraries to implement key updates [2]. Thanks to Frantisek
> > > > Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> > > > gnutls and testing the kernel patches.  
> > > 
> > > Please explain why - the kernel TLS is not faster than user space, 
> > > the point of it is primarily to enable offload. And you don't add
> > > offload support here.  
> > 
> > Well, TLS1.3 support was added 4 years ago, and yet the offload still
> > doesn't support 1.3 at all.
> 
> I'm pretty sure some devices support it. None of the vendors could 
> be bothered to plumb in the kernel support, yet, tho.
> I don't know of anyone supporting rekeying.
>
> > IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
> > kernel can't claim to support 1.3, independent of offloading.
> 
> The problem is that we will not be able to rekey offloaded connections.
> For Tx it's a non-trivial problem given the current architecture.
> The offload is supposed to be transparent, we can't fail the rekey just
> because the TLS gotten offloaded.

What's their plan when the peer sends a KeyUpdate request then? Let
the connection break?

-- 
Sabrina

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Jakub Kicinski]

On Thu, 19 Jan 2023 16:40:39 +0100 Sabrina Dubroca wrote:
> > > IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
> > > kernel can't claim to support 1.3, independent of offloading.  
> > 
> > The problem is that we will not be able to rekey offloaded connections.
> > For Tx it's a non-trivial problem given the current architecture.
> > The offload is supposed to be transparent, we can't fail the rekey just
> > because the TLS gotten offloaded.  
> 
> What's their plan when the peer sends a KeyUpdate request then? Let
> the connection break?

I believe so, yes, just open a new connection. TLS rekeying seems 
to be extremely rare.

You mentioned nbd as a potential use case for kernel SW implementation.
Can nbd rekey? Is use space responding to control messages in case of
nbd?

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Apoorv Kothari]

> 2023-01-18, 18:55:22 -0800, Jakub Kicinski wrote:
> > On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
> > > 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> > > > On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
> > > > > This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> > > > > [1]). A sender transmits a KeyUpdate message and then changes its TX
> > > > > key. The receiver should react by updating its RX key before
> > > > > processing the next message.
> > > > > 
> > > > > This patchset implements key updates by:
> > > > >  1. pausing decryption when a KeyUpdate message is received, to avoid
> > > > >     attempting to use the old key to decrypt a record encrypted with
> > > > >     the new key
> > > > >  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> > > > >     KeyUpdate message, until the rekey has been performed by userspace  
> > > > 
> > > > Why? We return to user space after hitting a cmsg, don't we?
> > > > If the user space wants to keep reading with the old key - 🤷️  
> > > 
> > > But they won't be able to read anything. Either we don't pause
> > > decryption, and the socket is just broken when we look at the next
> > > record, or we pause, and there's nothing to read until the rekey is
> > > done. I think that -EKEYEXPIRED is better than breaking the socket
> > > just because a read snuck in between getting the cmsg and setting the
> > > new key.
> > 
> > IDK, we don't interpret any other content types/cmsgs, and for well
> > behaved user space there should be no problem (right?).
> > I'm weakly against, if nobody agrees with me you can keep as is.
> 
> I was concerned (I don't know if it's realistic) about a userspace
> application with two threads:
> 
> 
>   Thread A            Thread B
>   --------            --------
> 
>   read cmsg
> 
>                       read some data (still on the old key)
> 
>   sets the new key
> 
> 
> I guess one could claim that's a userspace bug.
> 
> František's implementation in gnutls doesn't seem to rely on this.
> 
> Apoorv, since you were also looking into key updates, do you have an
> opinion on pausing decryption/reads until userspace has provides the
> new key?
> 

There are a few reason I can think of why we would want the pausing behavior.

0) If possible, we should enforce correctness and prevent the userspace from
doing something bad.

1) Pausing better aligns with the RFC since all subsequent messages
should be encrypted with the new key.

From the RFC https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3
   After sending a KeyUpdate message, the sender SHALL send all
   its traffic using the next generation of keys, computed as described
   in Section 7.2.  Upon receiving a KeyUpdate, the receiver MUST update
   its receiving keys.

2) Its possible to receive multiple KeyUpdate messages. If we allow the
userspace to read more records, they would read multiple KeyUpdate messages.
However since we currently track `bool key_update_pending;` using a bool,
we would only require a single rekey and end up in a bad state. Its possible
to fix this by not using bool but then the logic get more complicate and not
worth it IMO.

> > > > >  3. passing the KeyUpdate message to userspace as a control message
> > > > >  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> > > > >     setsockopts
> > > > > 
> > > > > This API has been tested with gnutls to make sure that it allows
> > > > > userspace libraries to implement key updates [2]. Thanks to Frantisek
> > > > > Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> > > > > gnutls and testing the kernel patches.  
> > > > 
> > > > Please explain why - the kernel TLS is not faster than user space, 
> > > > the point of it is primarily to enable offload. And you don't add
> > > > offload support here.  
> > > 
> > > Well, TLS1.3 support was added 4 years ago, and yet the offload still
> > > doesn't support 1.3 at all.
> > 
> > I'm pretty sure some devices support it. None of the vendors could 
> > be bothered to plumb in the kernel support, yet, tho.
> > I don't know of anyone supporting rekeying.
> >
> > > IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
> > > kernel can't claim to support 1.3, independent of offloading.
> > 
> > The problem is that we will not be able to rekey offloaded connections.
> > For Tx it's a non-trivial problem given the current architecture.
> > The offload is supposed to be transparent, we can't fail the rekey just
> > because the TLS gotten offloaded.
> 
> What's their plan when the peer sends a KeyUpdate request then? Let
> the connection break?
> 
> -- 
> Sabrina

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Vadim Fedorenko]

On 19.01.2023 02:55, Jakub Kicinski wrote:
> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
>> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
>>> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:
>>>> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
>>>> [1]). A sender transmits a KeyUpdate message and then changes its TX
>>>> key. The receiver should react by updating its RX key before
>>>> processing the next message.
>>>>
>>>> This patchset implements key updates by:
>>>>   1. pausing decryption when a KeyUpdate message is received, to avoid
>>>>      attempting to use the old key to decrypt a record encrypted with
>>>>      the new key
>>>>   2. returning -EKEYEXPIRED to syscalls that cannot receive the
>>>>      KeyUpdate message, until the rekey has been performed by userspace
>>>
>>> Why? We return to user space after hitting a cmsg, don't we?
>>> If the user space wants to keep reading with the old key - 🤷️
>>
>> But they won't be able to read anything. Either we don't pause
>> decryption, and the socket is just broken when we look at the next
>> record, or we pause, and there's nothing to read until the rekey is
>> done. I think that -EKEYEXPIRED is better than breaking the socket
>> just because a read snuck in between getting the cmsg and setting the
>> new key.
> 
> IDK, we don't interpret any other content types/cmsgs, and for well
> behaved user space there should be no problem (right?).
> I'm weakly against, if nobody agrees with me you can keep as is.
> 
>>>>   3. passing the KeyUpdate message to userspace as a control message
>>>>   4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
>>>>      setsockopts
>>>>
>>>> This API has been tested with gnutls to make sure that it allows
>>>> userspace libraries to implement key updates [2]. Thanks to Frantisek
>>>> Krenzelok <fkrenzel@redhat.com> for providing the implementation in
>>>> gnutls and testing the kernel patches.
>>>
>>> Please explain why - the kernel TLS is not faster than user space,
>>> the point of it is primarily to enable offload. And you don't add
>>> offload support here.
>>
>> Well, TLS1.3 support was added 4 years ago, and yet the offload still
>> doesn't support 1.3 at all.
> 
> I'm pretty sure some devices support it. None of the vendors could
> be bothered to plumb in the kernel support, yet, tho.
> I don't know of anyone supporting rekeying.
> 
>> IIRC support for KeyUpdates is mandatory in TLS1.3, so currently the
>> kernel can't claim to support 1.3, independent of offloading.
> 
> The problem is that we will not be able to rekey offloaded connections.
> For Tx it's a non-trivial problem given the current architecture.
> The offload is supposed to be transparent, we can't fail the rekey just
> because the TLS gotten offloaded.

But we really don't have any HW offload implementation for TLSv1.3. But it would 
be great to have SW implementation better align to RFC.

>> Some folks did tests with and without kTLS using nbdcopy and found a
>> small but noticeable performance improvement (around 8-10%).

[PATCH net-next 5/5] selftests: tls: add rekey tests

[Apoorv Kothari]

> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> ---
>  tools/testing/selftests/net/tls.c | 258 ++++++++++++++++++++++++++++++
>  1 file changed, 258 insertions(+)
> 
> diff --git a/tools/testing/selftests/net/tls.c b/tools/testing/selftests/net/tls.c
> index 5f3adb28fee1..97c20e2246e1 100644
> --- a/tools/testing/selftests/net/tls.c
> +++ b/tools/testing/selftests/net/tls.c
> @@ -1453,6 +1453,264 @@ TEST_F(tls, shutdown_reuse)
>      EXPECT_EQ(errno, EISCONN);
>  }
>  
> +#define TLS_RECORD_TYPE_HANDSHAKE      0x16
> +/* key_update, length 1, update_not_requested */
> +static const char key_update_msg[] = "\x18\x00\x00\x01\x00";
> +static void tls_send_keyupdate(struct __test_metadata *_metadata, int fd)
> +{
> +    size_t len = sizeof(key_update_msg);
> +
> +    EXPECT_EQ(tls_send_cmsg(fd, TLS_RECORD_TYPE_HANDSHAKE,
> +                (char *)key_update_msg, len, 0),
> +          len);
> +}
> +
> +static void tls_recv_keyupdate(struct __test_metadata *_metadata, int fd, int flags)
> +{
> +    char buf[100];
> +
> +    EXPECT_EQ(tls_recv_cmsg(_metadata, fd, TLS_RECORD_TYPE_HANDSHAKE, buf, sizeof(buf), flags),
> +          sizeof(key_update_msg));
> +    EXPECT_EQ(memcmp(buf, key_update_msg, sizeof(key_update_msg)), 0);
> +}
> +
> +/* set the key to 0 then 1 for RX, immediately to 1 for TX */
> +TEST_F(tls_basic, rekey_rx)
> +{
> +    struct tls_crypto_info_keys tls12_0, tls12_1;

nit: Did you mean tls13_0 and tls13_1? There are also a few others in this patch.

> +    char const *test_str = "test_message";
> +    int send_len = strlen(test_str) + 1;
> +    char buf[20];
> +    int ret;
> +
> +    if (self->notls)
> +        return;
> +
> +    tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
> +                 &tls12_0, 0);
> +    tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
> +                 &tls12_1, 1);
> +
> +
> +    ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_1, tls12_1.len);
> +    ASSERT_EQ(ret, 0);
> +
> +    ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_0, tls12_0.len);
> +    ASSERT_EQ(ret, 0);
> +
> +    ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_1, tls12_1.len);
> +    EXPECT_EQ(ret, 0);
> +
> +    EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
> +    EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
> +    EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
> +}
> +
> +/* set the key to 0 then 1 for TX, immediately to 1 for RX */
> +TEST_F(tls_basic, rekey_tx)
> +{
> +    struct tls_crypto_info_keys tls12_0, tls12_1;
> +    char const *test_str = "test_message";
> +    int send_len = strlen(test_str) + 1;
> +    char buf[20];
> +    int ret;
> +
> +    if (self->notls)
> +        return;
> +
> +    tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
> +                 &tls12_0, 0);
> +    tls_crypto_info_init(TLS_1_3_VERSION, TLS_CIPHER_AES_GCM_128,
> +                 &tls12_1, 1);
> +
> +
> +    ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_0, tls12_0.len);
> +    ASSERT_EQ(ret, 0);
> +
> +    ret = setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_1, tls12_1.len);
> +    ASSERT_EQ(ret, 0);
> +
> +    ret = setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_1, tls12_1.len);
> +    EXPECT_EQ(ret, 0);
> +
> +    EXPECT_EQ(send(self->fd, test_str, send_len, 0), send_len);
> +    EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
> +    EXPECT_EQ(memcmp(buf, test_str, send_len), 0);
> +}
> +
> +TEST_F(tls, rekey)
> +{
> +    char const *test_str_1 = "test_message_before_rekey";
> +    char const *test_str_2 = "test_message_after_rekey";
> +    struct tls_crypto_info_keys tls12;
> +    int send_len;
> +    char buf[100];
> +
> +    if (variant->tls_version != TLS_1_3_VERSION)
> +        return;
> +
> +    /* initial send/recv */
> +    send_len = strlen(test_str_1) + 1;
> +    EXPECT_EQ(send(self->fd, test_str_1, send_len, 0), send_len);
> +    EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
> +    EXPECT_EQ(memcmp(buf, test_str_1, send_len), 0);
> +
> +    /* update TX key */
> +    tls_send_keyupdate(_metadata, self->fd);
> +    tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
> +    EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
> +
> +    /* send after rekey */
> +    send_len = strlen(test_str_2) + 1;
> +    EXPECT_EQ(send(self->fd, test_str_2, send_len, 0), send_len);
> +
> +    /* can't receive the KeyUpdate without a control message */
> +    EXPECT_EQ(recv(self->cfd, buf, send_len, 0), -1);
> +
> +    /* get KeyUpdate */
> +    tls_recv_keyupdate(_metadata, self->cfd, 0);
> +
> +    /* recv blocking -> -EKEYEXPIRED */
> +    EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), 0), -1);
> +    EXPECT_EQ(errno, EKEYEXPIRED);
> +
> +    /* recv non-blocking -> -EKEYEXPIRED */
> +    EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_DONTWAIT), -1);
> +    EXPECT_EQ(errno, EKEYEXPIRED);
> +
> +    /* update RX key */
> +    EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
> +
> +    /* recv after rekey */
> +    EXPECT_NE(recv(self->cfd, buf, send_len, 0), -1);
> +    EXPECT_EQ(memcmp(buf, test_str_2, send_len), 0);
> +}
> +
> +TEST_F(tls, rekey_peek)
> +{
> +    char const *test_str_1 = "test_message_before_rekey";
> +    struct tls_crypto_info_keys tls12;
> +    int send_len;
> +    char buf[100];
> +
> +    if (variant->tls_version != TLS_1_3_VERSION)
> +        return;
> +
> +    send_len = strlen(test_str_1) + 1;
> +    EXPECT_EQ(send(self->fd, test_str_1, send_len, 0), send_len);
> +
> +    /* update TX key */
> +    tls_send_keyupdate(_metadata, self->fd);
> +    tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
> +    EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
> +
> +    EXPECT_EQ(recv(self->cfd, buf, sizeof(buf), MSG_PEEK), send_len);
> +    EXPECT_EQ(memcmp(buf, test_str_1, send_len), 0);
> +
> +    EXPECT_EQ(recv(self->cfd, buf, send_len, 0), send_len);
> +    EXPECT_EQ(memcmp(buf, test_str_1, send_len), 0);
> +
> +    /* can't receive the KeyUpdate without a control message */
> +    EXPECT_EQ(recv(self->cfd, buf, send_len, MSG_PEEK), -1);
> +
> +    /* peek KeyUpdate */
> +    tls_recv_keyupdate(_metadata, self->cfd, MSG_PEEK);
> +
> +    /* get KeyUpdate */
> +    tls_recv_keyupdate(_metadata, self->cfd, 0);
> +
> +    /* update RX key */
> +    EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
> +}
> +
> +TEST_F(tls, splice_rekey)
> +{
> +    int send_len = TLS_PAYLOAD_MAX_LEN / 2;
> +    char mem_send[TLS_PAYLOAD_MAX_LEN];
> +    char mem_recv[TLS_PAYLOAD_MAX_LEN];
> +    struct tls_crypto_info_keys tls12;
> +    int p[2];
> +
> +    if (variant->tls_version != TLS_1_3_VERSION)
> +        return;
> +
> +    memrnd(mem_send, sizeof(mem_send));
> +
> +    ASSERT_GE(pipe(p), 0);
> +    EXPECT_EQ(send(self->fd, mem_send, send_len, 0), send_len);
> +
> +    /* update TX key */
> +    tls_send_keyupdate(_metadata, self->fd);
> +    tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
> +    EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
> +
> +    EXPECT_EQ(send(self->fd, mem_send, send_len, 0), send_len);
> +
> +    EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), send_len);
> +    EXPECT_EQ(read(p[0], mem_recv, send_len), send_len);
> +    EXPECT_EQ(memcmp(mem_send, mem_recv, send_len), 0);
> +
> +    /* can't splice the KeyUpdate */
> +    EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), -1);
> +    EXPECT_EQ(errno, EINVAL);
> +
> +    /* peek KeyUpdate */
> +    tls_recv_keyupdate(_metadata, self->cfd, MSG_PEEK);
> +
> +    /* get KeyUpdate */
> +    tls_recv_keyupdate(_metadata, self->cfd, 0);
> +
> +    /* can't splice before updating the key */
> +    EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), -1);
> +    EXPECT_EQ(errno, EKEYEXPIRED);
> +
> +    /* update RX key */
> +    EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
> +
> +    EXPECT_EQ(splice(self->cfd, NULL, p[1], NULL, TLS_PAYLOAD_MAX_LEN, 0), send_len);
> +    EXPECT_EQ(read(p[0], mem_recv, send_len), send_len);
> +    EXPECT_EQ(memcmp(mem_send, mem_recv, send_len), 0);
> +}
> +
> +TEST_F(tls, rekey_getsockopt)
> +{
> +    struct tls_crypto_info_keys tls12;
> +    struct tls_crypto_info_keys tls12_get;
> +    socklen_t len;
> +
> +    tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 0);
> +
> +    len = tls12.len;
> +    EXPECT_EQ(getsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_get, &len), 0);
> +    EXPECT_EQ(len, tls12.len);
> +    EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
> +
> +    len = tls12.len;
> +    EXPECT_EQ(getsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_get, &len), 0);
> +    EXPECT_EQ(len, tls12.len);
> +    EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
> +
> +    if (variant->tls_version != TLS_1_3_VERSION)
> +        return;
> +
> +    tls_send_keyupdate(_metadata, self->fd);
> +    tls_crypto_info_init(variant->tls_version, variant->cipher_type, &tls12, 1);
> +    EXPECT_EQ(setsockopt(self->fd, SOL_TLS, TLS_TX, &tls12, tls12.len), 0);
> +
> +    tls_recv_keyupdate(_metadata, self->cfd, 0);
> +    EXPECT_EQ(setsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12, tls12.len), 0);
> +
> +    len = tls12.len;
> +    EXPECT_EQ(getsockopt(self->fd, SOL_TLS, TLS_TX, &tls12_get, &len), 0);
> +    EXPECT_EQ(len, tls12.len);
> +    EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
> +
> +    len = tls12.len;
> +    EXPECT_EQ(getsockopt(self->cfd, SOL_TLS, TLS_RX, &tls12_get, &len), 0);
> +    EXPECT_EQ(len, tls12.len);
> +    EXPECT_EQ(memcmp(&tls12_get, &tls12, tls12.len), 0);
> +}
> +
>  FIXTURE(tls_err)
>  {
>      int fd, cfd;
> -- 
> 2.38.1

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Boris Pismenny]

On 19/01/2023 10:27, Gal Pressman wrote:
> On 19/01/2023 4:55, Jakub Kicinski wrote:
>> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
>>> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
>>>> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
>>>>> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
>>>>> [1]). A sender transmits a KeyUpdate message and then changes its TX
>>>>> key. The receiver should react by updating its RX key before
>>>>> processing the next message.
>>>>>
>>>>> This patchset implements key updates by:
>>>>>  1. pausing decryption when a KeyUpdate message is received, to avoid
>>>>>     attempting to use the old key to decrypt a record encrypted with
>>>>>     the new key
>>>>>  2. returning -EKEYEXPIRED to syscalls that cannot receive the
>>>>>     KeyUpdate message, until the rekey has been performed by userspace  
>>>>
>>>> Why? We return to user space after hitting a cmsg, don't we?
>>>> If the user space wants to keep reading with the old key - 🤷️  
>>>
>>> But they won't be able to read anything. Either we don't pause
>>> decryption, and the socket is just broken when we look at the next
>>> record, or we pause, and there's nothing to read until the rekey is
>>> done. I think that -EKEYEXPIRED is better than breaking the socket
>>> just because a read snuck in between getting the cmsg and setting the
>>> new key.
>>
>> IDK, we don't interpret any other content types/cmsgs, and for well
>> behaved user space there should be no problem (right?).
>> I'm weakly against, if nobody agrees with me you can keep as is.
>>
>>>>>  3. passing the KeyUpdate message to userspace as a control message
>>>>>  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
>>>>>     setsockopts
>>>>>
>>>>> This API has been tested with gnutls to make sure that it allows
>>>>> userspace libraries to implement key updates [2]. Thanks to Frantisek
>>>>> Krenzelok <fkrenzel@redhat.com> for providing the implementation in
>>>>> gnutls and testing the kernel patches.  
>>>>
>>>> Please explain why - the kernel TLS is not faster than user space, 
>>>> the point of it is primarily to enable offload. And you don't add
>>>> offload support here.  
>>>
>>> Well, TLS1.3 support was added 4 years ago, and yet the offload still
>>> doesn't support 1.3 at all.
>>
>> I'm pretty sure some devices support it. None of the vendors could 
>> be bothered to plumb in the kernel support, yet, tho.
> 
> Our device supports TLS 1.3, it's in our plans to add driver/kernel support.
> 
>> I don't know of anyone supporting rekeying.
> 
> Boris, Tariq, do you know?

Rekeying is not trivial to get right with offload. There are at least
two problems to solve:
1. On transmit, we need to handle both the new and the old key for new
and old (retransmitted) data, respectively. Our device will be able to
hold both keys in parallel and to choose the right one at the cost of an
if statement in the data-path. Alternatively, we can just fallback to
software for the old key and focus on the new key.
2. On Rx, packets with the new key may arrive before the key is
installed unless we design a mechanism for preemptively setting the next
key in HW. As a result, we may get a resync on every rekey.

Have you considered an API to preemptively set the next key in the
kernel such that there is never a need to stop the datapath? I think
that the change in SSL libraries is minor and it can really help KTLS.

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Sabrina Dubroca]

(adding Simo and Daiki for the OpenSSL/GnuTLS sides)

2023-01-23, 10:13:58 +0000, Boris Pismenny wrote:
> On 19/01/2023 10:27, Gal Pressman wrote:
> > On 19/01/2023 4:55, Jakub Kicinski wrote:
> >> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
> >>> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> >>>> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
> >>>>> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> >>>>> [1]). A sender transmits a KeyUpdate message and then changes its TX
> >>>>> key. The receiver should react by updating its RX key before
> >>>>> processing the next message.
> >>>>>
> >>>>> This patchset implements key updates by:
> >>>>>  1. pausing decryption when a KeyUpdate message is received, to avoid
> >>>>>     attempting to use the old key to decrypt a record encrypted with
> >>>>>     the new key
> >>>>>  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> >>>>>     KeyUpdate message, until the rekey has been performed by userspace  
> >>>>
> >>>> Why? We return to user space after hitting a cmsg, don't we?
> >>>> If the user space wants to keep reading with the old key - 🤷️  
> >>>
> >>> But they won't be able to read anything. Either we don't pause
> >>> decryption, and the socket is just broken when we look at the next
> >>> record, or we pause, and there's nothing to read until the rekey is
> >>> done. I think that -EKEYEXPIRED is better than breaking the socket
> >>> just because a read snuck in between getting the cmsg and setting the
> >>> new key.
> >>
> >> IDK, we don't interpret any other content types/cmsgs, and for well
> >> behaved user space there should be no problem (right?).
> >> I'm weakly against, if nobody agrees with me you can keep as is.
> >>
> >>>>>  3. passing the KeyUpdate message to userspace as a control message
> >>>>>  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> >>>>>     setsockopts
> >>>>>
> >>>>> This API has been tested with gnutls to make sure that it allows
> >>>>> userspace libraries to implement key updates [2]. Thanks to Frantisek
> >>>>> Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> >>>>> gnutls and testing the kernel patches.  
> >>>>
> >>>> Please explain why - the kernel TLS is not faster than user space, 
> >>>> the point of it is primarily to enable offload. And you don't add
> >>>> offload support here.  
> >>>
> >>> Well, TLS1.3 support was added 4 years ago, and yet the offload still
> >>> doesn't support 1.3 at all.
> >>
> >> I'm pretty sure some devices support it. None of the vendors could 
> >> be bothered to plumb in the kernel support, yet, tho.
> > 
> > Our device supports TLS 1.3, it's in our plans to add driver/kernel support.
> > 
> >> I don't know of anyone supporting rekeying.
> > 
> > Boris, Tariq, do you know?
> 
> Rekeying is not trivial to get right with offload. There are at least
> two problems to solve:
> 1. On transmit, we need to handle both the new and the old key for new
> and old (retransmitted) data, respectively. Our device will be able to
> hold both keys in parallel and to choose the right one at the cost of an
> if statement in the data-path. Alternatively, we can just fallback to
> software for the old key and focus on the new key.

We'll need to keep the old key around until we know all the records
using it have been fully received, right?  And that could be multiple
old keys, in case of a quick series of key updates.

> 2. On Rx, packets with the new key may arrive before the key is
> installed unless we design a mechanism for preemptively setting the next
> key in HW. As a result, we may get a resync on every rekey.
> 
> Have you considered an API to preemptively set the next key in the
> kernel such that there is never a need to stop the datapath? I think
> that the change in SSL libraries is minor and it can really help KTLS.

I don't like the idea of having some unused key stored in the kernel
for long durations too much.

You can't be sure that there will be only one rekey in the next short
interval, so you'll need to be able to handle those resyncs anyway, in
case userspace is too slow providing the 3rd key (for the 2nd
rekey). For example, the RFC mentions:

   If implementations independently send their own KeyUpdates with
   request_update set to "update_requested" and they cross in flight,
   then each side will also send a response, with the result that each
   side increments by two generations.

   https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3

So I think what you're suggesting can only be an optimization,
not a full solution.

I hope I'm not getting too confused by all this.

-- 
Sabrina

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Apoorv Kothari]

> 2023-01-23, 10:13:58 +0000, Boris Pismenny wrote:
> > On 19/01/2023 10:27, Gal Pressman wrote:
> > > On 19/01/2023 4:55, Jakub Kicinski wrote:
> > >> On Wed, 18 Jan 2023 11:06:25 +0100 Sabrina Dubroca wrote:
> > >>> 2023-01-17, 18:03:51 -0800, Jakub Kicinski wrote:
> > >>>> On Tue, 17 Jan 2023 14:45:26 +0100 Sabrina Dubroca wrote:  
> > >>>>> This adds support for receiving KeyUpdate messages (RFC 8446, 4.6.3
> > >>>>> [1]). A sender transmits a KeyUpdate message and then changes its TX
> > >>>>> key. The receiver should react by updating its RX key before
> > >>>>> processing the next message.
> > >>>>>
> > >>>>> This patchset implements key updates by:
> > >>>>>  1. pausing decryption when a KeyUpdate message is received, to avoid
> > >>>>>     attempting to use the old key to decrypt a record encrypted with
> > >>>>>     the new key
> > >>>>>  2. returning -EKEYEXPIRED to syscalls that cannot receive the
> > >>>>>     KeyUpdate message, until the rekey has been performed by userspace  
> > >>>>
> > >>>> Why? We return to user space after hitting a cmsg, don't we?
> > >>>> If the user space wants to keep reading with the old key - 🤷️  
> > >>>
> > >>> But they won't be able to read anything. Either we don't pause
> > >>> decryption, and the socket is just broken when we look at the next
> > >>> record, or we pause, and there's nothing to read until the rekey is
> > >>> done. I think that -EKEYEXPIRED is better than breaking the socket
> > >>> just because a read snuck in between getting the cmsg and setting the
> > >>> new key.
> > >>
> > >> IDK, we don't interpret any other content types/cmsgs, and for well
> > >> behaved user space there should be no problem (right?).
> > >> I'm weakly against, if nobody agrees with me you can keep as is.
> > >>
> > >>>>>  3. passing the KeyUpdate message to userspace as a control message
> > >>>>>  4. allowing updates of the crypto_info via the TLS_TX/TLS_RX
> > >>>>>     setsockopts
> > >>>>>
> > >>>>> This API has been tested with gnutls to make sure that it allows
> > >>>>> userspace libraries to implement key updates [2]. Thanks to Frantisek
> > >>>>> Krenzelok <fkrenzel@redhat.com> for providing the implementation in
> > >>>>> gnutls and testing the kernel patches.  
> > >>>>
> > >>>> Please explain why - the kernel TLS is not faster than user space, 
> > >>>> the point of it is primarily to enable offload. And you don't add
> > >>>> offload support here.  
> > >>>
> > >>> Well, TLS1.3 support was added 4 years ago, and yet the offload still
> > >>> doesn't support 1.3 at all.
> > >>
> > >> I'm pretty sure some devices support it. None of the vendors could 
> > >> be bothered to plumb in the kernel support, yet, tho.
> > > 
> > > Our device supports TLS 1.3, it's in our plans to add driver/kernel support.
> > > 
> > >> I don't know of anyone supporting rekeying.
> > > 
> > > Boris, Tariq, do you know?
> > 
> > Rekeying is not trivial to get right with offload. There are at least
> > two problems to solve:
> > 1. On transmit, we need to handle both the new and the old key for new
> > and old (retransmitted) data, respectively. Our device will be able to
> > hold both keys in parallel and to choose the right one at the cost of an
> > if statement in the data-path. Alternatively, we can just fallback to
> > software for the old key and focus on the new key.
> 
> We'll need to keep the old key around until we know all the records
> using it have been fully received, right?  And that could be multiple
> old keys, in case of a quick series of key updates.

Why does the hardware implementation need to store old keys? Does the need
for retransmitted data assume we are operating in TLS_HW_RECORD mode and
the hardware is also implementing the TCP stack?

The TLS RFC assumes that the underlying transport layer provides reliable
and in-order deliver so storing previous keys and encrypting 'old' data
would be quite divergent from normal TLS behavior. Is the TLS_HW_RECORD mode
processing TLS records out of order? If the hardware offload is handling
the TCP networking stack then I feel it should also handle the
retransmission of lost data.

   https://www.rfc-editor.org/rfc/rfc8446#section-1
   the only requirement from the underlying
   transport is a reliable, in-order data stream.

> > 2. On Rx, packets with the new key may arrive before the key is
> > installed unless we design a mechanism for preemptively setting the next
> > key in HW. As a result, we may get a resync on every rekey.
> > 
> > Have you considered an API to preemptively set the next key in the
> > kernel such that there is never a need to stop the datapath? I think
> > that the change in SSL libraries is minor and it can really help KTLS.
> 
> I don't like the idea of having some unused key stored in the kernel
> for long durations too much.
> 
> You can't be sure that there will be only one rekey in the next short
> interval, so you'll need to be able to handle those resyncs anyway, in
> case userspace is too slow providing the 3rd key (for the 2nd
> rekey). For example, the RFC mentions:
> 
>    If implementations independently send their own KeyUpdates with
>    request_update set to "update_requested" and they cross in flight,
>    then each side will also send a response, with the result that each
>    side increments by two generations.
> 
>    https://www.rfc-editor.org/rfc/rfc8446#section-4.6.3
> 
> So I think what you're suggesting can only be an optimization,
> not a full solution.
> 
> I hope I'm not getting too confused by all this.

-- 
Apoorv

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Jakub Kicinski]

On Wed, 25 Jan 2023 10:47:20 -0800 Apoorv Kothari wrote:
> > We'll need to keep the old key around until we know all the records
> > using it have been fully received, right?  And that could be multiple
> > old keys, in case of a quick series of key updates.  
> 
> Why does the hardware implementation need to store old keys? Does the need
> for retransmitted data assume we are operating in TLS_HW_RECORD mode and
> the hardware is also implementing the TCP stack?

We're talking about the Tx direction, the packets are queued to the
lower layers of the stack unencrypted, and get encrypted by the NIC.
Until TCP gets acks for all the data awaiting offloaded crypto - we
must hold onto the keys.

Rx direction is much simpler indeed.

> The TLS RFC assumes that the underlying transport layer provides reliable
> and in-order deliver so storing previous keys and encrypting 'old' data
> would be quite divergent from normal TLS behavior. Is the TLS_HW_RECORD mode
> processing TLS records out of order? If the hardware offload is handling
> the TCP networking stack then I feel it should also handle the
> retransmission of lost data.

Ignore TLS_HW_RECORD, it's a ToE offload, the offload we care about
just offloads encryption.

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Simo Sorce]

On Wed, 2023-01-25 at 10:57 -0800, Jakub Kicinski wrote:
> On Wed, 25 Jan 2023 10:47:20 -0800 Apoorv Kothari wrote:
> > > We'll need to keep the old key around until we know all the records
> > > using it have been fully received, right?  And that could be multiple
> > > old keys, in case of a quick series of key updates.  
> > 
> > Why does the hardware implementation need to store old keys? Does the need
> > for retransmitted data assume we are operating in TLS_HW_RECORD mode and
> > the hardware is also implementing the TCP stack?
> 
> We're talking about the Tx direction, the packets are queued to the
> lower layers of the stack unencrypted, and get encrypted by the NIC.
> Until TCP gets acks for all the data awaiting offloaded crypto - we
> must hold onto the keys.

Is this because the NIC does not cache the already encrypted outgoing
packets?

If that is the case is it _guaranteed_ that the re-encrypted packets
are exactly identical to the previously sent ones?

If it is not guaranteed, are you blocking use of AES GCM and any other
block cipher that may have very bad failure modes in a situation like
this (in the case of AES GCM I am thinking of IV reuse) ?

> 
> Rx direction is much simpler indeed.
> 
> > The TLS RFC assumes that the underlying transport layer provides reliable
> > and in-order deliver so storing previous keys and encrypting 'old' data
> > would be quite divergent from normal TLS behavior. Is the TLS_HW_RECORD mode
> > processing TLS records out of order? If the hardware offload is handling
> > the TCP networking stack then I feel it should also handle the
> > retransmission of lost data.
> 
> Ignore TLS_HW_RECORD, it's a ToE offload, the offload we care about
> just offloads encryption.
> 

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Jakub Kicinski]

On Wed, 25 Jan 2023 16:17:26 -0500 Simo Sorce wrote:
> > We're talking about the Tx direction, the packets are queued to the
> > lower layers of the stack unencrypted, and get encrypted by the NIC.
> > Until TCP gets acks for all the data awaiting offloaded crypto - we
> > must hold onto the keys.  
> 
> Is this because the NIC does not cache the already encrypted outgoing
> packets?

NIC can't cache outgoing packets, there's too many and NIC is supposed
to only do crypto. TCP stack is responsible for handing rtx.

> If that is the case is it _guaranteed_ that the re-encrypted packets
> are exactly identical to the previously sent ones?

In terms of payload, yes. Modulo zero-copy cases we don't need to get
into.

> If it is not guaranteed, are you blocking use of AES GCM and any other
> block cipher that may have very bad failure modes in a situation like
> this (in the case of AES GCM I am thinking of IV reuse) ?

I don't know what you mean.

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Simo Sorce]

On Wed, 2023-01-25 at 14:43 -0800, Jakub Kicinski wrote:
> On Wed, 25 Jan 2023 16:17:26 -0500 Simo Sorce wrote:
> > > We're talking about the Tx direction, the packets are queued to the
> > > lower layers of the stack unencrypted, and get encrypted by the NIC.
> > > Until TCP gets acks for all the data awaiting offloaded crypto - we
> > > must hold onto the keys.  
> > 
> > Is this because the NIC does not cache the already encrypted outgoing
> > packets?
> 
> NIC can't cache outgoing packets, there's too many and NIC is supposed
> to only do crypto. TCP stack is responsible for handing rtx.
> 
> > If that is the case is it _guaranteed_ that the re-encrypted packets
> > are exactly identical to the previously sent ones?
> 
> In terms of payload, yes. Modulo zero-copy cases we don't need to get
> into.
> 
> > If it is not guaranteed, are you blocking use of AES GCM and any other
> > block cipher that may have very bad failure modes in a situation like
> > this (in the case of AES GCM I am thinking of IV reuse) ?
> 
> I don't know what you mean.

The question was if there is *any* case where re-transmission can cause
different data to be encrypted with the same key + same IV

Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Jakub Kicinski]

On Wed, 25 Jan 2023 18:05:38 -0500 Simo Sorce wrote:
> > > If it is not guaranteed, are you blocking use of AES GCM and any other
> > > block cipher that may have very bad failure modes in a situation like
> > > this (in the case of AES GCM I am thinking of IV reuse) ?  
> > 
> > I don't know what you mean.  
> 
> The question was if there is *any* case where re-transmission can cause
> different data to be encrypted with the same key + same IV

Not in valid use cases. With zero-copy / sendfile Tx technically 
the page from the page cache can change between tx and rtx, but 
the user needs to opt in explicitly acknowledging the application 
will prevent this from happening. If they don't opt-in we'll copy 
the data.

Re: [PATCH net-next 0/5] tls: implement key updates for TLS1.3

[Simo Sorce]

On Wed, 2023-01-25 at 15:08 -0800, Jakub Kicinski wrote:
> On Wed, 25 Jan 2023 18:05:38 -0500 Simo Sorce wrote:
> > > > If it is not guaranteed, are you blocking use of AES GCM and any other
> > > > block cipher that may have very bad failure modes in a situation like
> > > > this (in the case of AES GCM I am thinking of IV reuse) ?  
> > > 
> > > I don't know what you mean.  
> > 
> > The question was if there is *any* case where re-transmission can cause
> > different data to be encrypted with the same key + same IV
> 
> Not in valid use cases. With zero-copy / sendfile Tx technically 
> the page from the page cache can change between tx and rtx, but 
> the user needs to opt in explicitly acknowledging the application 
> will prevent this from happening. If they don't opt-in we'll copy 
> the data.

Uhmm is there a way to detect this happening and abort further crypto
operations in case it happens ?

Simo.

-- 
Simo Sorce
RHEL Crypto Team
Red Hat, Inc