| Message ID | 979d2f89a6a994d5bb49cae49a80be54150d094d.1697653889.git.sd@queasysnail.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b7c4f5730a9fa258c8e79f6387a03f3a95c681a2 |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] tls: don't reset prot->aad_size and prot->tail_size for TLS_HW | expand |
On Fri, 20 Oct 2023 16:00:55 +0200 Sabrina Dubroca wrote: > Prior to commit 1a074f7618e8 ("tls: also use init_prot_info in > tls_set_device_offload"), setting TLS_HW on TX didn't touch > prot->aad_size and prot->tail_size. They are set to 0 during context > allocation (tls_prot_info is embedded in tls_context, kzalloc'd by > tls_ctx_create). > > When the RX key is configured, tls_set_sw_offload is called (for both > TLS_SW and TLS_HW). If the TX key is configured in TLS_HW mode after > the RX key has been installed, init_prot_info will now overwrite the > correct values of aad_size and tail_size, breaking SW decryption and > causing -EBADMSG errors to be returned to userspace. > > Since TLS_HW doesn't use aad_size and tail_size at all (for TLS1.2, > tail_size is always 0, and aad_size is equal to TLS_HEADER_SIZE + > rec_seq_size), we can simply drop this hunk. > > Fixes: 1a074f7618e8 ("tls: also use init_prot_info in tls_set_device_offload") > Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> > --- > Tariq, does that solve the problem you reported in > https://lore.kernel.org/netdev/3ace1e75-c0a5-4473-848d-91f9ac0a8f9c@gmail.com/ > ? In case Tariq replies before Monday and DaveM wants to take it, LGTM: Acked-by: Jakub Kicinski <kuba@kernel.org>
On 21/10/2023 3:14, Jakub Kicinski wrote: > On Fri, 20 Oct 2023 16:00:55 +0200 Sabrina Dubroca wrote: >> Prior to commit 1a074f7618e8 ("tls: also use init_prot_info in >> tls_set_device_offload"), setting TLS_HW on TX didn't touch >> prot->aad_size and prot->tail_size. They are set to 0 during context >> allocation (tls_prot_info is embedded in tls_context, kzalloc'd by >> tls_ctx_create). >> >> When the RX key is configured, tls_set_sw_offload is called (for both >> TLS_SW and TLS_HW). If the TX key is configured in TLS_HW mode after >> the RX key has been installed, init_prot_info will now overwrite the >> correct values of aad_size and tail_size, breaking SW decryption and >> causing -EBADMSG errors to be returned to userspace. >> >> Since TLS_HW doesn't use aad_size and tail_size at all (for TLS1.2, >> tail_size is always 0, and aad_size is equal to TLS_HEADER_SIZE + >> rec_seq_size), we can simply drop this hunk. >> >> Fixes: 1a074f7618e8 ("tls: also use init_prot_info in tls_set_device_offload") >> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> >> --- >> Tariq, does that solve the problem you reported in >> https://lore.kernel.org/netdev/3ace1e75-c0a5-4473-848d-91f9ac0a8f9c@gmail.com/ >> ? > > In case Tariq replies before Monday and DaveM wants to take it, LGTM: > > Acked-by: Jakub Kicinski <kuba@kernel.org> Hi, We're testing this fix and will reply ASAP. Regards, Tariq
On 22/10/2023 15:02, Tariq Toukan wrote: > > > On 21/10/2023 3:14, Jakub Kicinski wrote: >> On Fri, 20 Oct 2023 16:00:55 +0200 Sabrina Dubroca wrote: >>> Prior to commit 1a074f7618e8 ("tls: also use init_prot_info in >>> tls_set_device_offload"), setting TLS_HW on TX didn't touch >>> prot->aad_size and prot->tail_size. They are set to 0 during context >>> allocation (tls_prot_info is embedded in tls_context, kzalloc'd by >>> tls_ctx_create). >>> >>> When the RX key is configured, tls_set_sw_offload is called (for both >>> TLS_SW and TLS_HW). If the TX key is configured in TLS_HW mode after >>> the RX key has been installed, init_prot_info will now overwrite the >>> correct values of aad_size and tail_size, breaking SW decryption and >>> causing -EBADMSG errors to be returned to userspace. >>> >>> Since TLS_HW doesn't use aad_size and tail_size at all (for TLS1.2, >>> tail_size is always 0, and aad_size is equal to TLS_HEADER_SIZE + >>> rec_seq_size), we can simply drop this hunk. >>> >>> Fixes: 1a074f7618e8 ("tls: also use init_prot_info in >>> tls_set_device_offload") >>> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> >>> --- >>> Tariq, does that solve the problem you reported in >>> https://lore.kernel.org/netdev/3ace1e75-c0a5-4473-848d-91f9ac0a8f9c@gmail.com/ >>> ? >> >> In case Tariq replies before Monday and DaveM wants to take it, LGTM: >> >> Acked-by: Jakub Kicinski <kuba@kernel.org> > > Hi, > > We're testing this fix and will reply ASAP. > Test passes: Tested-by: Ran Rozenstein <ranro@nvidia.com> We suspect that it was not the only degradation introduced by this series. We are going to run more comprehensive tests with the recent series and this new fix. Of course we'll update about any remaining issues. Tariq
Hello: This patch was applied to netdev/net-next.git (main) by Jakub Kicinski <kuba@kernel.org>: On Fri, 20 Oct 2023 16:00:55 +0200 you wrote: > Prior to commit 1a074f7618e8 ("tls: also use init_prot_info in > tls_set_device_offload"), setting TLS_HW on TX didn't touch > prot->aad_size and prot->tail_size. They are set to 0 during context > allocation (tls_prot_info is embedded in tls_context, kzalloc'd by > tls_ctx_create). > > When the RX key is configured, tls_set_sw_offload is called (for both > TLS_SW and TLS_HW). If the TX key is configured in TLS_HW mode after > the RX key has been installed, init_prot_info will now overwrite the > correct values of aad_size and tail_size, breaking SW decryption and > causing -EBADMSG errors to be returned to userspace. > > [...] Here is the summary with links: - [net-next] tls: don't reset prot->aad_size and prot->tail_size for TLS_HW https://git.kernel.org/netdev/net-next/c/b7c4f5730a9f You are awesome, thank you!
diff --git a/net/tls/tls.h b/net/tls/tls.h index 478b2c0060aa..762f424ff2d5 100644 --- a/net/tls/tls.h +++ b/net/tls/tls.h @@ -144,8 +144,7 @@ void tls_err_abort(struct sock *sk, int err); int init_prot_info(struct tls_prot_info *prot, const struct tls_crypto_info *crypto_info, - const struct tls_cipher_desc *cipher_desc, - int mode); + const struct tls_cipher_desc *cipher_desc); 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); diff --git a/net/tls/tls_device.c b/net/tls/tls_device.c index f01543557a60..bf8ed36b1ad6 100644 --- a/net/tls/tls_device.c +++ b/net/tls/tls_device.c @@ -1099,7 +1099,7 @@ int tls_set_device_offload(struct sock *sk) goto release_netdev; } - rc = init_prot_info(prot, crypto_info, cipher_desc, TLS_HW); + rc = init_prot_info(prot, crypto_info, cipher_desc); if (rc) goto release_netdev; diff --git a/net/tls/tls_sw.c b/net/tls/tls_sw.c index 0f6da4ce3ed7..93747ba0d4f0 100644 --- a/net/tls/tls_sw.c +++ b/net/tls/tls_sw.c @@ -2622,8 +2622,7 @@ static struct tls_sw_context_rx *init_ctx_rx(struct tls_context *ctx) int init_prot_info(struct tls_prot_info *prot, const struct tls_crypto_info *crypto_info, - const struct tls_cipher_desc *cipher_desc, - int mode) + const struct tls_cipher_desc *cipher_desc) { u16 nonce_size = cipher_desc->nonce; @@ -2636,11 +2635,6 @@ int init_prot_info(struct tls_prot_info *prot, prot->tail_size = 0; } - if (mode == TLS_HW) { - prot->aad_size = 0; - prot->tail_size = 0; - } - /* Sanity-check the sizes for stack allocations. */ if (nonce_size > TLS_MAX_IV_SIZE || prot->aad_size > TLS_MAX_AAD_SIZE) return -EINVAL; @@ -2700,7 +2694,7 @@ int tls_set_sw_offload(struct sock *sk, int tx) goto free_priv; } - rc = init_prot_info(prot, crypto_info, cipher_desc, TLS_SW); + rc = init_prot_info(prot, crypto_info, cipher_desc); if (rc) goto free_priv;
Prior to commit 1a074f7618e8 ("tls: also use init_prot_info in tls_set_device_offload"), setting TLS_HW on TX didn't touch prot->aad_size and prot->tail_size. They are set to 0 during context allocation (tls_prot_info is embedded in tls_context, kzalloc'd by tls_ctx_create). When the RX key is configured, tls_set_sw_offload is called (for both TLS_SW and TLS_HW). If the TX key is configured in TLS_HW mode after the RX key has been installed, init_prot_info will now overwrite the correct values of aad_size and tail_size, breaking SW decryption and causing -EBADMSG errors to be returned to userspace. Since TLS_HW doesn't use aad_size and tail_size at all (for TLS1.2, tail_size is always 0, and aad_size is equal to TLS_HEADER_SIZE + rec_seq_size), we can simply drop this hunk. Fixes: 1a074f7618e8 ("tls: also use init_prot_info in tls_set_device_offload") Signed-off-by: Sabrina Dubroca <sd@queasysnail.net> --- Tariq, does that solve the problem you reported in https://lore.kernel.org/netdev/3ace1e75-c0a5-4473-848d-91f9ac0a8f9c@gmail.com/ ? net/tls/tls.h | 3 +-- net/tls/tls_device.c | 2 +- net/tls/tls_sw.c | 10 ++-------- 3 files changed, 4 insertions(+), 11 deletions(-)