[Cocci] Querying symbol tables in SmPL scripts?
Hello, The semantic patch language supports to search for various identifiers in source files. It is often hoped then that a found identifier fits to a known application context. But it can happen with generic source code analysis approaches that a desirable confidence level can not easily be achieved. Thus I would be looking for additional means to reduce uncertainty about a software situation considerably. I imagine that a corresponding possibility would be the use of symbol tables. Such tables provide some information for known symbols. How do you think about to take such data structures into account a bit more? Regards, Markus ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()
On 4/14/20 2:08 AM, Christophe Leroy wrote:
>
>
> Le 14/04/2020 à 00:28, Waiman Long a écrit :
>> Since kfree_sensitive() will do an implicit memzero_explicit(), there
>> is no need to call memzero_explicit() before it. Eliminate those
>> memzero_explicit() and simplify the call sites. For better correctness,
>> the setting of keylen is also moved down after the key pointer check.
>>
>> Signed-off-by: Waiman Long
>> ---
>> .../allwinner/sun8i-ce/sun8i-ce-cipher.c | 19 +-
>> .../allwinner/sun8i-ss/sun8i-ss-cipher.c | 20 +--
>> drivers/crypto/amlogic/amlogic-gxl-cipher.c | 12 +++
>> drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
>> 4 files changed, 14 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> index aa4e8fdc2b32..8358fac98719 100644
>> --- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> +++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> @@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
>> {
>> struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
>> - if (op->key) {
>> - memzero_explicit(op->key, op->keylen);
>> - kfree(op->key);
>> - }
>> + kfree_sensitive(op->key);
>> crypto_free_sync_skcipher(op->fallback_tfm);
>> pm_runtime_put_sync_suspend(op->ce->dev);
>> }
>> @@ -391,14 +388,11 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher
>> *tfm, const u8 *key,
>> dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
>> return -EINVAL;
>> }
>> - if (op->key) {
>> - memzero_explicit(op->key, op->keylen);
>> - kfree(op->key);
>> - }
>> - op->keylen = keylen;
>> + kfree_sensitive(op->key);
>> op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>> if (!op->key)
>> return -ENOMEM;
>> + op->keylen = keylen;
>
> Does it matter at all to ensure op->keylen is not set when of->key is
> NULL ? I'm not sure.
>
> But if it does, then op->keylen should be set to 0 when freeing op->key.
My thinking is that if memory allocation fails, we just don't touch
anything and return an error code. I will not explicitly set keylen to 0
in this case unless it is specified in the API documentation.
Cheers,
Longman
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
Waiman Long wrote: > As said by Linus: > > A symmetric naming is only helpful if it implies symmetries in use. > Otherwise it's actively misleading. > > In "kzalloc()", the z is meaningful and an important part of what the > caller wants. > > In "kzfree()", the z is actively detrimental, because maybe in the > future we really _might_ want to use that "memfill(0xdeadbeef)" or > something. The "zero" part of the interface isn't even _relevant_. > > The main reason that kzfree() exists is to clear sensitive information > that should not be leaked to other future users of the same memory > objects. > > Rename kzfree() to kfree_sensitive() to follow the example of the > recently added kvfree_sensitive() and make the intention of the API > more explicit. In addition, memzero_explicit() is used to clear the > memory to make sure that it won't get optimized away by the compiler. > > The renaming is done by using the command sequence: > > git grep -w --name-only kzfree |\ > xargs sed -i 's/\bkzfree\b/kfree_sensitive/' > > followed by some editing of the kfree_sensitive() kerneldoc and the > use of memzero_explicit() instead of memset(). > > Suggested-by: Joe Perches > Signed-off-by: Waiman Long Since this changes a lot of crypto stuff, does it make sense for it to go via the crypto tree? Acked-by: David Howells ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
On Mon, Apr 13, 2020 at 05:15:49PM -0400, Waiman Long wrote: > fs/btrfs/ioctl.c | 2 +- > diff --git a/fs/btrfs/ioctl.c b/fs/btrfs/ioctl.c > index 40b729dce91c..eab3f8510426 100644 > --- a/fs/btrfs/ioctl.c > +++ b/fs/btrfs/ioctl.c > @@ -2691,7 +2691,7 @@ static int btrfs_ioctl_get_subvol_info(struct file > *file, void __user *argp) > btrfs_put_root(root); > out_free: > btrfs_free_path(path); > - kzfree(subvol_info); > + kfree_sensitive(subvol_info); This is not in a sensitive context so please switch it to plain kfree. With that you have my acked-by. Thanks. ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
On Mon 13-04-20 17:15:49, Waiman Long wrote: > As said by Linus: > > A symmetric naming is only helpful if it implies symmetries in use. > Otherwise it's actively misleading. > > In "kzalloc()", the z is meaningful and an important part of what the > caller wants. > > In "kzfree()", the z is actively detrimental, because maybe in the > future we really _might_ want to use that "memfill(0xdeadbeef)" or > something. The "zero" part of the interface isn't even _relevant_. > > The main reason that kzfree() exists is to clear sensitive information > that should not be leaked to other future users of the same memory > objects. > > Rename kzfree() to kfree_sensitive() to follow the example of the > recently added kvfree_sensitive() and make the intention of the API > more explicit. In addition, memzero_explicit() is used to clear the > memory to make sure that it won't get optimized away by the compiler. > > The renaming is done by using the command sequence: > > git grep -w --name-only kzfree |\ > xargs sed -i 's/\bkzfree\b/kfree_sensitive/' > > followed by some editing of the kfree_sensitive() kerneldoc and the > use of memzero_explicit() instead of memset(). > > Suggested-by: Joe Perches > Signed-off-by: Waiman Long Makes sense. I haven't checked all the conversions and will rely on the script doing the right thing. The core MM part is correct. Acked-by: Michal Hocko -- Michal Hocko SUSE Labs ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
On 4/13/20 3:15 PM, Waiman Long wrote:
As said by Linus:
A symmetric naming is only helpful if it implies symmetries in use.
Otherwise it's actively misleading.
In "kzalloc()", the z is meaningful and an important part of what the
caller wants.
In "kzfree()", the z is actively detrimental, because maybe in the
future we really _might_ want to use that "memfill(0xdeadbeef)" or
something. The "zero" part of the interface isn't even _relevant_.
The main reason that kzfree() exists is to clear sensitive information
that should not be leaked to other future users of the same memory
objects.
Rename kzfree() to kfree_sensitive() to follow the example of the
recently added kvfree_sensitive() and make the intention of the API
more explicit.
Seems reasonable to me. One bikeshed, that you can safely discard and
ignore as a mere bikeshed: kfree_memzero or kfree_scrub or
kfree_{someverb} seems like a better function name, as it describes what
the function does, rather than "_sensitive" that suggests something
about the data maybe but who knows what that entails. If you disagree,
not a big deal either way.
> In addition, memzero_explicit() is used to clear the
> memory to make sure that it won't get optimized away by the compiler.
This had occurred to me momentarily a number of years ago, but I was
under the impression that the kernel presumes extern function calls to
always imply a compiler barrier, making it difficult for the compiler to
reason about what happens in/after kfree, in order to be able to
optimize out the preceding memset. With LTO, that rule obviously
changes. I guess new code should be written with cross-object
optimizations in mind now a days? [Meanwhile, it would be sort of
interesting to teach gcc about kfree to enable additional scary
optimizations...]
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
On Mon, 13 Apr 2020, Waiman Long wrote: > As said by Linus: > > A symmetric naming is only helpful if it implies symmetries in use. > Otherwise it's actively misleading. > > In "kzalloc()", the z is meaningful and an important part of what the > caller wants. > > In "kzfree()", the z is actively detrimental, because maybe in the > future we really _might_ want to use that "memfill(0xdeadbeef)" or > something. The "zero" part of the interface isn't even _relevant_. > > The main reason that kzfree() exists is to clear sensitive information > that should not be leaked to other future users of the same memory > objects. > > Rename kzfree() to kfree_sensitive() to follow the example of the > recently added kvfree_sensitive() and make the intention of the API > more explicit. In addition, memzero_explicit() is used to clear the > memory to make sure that it won't get optimized away by the compiler. > > The renaming is done by using the command sequence: > > git grep -w --name-only kzfree |\ > xargs sed -i 's/\bkzfree\b/kfree_sensitive/' > > followed by some editing of the kfree_sensitive() kerneldoc and the > use of memzero_explicit() instead of memset(). > > Suggested-by: Joe Perches > Signed-off-by: Waiman Long Acked-by: David Rientjes ___ Cocci mailing list Cocci@systeme.lip6.fr https://systeme.lip6.fr/mailman/listinfo/cocci
[Cocci] [PATCH 1/2] mm, treewide: Rename kzfree() to kfree_sensitive()
As said by Linus: A symmetric naming is only helpful if it implies symmetries in use. Otherwise it's actively misleading. In "kzalloc()", the z is meaningful and an important part of what the caller wants. In "kzfree()", the z is actively detrimental, because maybe in the future we really _might_ want to use that "memfill(0xdeadbeef)" or something. The "zero" part of the interface isn't even _relevant_. The main reason that kzfree() exists is to clear sensitive information that should not be leaked to other future users of the same memory objects. Rename kzfree() to kfree_sensitive() to follow the example of the recently added kvfree_sensitive() and make the intention of the API more explicit. In addition, memzero_explicit() is used to clear the memory to make sure that it won't get optimized away by the compiler. The renaming is done by using the command sequence: git grep -w --name-only kzfree |\ xargs sed -i 's/\bkzfree\b/kfree_sensitive/' followed by some editing of the kfree_sensitive() kerneldoc and the use of memzero_explicit() instead of memset(). Suggested-by: Joe Perches Signed-off-by: Waiman Long --- arch/s390/crypto/prng.c | 4 +-- arch/x86/power/hibernate.c| 2 +- crypto/adiantum.c | 2 +- crypto/ahash.c| 4 +-- crypto/api.c | 2 +- crypto/asymmetric_keys/verify_pefile.c| 4 +-- crypto/deflate.c | 2 +- crypto/drbg.c | 10 +++--- crypto/ecc.c | 8 ++--- crypto/ecdh.c | 2 +- crypto/gcm.c | 2 +- crypto/gf128mul.c | 4 +-- crypto/jitterentropy-kcapi.c | 2 +- crypto/rng.c | 2 +- crypto/rsa-pkcs1pad.c | 6 ++-- crypto/seqiv.c| 2 +- crypto/shash.c| 2 +- crypto/skcipher.c | 2 +- crypto/testmgr.c | 6 ++-- crypto/zstd.c | 2 +- .../allwinner/sun8i-ce/sun8i-ce-cipher.c | 2 +- .../allwinner/sun8i-ss/sun8i-ss-cipher.c | 2 +- drivers/crypto/amlogic/amlogic-gxl-cipher.c | 4 +-- drivers/crypto/atmel-ecc.c| 2 +- drivers/crypto/caam/caampkc.c | 28 +++ drivers/crypto/cavium/cpt/cptvf_main.c| 6 ++-- drivers/crypto/cavium/cpt/cptvf_reqmanager.c | 12 +++ drivers/crypto/cavium/nitrox/nitrox_lib.c | 4 +-- drivers/crypto/cavium/zip/zip_crypto.c| 6 ++-- drivers/crypto/ccp/ccp-crypto-rsa.c | 6 ++-- drivers/crypto/ccree/cc_aead.c| 4 +-- drivers/crypto/ccree/cc_buffer_mgr.c | 4 +-- drivers/crypto/ccree/cc_cipher.c | 6 ++-- drivers/crypto/ccree/cc_hash.c| 8 ++--- drivers/crypto/ccree/cc_request_mgr.c | 2 +- drivers/crypto/marvell/cesa/hash.c| 2 +- .../crypto/marvell/octeontx/otx_cptvf_main.c | 6 ++-- .../marvell/octeontx/otx_cptvf_reqmgr.h | 2 +- drivers/crypto/mediatek/mtk-aes.c | 2 +- drivers/crypto/nx/nx.c| 4 +-- drivers/crypto/virtio/virtio_crypto_algs.c| 12 +++ drivers/crypto/virtio/virtio_crypto_core.c| 2 +- drivers/md/dm-crypt.c | 34 +-- drivers/md/dm-integrity.c | 6 ++-- drivers/misc/ibmvmc.c | 6 ++-- .../hisilicon/hns3/hns3pf/hclge_mbx.c | 2 +- .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c| 6 ++-- drivers/net/ppp/ppp_mppe.c| 6 ++-- drivers/net/wireguard/noise.c | 4 +-- drivers/net/wireguard/peer.c | 2 +- drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 2 +- .../net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 6 ++-- drivers/net/wireless/intel/iwlwifi/pcie/tx.c | 6 ++-- drivers/net/wireless/intersil/orinoco/wext.c | 4 +-- drivers/s390/crypto/ap_bus.h | 4 +-- drivers/staging/ks7010/ks_hostif.c| 2 +- drivers/staging/rtl8723bs/core/rtw_security.c | 2 +- drivers/staging/wlan-ng/p80211netdev.c| 2 +- drivers/target/iscsi/iscsi_target_auth.c | 2 +- fs/btrfs/ioctl.c | 2 +- fs/cifs/cifsencrypt.c | 2 +- fs/cifs/connect.c | 10 +++--- fs/cifs/dfs_cache.c | 2 +- fs/cifs/misc.c| 8 ++--- fs/crypto/keyring.c | 6 ++-- fs/crypto/keysetup_v1.c | 4 +-- fs/ecryptfs/keystore.c| 4 +-- fs/ecryptfs/messaging.c | 2 +-
[Cocci] [PATCH 0/2] mm, treewide: Rename kzfree() to kfree_sensitive()
This patchset makes a global rename of the kzfree() to kfree_sensitive() to highlight the fact buffer clearing is only needed if the data objects contain sensitive information like encrpytion key. The fact that kzfree() uses memset() to do the clearing isn't totally safe either as compiler may compile out the clearing in their optimizer. Instead, the new kfree_sensitive() uses memzero_explicit() which won't get compiled out. Waiman Long (2): mm, treewide: Rename kzfree() to kfree_sensitive() crypto: Remove unnecessary memzero_explicit() arch/s390/crypto/prng.c | 4 +-- arch/x86/power/hibernate.c| 2 +- crypto/adiantum.c | 2 +- crypto/ahash.c| 4 +-- crypto/api.c | 2 +- crypto/asymmetric_keys/verify_pefile.c| 4 +-- crypto/deflate.c | 2 +- crypto/drbg.c | 10 +++--- crypto/ecc.c | 8 ++--- crypto/ecdh.c | 2 +- crypto/gcm.c | 2 +- crypto/gf128mul.c | 4 +-- crypto/jitterentropy-kcapi.c | 2 +- crypto/rng.c | 2 +- crypto/rsa-pkcs1pad.c | 6 ++-- crypto/seqiv.c| 2 +- crypto/shash.c| 2 +- crypto/skcipher.c | 2 +- crypto/testmgr.c | 6 ++-- crypto/zstd.c | 2 +- .../allwinner/sun8i-ce/sun8i-ce-cipher.c | 17 +++--- .../allwinner/sun8i-ss/sun8i-ss-cipher.c | 18 +++--- drivers/crypto/amlogic/amlogic-gxl-cipher.c | 14 +++- drivers/crypto/atmel-ecc.c| 2 +- drivers/crypto/caam/caampkc.c | 28 +++ drivers/crypto/cavium/cpt/cptvf_main.c| 6 ++-- drivers/crypto/cavium/cpt/cptvf_reqmanager.c | 12 +++ drivers/crypto/cavium/nitrox/nitrox_lib.c | 4 +-- drivers/crypto/cavium/zip/zip_crypto.c| 6 ++-- drivers/crypto/ccp/ccp-crypto-rsa.c | 6 ++-- drivers/crypto/ccree/cc_aead.c| 4 +-- drivers/crypto/ccree/cc_buffer_mgr.c | 4 +-- drivers/crypto/ccree/cc_cipher.c | 6 ++-- drivers/crypto/ccree/cc_hash.c| 8 ++--- drivers/crypto/ccree/cc_request_mgr.c | 2 +- drivers/crypto/inside-secure/safexcel_hash.c | 3 +- drivers/crypto/marvell/cesa/hash.c| 2 +- .../crypto/marvell/octeontx/otx_cptvf_main.c | 6 ++-- .../marvell/octeontx/otx_cptvf_reqmgr.h | 2 +- drivers/crypto/mediatek/mtk-aes.c | 2 +- drivers/crypto/nx/nx.c| 4 +-- drivers/crypto/virtio/virtio_crypto_algs.c| 12 +++ drivers/crypto/virtio/virtio_crypto_core.c| 2 +- drivers/md/dm-crypt.c | 34 +-- drivers/md/dm-integrity.c | 6 ++-- drivers/misc/ibmvmc.c | 6 ++-- .../hisilicon/hns3/hns3pf/hclge_mbx.c | 2 +- .../net/ethernet/intel/ixgbe/ixgbe_ipsec.c| 6 ++-- drivers/net/ppp/ppp_mppe.c| 6 ++-- drivers/net/wireguard/noise.c | 4 +-- drivers/net/wireguard/peer.c | 2 +- drivers/net/wireless/intel/iwlwifi/pcie/rx.c | 2 +- .../net/wireless/intel/iwlwifi/pcie/tx-gen2.c | 6 ++-- drivers/net/wireless/intel/iwlwifi/pcie/tx.c | 6 ++-- drivers/net/wireless/intersil/orinoco/wext.c | 4 +-- drivers/s390/crypto/ap_bus.h | 4 +-- drivers/staging/ks7010/ks_hostif.c| 2 +- drivers/staging/rtl8723bs/core/rtw_security.c | 2 +- drivers/staging/wlan-ng/p80211netdev.c| 2 +- drivers/target/iscsi/iscsi_target_auth.c | 2 +- fs/btrfs/ioctl.c | 2 +- fs/cifs/cifsencrypt.c | 2 +- fs/cifs/connect.c | 10 +++--- fs/cifs/dfs_cache.c | 2 +- fs/cifs/misc.c| 8 ++--- fs/crypto/keyring.c | 6 ++-- fs/crypto/keysetup_v1.c | 4 +-- fs/ecryptfs/keystore.c| 4 +-- fs/ecryptfs/messaging.c | 2 +- include/crypto/aead.h | 2 +- include/crypto/akcipher.h | 2 +- include/crypto/gf128mul.h | 2 +- include/crypto/hash.h | 2 +- include/crypto/internal/acompress.h | 2 +- include/crypto/kpp.h | 2 +- include/crypto/skcipher.h | 2 +- include/linux/slab.h | 2 +- lib/mpi/mpiutil.c | 6 ++-- lib/test_kasan.c
Re: [Cocci] [PATCH 2/2] crypto: Remove unnecessary memzero_explicit()
On Mon, 2020-04-13 at 17:15 -0400, Waiman Long wrote:
> Since kfree_sensitive() will do an implicit memzero_explicit(), there
> is no need to call memzero_explicit() before it. Eliminate those
> memzero_explicit() and simplify the call sites.
2 bits of trivia:
> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
[]
> @@ -391,10 +388,7 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm,
> const u8 *key,
> dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
> return -EINVAL;
> }
> - if (op->key) {
> - memzero_explicit(op->key, op->keylen);
> - kfree(op->key);
> - }
> + kfree_sensitive(op->key);
> op->keylen = keylen;
> op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
> if (!op->key)
It might be a defect to set op->keylen before the kmemdup succeeds.
> @@ -416,10 +410,7 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm,
> const u8 *key,
> if (err)
> return err;
>
> - if (op->key) {
> - memzero_explicit(op->key, op->keylen);
> - kfree(op->key);
> - }
> + free_sensitive(op->key, op->keylen);
Why not kfree_sensitive(op->key) ?
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH 2/2] crypto: Remove unnecessary memzero_explicit()
On 4/13/20 5:31 PM, Joe Perches wrote:
> On Mon, 2020-04-13 at 17:15 -0400, Waiman Long wrote:
>> Since kfree_sensitive() will do an implicit memzero_explicit(), there
>> is no need to call memzero_explicit() before it. Eliminate those
>> memzero_explicit() and simplify the call sites.
> 2 bits of trivia:
>
>> diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
>> b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
> []
>> @@ -391,10 +388,7 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm,
>> const u8 *key,
>> dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
>> return -EINVAL;
>> }
>> -if (op->key) {
>> -memzero_explicit(op->key, op->keylen);
>> -kfree(op->key);
>> -}
>> +kfree_sensitive(op->key);
>> op->keylen = keylen;
>> op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
>> if (!op->key)
> It might be a defect to set op->keylen before the kmemdup succeeds.
It could be. I can move it down after the op->key check.
>> @@ -416,10 +410,7 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm,
>> const u8 *key,
>> if (err)
>> return err;
>>
>> -if (op->key) {
>> -memzero_explicit(op->key, op->keylen);
>> -kfree(op->key);
>> -}
>> +free_sensitive(op->key, op->keylen);
> Why not kfree_sensitive(op->key) ?
Oh, it is a bug. I will send out v2 to fix that.
Thanks for spotting it.
Cheers,
Longman
>
>
___
Cocci mailing list
Cocci@systeme.lip6.fr
https://systeme.lip6.fr/mailman/listinfo/cocci
Re: [Cocci] [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()
Le 14/04/2020 à 00:28, Waiman Long a écrit :
Since kfree_sensitive() will do an implicit memzero_explicit(), there
is no need to call memzero_explicit() before it. Eliminate those
memzero_explicit() and simplify the call sites. For better correctness,
the setting of keylen is also moved down after the key pointer check.
Signed-off-by: Waiman Long
---
.../allwinner/sun8i-ce/sun8i-ce-cipher.c | 19 +-
.../allwinner/sun8i-ss/sun8i-ss-cipher.c | 20 +--
drivers/crypto/amlogic/amlogic-gxl-cipher.c | 12 +++
drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
4 files changed, 14 insertions(+), 40 deletions(-)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index aa4e8fdc2b32..8358fac98719 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync_suspend(op->ce->dev);
}
@@ -391,14 +388,11 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm,
const u8 *key,
dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+ op->keylen = keylen;
Does it matter at all to ensure op->keylen is not set when of->key is
NULL ? I'm not sure.
But if it does, then op->keylen should be set to 0 when freeing op->key.
crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);
crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags &
CRYPTO_TFM_REQ_MASK);
@@ -416,14 +410,11 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
if (err)
return err;
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+ op->keylen = keylen;
Same comment as above.
crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);
crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags &
CRYPTO_TFM_REQ_MASK);
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index 5246ef4f5430..0495fbc27fcc 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -249,7 +249,6 @@ static int sun8i_ss_cipher(struct skcipher_request *areq)
offset = areq->cryptlen - ivsize;
if (rctx->op_dir & SS_DECRYPTION) {
memcpy(areq->iv, backup_iv, ivsize);
- memzero_explicit(backup_iv, ivsize);
kfree_sensitive(backup_iv);
} else {
scatterwalk_map_and_copy(areq->iv, areq->dst,
offset,
@@ -367,10 +366,7 @@ void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync(op->ss->dev);
}
@@ -392,14 +388,11 @@ int sun8i_ss_aes_setkey(struct crypto_skcipher *tfm,
const u8 *key,
dev_dbg(ss->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+ op->keylen = keylen;
Same comment as above.
crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);
crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags &
CRYPTO_TFM_REQ_MASK);
@@ -418,14 +411,11 @@ int sun8i_ss_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
return -EINVAL;
}
- if (op->key) {
-
[Cocci] [PATCH 2/2] crypto: Remove unnecessary memzero_explicit()
Since kfree_sensitive() will do an implicit memzero_explicit(), there
is no need to call memzero_explicit() before it. Eliminate those
memzero_explicit() and simplify the call sites.
Signed-off-by: Waiman Long
---
.../crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c | 15 +++
.../crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c | 16 +++-
drivers/crypto/amlogic/amlogic-gxl-cipher.c | 10 ++
drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
4 files changed, 9 insertions(+), 35 deletions(-)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index aa4e8fdc2b32..46c10c7ca6d0 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync_suspend(op->ce->dev);
}
@@ -391,10 +388,7 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm, const
u8 *key,
dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
@@ -416,10 +410,7 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
if (err)
return err;
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ free_sensitive(op->key, op->keylen);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index 5246ef4f5430..7e09a923cbaf 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -249,7 +249,6 @@ static int sun8i_ss_cipher(struct skcipher_request *areq)
offset = areq->cryptlen - ivsize;
if (rctx->op_dir & SS_DECRYPTION) {
memcpy(areq->iv, backup_iv, ivsize);
- memzero_explicit(backup_iv, ivsize);
kfree_sensitive(backup_iv);
} else {
scatterwalk_map_and_copy(areq->iv, areq->dst,
offset,
@@ -367,10 +366,7 @@ void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync(op->ss->dev);
}
@@ -392,10 +388,7 @@ int sun8i_ss_aes_setkey(struct crypto_skcipher *tfm, const
u8 *key,
dev_dbg(ss->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
@@ -418,10 +411,7 @@ int sun8i_ss_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
op->keylen = keylen;
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
diff --git a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
index fd1269900d67..f424397fbba4 100644
--- a/drivers/crypto/amlogic/amlogic-gxl-cipher.c
+++ b/drivers/crypto/amlogic/amlogic-gxl-cipher.c
@@ -341,10 +341,7 @@ void meson_cipher_exit(struct crypto_tfm *tfm)
{
struct meson_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key)
crypto_free_sync_skcipher(op->fallback_tfm);
}
@@ -368,10 +365,7 @@ int meson_aes_setkey(struct crypto_skcipher *tfm, const u8
*key,
dev_dbg(mc->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
-
[Cocci] [PATCH v2 2/2] crypto: Remove unnecessary memzero_explicit()
Since kfree_sensitive() will do an implicit memzero_explicit(), there
is no need to call memzero_explicit() before it. Eliminate those
memzero_explicit() and simplify the call sites. For better correctness,
the setting of keylen is also moved down after the key pointer check.
Signed-off-by: Waiman Long
---
.../allwinner/sun8i-ce/sun8i-ce-cipher.c | 19 +-
.../allwinner/sun8i-ss/sun8i-ss-cipher.c | 20 +--
drivers/crypto/amlogic/amlogic-gxl-cipher.c | 12 +++
drivers/crypto/inside-secure/safexcel_hash.c | 3 +--
4 files changed, 14 insertions(+), 40 deletions(-)
diff --git a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
index aa4e8fdc2b32..8358fac98719 100644
--- a/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ce/sun8i-ce-cipher.c
@@ -366,10 +366,7 @@ void sun8i_ce_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync_suspend(op->ce->dev);
}
@@ -391,14 +388,11 @@ int sun8i_ce_aes_setkey(struct crypto_skcipher *tfm,
const u8 *key,
dev_dbg(ce->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+ op->keylen = keylen;
crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);
crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags &
CRYPTO_TFM_REQ_MASK);
@@ -416,14 +410,11 @@ int sun8i_ce_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
if (err)
return err;
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+ op->keylen = keylen;
crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);
crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags &
CRYPTO_TFM_REQ_MASK);
diff --git a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
index 5246ef4f5430..0495fbc27fcc 100644
--- a/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
+++ b/drivers/crypto/allwinner/sun8i-ss/sun8i-ss-cipher.c
@@ -249,7 +249,6 @@ static int sun8i_ss_cipher(struct skcipher_request *areq)
offset = areq->cryptlen - ivsize;
if (rctx->op_dir & SS_DECRYPTION) {
memcpy(areq->iv, backup_iv, ivsize);
- memzero_explicit(backup_iv, ivsize);
kfree_sensitive(backup_iv);
} else {
scatterwalk_map_and_copy(areq->iv, areq->dst,
offset,
@@ -367,10 +366,7 @@ void sun8i_ss_cipher_exit(struct crypto_tfm *tfm)
{
struct sun8i_cipher_tfm_ctx *op = crypto_tfm_ctx(tfm);
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
+ kfree_sensitive(op->key);
crypto_free_sync_skcipher(op->fallback_tfm);
pm_runtime_put_sync(op->ss->dev);
}
@@ -392,14 +388,11 @@ int sun8i_ss_aes_setkey(struct crypto_skcipher *tfm,
const u8 *key,
dev_dbg(ss->dev, "ERROR: Invalid keylen %u\n", keylen);
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return -ENOMEM;
+ op->keylen = keylen;
crypto_sync_skcipher_clear_flags(op->fallback_tfm, CRYPTO_TFM_REQ_MASK);
crypto_sync_skcipher_set_flags(op->fallback_tfm, tfm->base.crt_flags &
CRYPTO_TFM_REQ_MASK);
@@ -418,14 +411,11 @@ int sun8i_ss_des3_setkey(struct crypto_skcipher *tfm,
const u8 *key,
return -EINVAL;
}
- if (op->key) {
- memzero_explicit(op->key, op->keylen);
- kfree(op->key);
- }
- op->keylen = keylen;
+ kfree_sensitive(op->key);
op->key = kmemdup(key, keylen, GFP_KERNEL | GFP_DMA);
if (!op->key)
return
