Re: [PATCH net 3/3] tls: zero the crypto information from tls_context before freeing
2018-09-05, 13:48:48 +, Vakul Garg wrote: > > > > -Original Message- > > From: netdev-ow...@vger.kernel.org On > > Behalf Of Sabrina Dubroca > > Sent: Wednesday, September 5, 2018 6:52 PM > > To: netdev@vger.kernel.org > > Cc: Sabrina Dubroca ; Boris Pismenny > > ; Ilya Lesokhin ; Aviad > > Yehezkel ; Dave Watson > > Subject: [PATCH net 3/3] tls: zero the crypto information from tls_context > > before freeing > > > > This contains key material in crypto_send_aes_gcm_128 and > > crypto_recv_aes_gcm_128. > > > > Fixes: 3c4d7559159b ("tls: kernel TLS support") > > Signed-off-by: Sabrina Dubroca > > --- > > include/net/tls.h | 1 + > > net/tls/tls_main.c | 14 -- > > 2 files changed, 13 insertions(+), 2 deletions(-) > > > > diff --git a/include/net/tls.h b/include/net/tls.h index > > d5c683e8bb22..2010d23112f9 100644 > > --- a/include/net/tls.h > > +++ b/include/net/tls.h > > @@ -180,6 +180,7 @@ struct tls_context { > > struct tls_crypto_info crypto_recv; > > struct tls12_crypto_info_aes_gcm_128 > > crypto_recv_aes_gcm_128; > > }; > > + char tls_crypto_ctx_end[0]; > > This makes the key zeroization dependent upon the position of unions > in structure. Yes. I could add char tls_crypto_ctx_start[0]. > Can you zeroise the crypto_send, crypto_recv separately using two > memzero_explicit calls? It's not crypto_send, it's crypto_send_aes_gcm_128. I don't know how likely it is that another crypto algorithm will ever be added, but then you'd have to switch to zeroing that thing. I had a different version of the patch that gave a name to those unions, but then I need to change all the references everywhere and the patch becomes a bit ugly, especially for net. struct tls_context { union { struct tls_crypto_info info; struct tls12_crypto_info_aes_gcm_128 aes_gcm_128; } crypto_send; union { struct tls_crypto_info info; struct tls12_crypto_info_aes_gcm_128 aes_gcm_128; } crypto_recv; [...] } Or even: union tls_crypto_context { struct tls_crypto_info info; struct tls12_crypto_info_aes_gcm_128 aes_gcm_128; }; struct tls_context { union tls_crypto_context crypto_send; union tls_crypto_context crypto_recv; [...] } -- Sabrina
RE: [PATCH net 3/3] tls: zero the crypto information from tls_context before freeing
> -Original Message- > From: netdev-ow...@vger.kernel.org On > Behalf Of Sabrina Dubroca > Sent: Wednesday, September 5, 2018 6:52 PM > To: netdev@vger.kernel.org > Cc: Sabrina Dubroca ; Boris Pismenny > ; Ilya Lesokhin ; Aviad > Yehezkel ; Dave Watson > Subject: [PATCH net 3/3] tls: zero the crypto information from tls_context > before freeing > > This contains key material in crypto_send_aes_gcm_128 and > crypto_recv_aes_gcm_128. > > Fixes: 3c4d7559159b ("tls: kernel TLS support") > Signed-off-by: Sabrina Dubroca > --- > include/net/tls.h | 1 + > net/tls/tls_main.c | 14 -- > 2 files changed, 13 insertions(+), 2 deletions(-) > > diff --git a/include/net/tls.h b/include/net/tls.h index > d5c683e8bb22..2010d23112f9 100644 > --- a/include/net/tls.h > +++ b/include/net/tls.h > @@ -180,6 +180,7 @@ struct tls_context { > struct tls_crypto_info crypto_recv; > struct tls12_crypto_info_aes_gcm_128 > crypto_recv_aes_gcm_128; > }; > + char tls_crypto_ctx_end[0]; This makes the key zeroization dependent upon the position of unions in structure. Can you zeroise the crypto_send, crypto_recv separately using two memzero_explicit calls? > > struct list_head list; > struct net_device *netdev; > diff --git a/net/tls/tls_main.c b/net/tls/tls_main.c index > 0d432d025471..d3a57c0b2182 100644 > --- a/net/tls/tls_main.c > +++ b/net/tls/tls_main.c > @@ -241,6 +241,16 @@ static void tls_write_space(struct sock *sk) > ctx->sk_write_space(sk); > } > > +static void tls_ctx_free(struct tls_context *ctx) { > + if (!ctx) > + return; > + > + memzero_explicit(>crypto_send, > + offsetof(struct tls_context, tls_crypto_ctx_end)); > + kfree(ctx); > +} > + > static void tls_sk_proto_close(struct sock *sk, long timeout) { > struct tls_context *ctx = tls_get_ctx(sk); @@ -294,7 +304,7 @@ > static void tls_sk_proto_close(struct sock *sk, long timeout) #else > { > #endif > - kfree(ctx); > + tls_ctx_free(ctx); > ctx = NULL; > } > > @@ -305,7 +315,7 @@ static void tls_sk_proto_close(struct sock *sk, long > timeout) >* for sk->sk_prot->unhash [tls_hw_unhash] >*/ > if (free_ctx) > - kfree(ctx); > + tls_ctx_free(ctx); > } > > static int do_tls_getsockopt_tx(struct sock *sk, char __user *optval, > -- > 2.18.0