Re: [PATCH v2 1/4] crypto: inside-secure - use kfree_sensitive()

[Denis Efremov]

Hi,

On 9/2/20 4:10 PM, Van Leeuwen, Pascal wrote:
>> -----Original Message-----
>> From: linux-crypto-owner@xxxxxxxxxxxxxxx <linux-crypto-owner@xxxxxxxxxxxxxxx> On Behalf Of Denis Efremov
>> Sent: Thursday, August 27, 2020 8:44 AM
>> To: linux-crypto@xxxxxxxxxxxxxxx
>> Cc: Denis Efremov <efremov@xxxxxxxxx>; Corentin Labbe <clabbe.montjoie@xxxxxxxxx>; Herbert Xu
>> <herbert@xxxxxxxxxxxxxxxxxxx>; linux-kernel@xxxxxxxxxxxxxxx
>> 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@xxxxxxxxx>
>> ---
>>  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