From mboxrd@z Thu Jan 1 00:00:00 1970 From: Sabrina Dubroca Subject: Re: [PATCH net 3/3] tls: zero the crypto information from tls_context before freeing Date: Thu, 6 Sep 2018 09:42:49 +0200 Message-ID: <20180906074249.GA14270@bistromath.localdomain> References: Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Cc: "netdev@vger.kernel.org" , Boris Pismenny , Ilya Lesokhin , Aviad Yehezkel , Dave Watson To: Vakul Garg Return-path: Received: from mx3-rdu2.redhat.com ([66.187.233.73]:42450 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1725929AbeIFMQm (ORCPT ); Thu, 6 Sep 2018 08:16:42 -0400 Content-Disposition: inline In-Reply-To: Sender: netdev-owner@vger.kernel.org List-ID: 2018-09-05, 13:48:48 +0000, Vakul Garg wrote: > > > > -----Original Message----- > > From: netdev-owner@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