| Message ID | 20200827064402.7130-2-efremov@linux.com (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | crypto: use kfree_sensitive() | expand |
On Thu, Aug 27, 2020 at 09:43:59AM +0300, Denis Efremov wrote: > Use kfree_sensitive() instead of open-coding it. > > Signed-off-by: Denis Efremov <efremov@linux.com> > --- > drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > index 16a467969d8e..5ffdc1cd5847 100644 > --- a/drivers/crypto/inside-secure/safexcel_hash.c > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq, > } > > /* Avoid leaking */ > - memzero_explicit(keydup, keylen); > - kfree(keydup); > + kfree_sensitive(keydup); > > if (ret) > return ret; > -- > 2.26.2 > The maintainer of this driver was not TO/CC. I Add him. Regards
Hello Denis, Quoting Denis Efremov (2020-08-27 08:43:59) > Use kfree_sensitive() instead of open-coding it. > > Signed-off-by: Denis Efremov <efremov@linux.com> Acked-by: Antoine Tenart <antoine.tenart@bootlin.com> Thanks! Antoine > --- > drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > index 16a467969d8e..5ffdc1cd5847 100644 > --- a/drivers/crypto/inside-secure/safexcel_hash.c > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq, > } > > /* Avoid leaking */ > - memzero_explicit(keydup, keylen); > - kfree(keydup); > + kfree_sensitive(keydup); > > if (ret) > return ret; > -- > 2.26.2 >
> -----Original Message----- > From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Denis Efremov > Sent: Thursday, August 27, 2020 8:44 AM > To: linux-crypto@vger.kernel.org > Cc: Denis Efremov <efremov@linux.com>; Corentin Labbe <clabbe.montjoie@gmail.com>; Herbert Xu > <herbert@gondor.apana.org.au>; linux-kernel@vger.kernel.org > Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive() > > <<< External Email >>> > Use kfree_sensitive() instead of open-coding it. > > Signed-off-by: Denis Efremov <efremov@linux.com> > --- > drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- > 1 file changed, 1 insertion(+), 2 deletions(-) > > diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > index 16a467969d8e..5ffdc1cd5847 100644 > --- a/drivers/crypto/inside-secure/safexcel_hash.c > +++ b/drivers/crypto/inside-secure/safexcel_hash.c > @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq, > } > > /* Avoid leaking */ > -memzero_explicit(keydup, keylen); > -kfree(keydup); > +kfree_sensitive(keydup); > I'm not sure here ... I verified it does not break the driver (not a big surprise), but ... memzero_explicit guarantees that it will not get optimized away and the keydata _always_ gets overwritten. Does kfree_sensitive also come with such a guarantee? I could not find a hard statement on that in its documentation. Although the "sensitive" part surely suggests it. Additionally, this remark is made in the documentation for kfree_sensitive: "this function zeroes the whole allocated buffer which can be a good deal bigger than the requested buffer size passed to kmalloc(). So be careful when using this function in performance sensitive code" While the memzero_explicit does not zeroize anything beyond keylen. Which is all you really need here, so why would you want to zeroize potentially a lot more? In any case the two are not fully equivalent. Any opinions? > if (ret) > return ret; > -- > 2.26.2 Regards, Pascal van Leeuwen Silicon IP Architect Multi-Protocol Engines, Rambus Security Rambus ROTW Holding BV +31-73 6581953 Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus. Please be so kind to update your e-mail address book with my new e-mail address. ** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. ** Rambus Inc.<http://www.rambus.com>
Hi, On 9/2/20 4:10 PM, Van Leeuwen, Pascal wrote: >> -----Original Message----- >> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Denis Efremov >> Sent: Thursday, August 27, 2020 8:44 AM >> To: linux-crypto@vger.kernel.org >> Cc: Denis Efremov <efremov@linux.com>; Corentin Labbe <clabbe.montjoie@gmail.com>; Herbert Xu >> <herbert@gondor.apana.org.au>; linux-kernel@vger.kernel.org >> Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive() >> >> <<< External Email >>> >> Use kfree_sensitive() instead of open-coding it. >> >> Signed-off-by: Denis Efremov <efremov@linux.com> >> --- >> drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- >> 1 file changed, 1 insertion(+), 2 deletions(-) >> >> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c >> index 16a467969d8e..5ffdc1cd5847 100644 >> --- a/drivers/crypto/inside-secure/safexcel_hash.c >> +++ b/drivers/crypto/inside-secure/safexcel_hash.c >> @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq, >> } >> >> /* Avoid leaking */ >> -memzero_explicit(keydup, keylen); >> -kfree(keydup); >> +kfree_sensitive(keydup); >> > I'm not sure here ... I verified it does not break the driver (not a big surprise), but ... > > memzero_explicit guarantees that it will not get optimized away and the keydata _always_ > gets overwritten. Does kfree_sensitive also come with such a guarantee? I could not find a > hard statement on that in its documentation. Although the "sensitive" part surely suggests > it. kfree_sensitive() uses memzero_explicit() internally. > Additionally, this remark is made in the documentation for kfree_sensitive: "this function > zeroes the whole allocated buffer which can be a good deal bigger than the requested buffer > size passed to kmalloc(). So be careful when using this function in performance sensitive > code" > > While the memzero_explicit does not zeroize anything beyond keylen. > Which is all you really need here, so why would you want to zeroize potentially a lot more? > In any case the two are not fully equivalent. There are a number of predefined allocation sizes (power of 2) for faster alloc, i.e. https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L349 and it looks like that keys we free in this patches are in bounds of these sizes. As far as I understand, if a key is not a power of 2 len, the buffer will be zeroed to the closest power of 2 size. For small sizes like these, performance difference should be unnoticeable because of cache lines and how arch-optimized memzero() works. Key freeing doesn't look like a frequent event. Thanks, Denis
> -----Original Message----- > From: Denis Efremov <efremov@linux.com> > Sent: Friday, September 4, 2020 10:55 AM > To: Van Leeuwen, Pascal <pvanleeuwen@rambus.com>; linux-crypto@vger.kernel.org > Cc: Corentin Labbe <clabbe.montjoie@gmail.com>; Herbert Xu <herbert@gondor.apana.org.au>; linux-kernel@vger.kernel.org > Subject: Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive() > > <<< External Email >>> > Hi, > > On 9/2/20 4:10 PM, Van Leeuwen, Pascal wrote: > >> -----Original Message----- > >> From: linux-crypto-owner@vger.kernel.org <linux-crypto-owner@vger.kernel.org> On Behalf Of Denis Efremov > >> Sent: Thursday, August 27, 2020 8:44 AM > >> To: linux-crypto@vger.kernel.org > >> Cc: Denis Efremov <efremov@linux.com>; Corentin Labbe <clabbe.montjoie@gmail.com>; Herbert Xu > >> <herbert@gondor.apana.org.au>; linux-kernel@vger.kernel.org > >> Subject: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive() > >> > >> <<< External Email >>> > >> Use kfree_sensitive() instead of open-coding it. > >> > >> Signed-off-by: Denis Efremov <efremov@linux.com> > >> --- > >> drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- > >> 1 file changed, 1 insertion(+), 2 deletions(-) > >> > >> diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c > >> index 16a467969d8e..5ffdc1cd5847 100644 > >> --- a/drivers/crypto/inside-secure/safexcel_hash.c > >> +++ b/drivers/crypto/inside-secure/safexcel_hash.c > >> @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq, > >> } > >> > >> /* Avoid leaking */ > >> -memzero_explicit(keydup, keylen); > >> -kfree(keydup); > >> +kfree_sensitive(keydup); > >> > > I'm not sure here ... I verified it does not break the driver (not a big surprise), but ... > > > > memzero_explicit guarantees that it will not get optimized away and the keydata _always_ > > gets overwritten. Does kfree_sensitive also come with such a guarantee? I could not find a > > hard statement on that in its documentation. Although the "sensitive" part surely suggests > > it. > > kfree_sensitive() uses memzero_explicit() internally. > Ok. Although formally that's still only _current_ implementation. But given the function name, I guess it's a fair assumption that the intention is to maintain this behavior going forward. > > Additionally, this remark is made in the documentation for kfree_sensitive: "this function > > zeroes the whole allocated buffer which can be a good deal bigger than the requested buffer > > size passed to kmalloc(). So be careful when using this function in performance sensitive > > code" > > > > While the memzero_explicit does not zeroize anything beyond keylen. > > Which is all you really need here, so why would you want to zeroize potentially a lot more? > > In any case the two are not fully equivalent. > > There are a number of predefined allocation sizes (power of 2) for faster alloc, > i.e. https://elixir.bootlin.com/linux/latest/source/include/linux/slab.h#L349 > and it looks like that keys we free in this patches are in bounds of these sizes. > As far as I understand, if a key is not a power of 2 len, the buffer will be zeroed to the closest > power of 2 size. > This path is for hash keys that are larger than the hash block size. Potentially, there is no upper bound on the size of such a hash key, so it doesn't need to be in that range hence zeroizing to the next power of 2 boundary could be expensive. OTOH, I don't expect this path to be frequently used, and the key processing itself already costs a lot of time, so it's probably not that relevant. Never mind. I guess was more about whether using kfree_sensitive() is a good replacement _in general_. For that, there should be some guaranteed upper bound on how much extra will be zeroized. Given the above considerations (and after testing this on my hardware): Tested-by: Pascal van Leeuwen <pvanleeuwen@rambus.com> > For small sizes like these, performance difference should be unnoticeable because > of cache lines and how arch-optimized memzero() works. Key freeing doesn't look like a frequent event. > > Thanks, > Denis Regards, Pascal van Leeuwen Silicon IP Architect Multi-Protocol Engines, Rambus Security Rambus ROTW Holding BV +31-73 6581953 Note: The Inside Secure/Verimatrix Silicon IP team was recently acquired by Rambus. Please be so kind to update your e-mail address book with my new e-mail address. ** This message and any attachments are for the sole use of the intended recipient(s). It may contain information that is confidential and privileged. If you are not the intended recipient of this message, you are prohibited from printing, copying, forwarding or saving it. Please delete the message and attachments and notify the sender immediately. ** Rambus Inc.<http://www.rambus.com>
diff --git a/drivers/crypto/inside-secure/safexcel_hash.c b/drivers/crypto/inside-secure/safexcel_hash.c index 16a467969d8e..5ffdc1cd5847 100644 --- a/drivers/crypto/inside-secure/safexcel_hash.c +++ b/drivers/crypto/inside-secure/safexcel_hash.c @@ -1082,8 +1082,7 @@ static int safexcel_hmac_init_pad(struct ahash_request *areq, } /* Avoid leaking */ - memzero_explicit(keydup, keylen); - kfree(keydup); + kfree_sensitive(keydup); if (ret) return ret;
Use kfree_sensitive() instead of open-coding it. Signed-off-by: Denis Efremov <efremov@linux.com> --- drivers/crypto/inside-secure/safexcel_hash.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-)