Skip to contentSkip to conversation

crypto: hisilicon: Wipe entire pool on error

PATCH In review Kees Cook 3 participants 3 messages Jan 6, 2023 – Jan 7, 2023

Subsystems (2)
Crypto API HiSilicon qm driver
lore
Share & tools
lore

Kees Cook

author
#0 lore raw

PATCH 1/1 crypto: hisilicon: Wipe entire pool on error

To work around a Clang __builtin_object_size bug that shows up under CONFIG_FORTIFY_SOURCE and UBSAN_BOUNDS, move the per-loop-iteration mem_block wipe into a single wipe of the entire pool structure after the loop.

Reported-by Nathan Chancellor <[email protected]>
Link https://github.com/ClangBuiltLinux/linux/issues/1780
Cc Weili Qian <[email protected]>
Cc Zhou Wang <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/crypto/hisilicon/sgl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/crypto/hisilicon/sgl.c +1 -2
index 2b6f2281cfd6..0974b0041405 100644
@@ -124,9 +124,8 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev,
 	for (j = 0; j < i; j++) {
 		dma_free_coherent(dev, block_size, block[j].sgl,
 				  block[j].sgl_dma);
-		memset(block + j, 0, sizeof(*block));
 	}
-	kfree(pool);
+	kfree_sensitive(pool);
 	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL_GPL(hisi_acc_create_sgl_pool);
Signature · 1 line
2.34.1
Tested-by on v1 1/1
#1 lore raw
Quoted 5 lines · Kees Cook

To work around a Clang __builtin_object_size bug that shows up under CONFIG_FORTIFY_SOURCE and UBSAN_BOUNDS, move the per-loop-iteration mem_block wipe into a single wipe of the entire pool structure after the loop.

Quoted 8 lines
Reported-by Nathan Chancellor <[email protected]>
Link https://github.com/ClangBuiltLinux/linux/issues/1780
Cc Weili Qian <[email protected]>
Cc Zhou Wang <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>

Thanks for the patch!

Tested-by Nathan Chancellor <[email protected]> # build
Quoted 22 lines
---
 drivers/crypto/hisilicon/sgl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/crypto/hisilicon/sgl.c +1 -2
index 2b6f2281cfd6..0974b0041405 100644
@@ -124,9 +124,8 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev,
 	for (j = 0; j < i; j++) {
 		dma_free_coherent(dev, block_size, block[j].sgl,
 				  block[j].sgl_dma);
-		memset(block + j, 0, sizeof(*block));
 	}
-	kfree(pool);
+	kfree_sensitive(pool);
 	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL_GPL(hisi_acc_create_sgl_pool);
Signature · 1 line
2.34.1
maintainer
#2 lore raw
In reply to Kees Cook
Quoted 23 lines · Kees Cook

To work around a Clang __builtin_object_size bug that shows up under CONFIG_FORTIFY_SOURCE and UBSAN_BOUNDS, move the per-loop-iteration mem_block wipe into a single wipe of the entire pool structure after the loop.

Reported-by Nathan Chancellor <[email protected]>
Link https://github.com/ClangBuiltLinux/linux/issues/1780
Cc Weili Qian <[email protected]>
Cc Zhou Wang <[email protected]>
Cc Herbert Xu <[email protected]>
Cc "David S. Miller" <[email protected]>
Cc [email protected]
Signed-off-by Kees Cook <[email protected]>
---
 drivers/crypto/hisilicon/sgl.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/crypto/hisilicon/sgl.c +0 -0
index 2b6f2281cfd6..0974b0041405 100644

drivers/crypto/hisilicon/sgl.c struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev, · line 131

Quoted 10 lines
drivers/crypto/hisilicon/sgl.c +1 -2
@@ -124,9 +124,8 @@ struct hisi_acc_sgl_pool *hisi_acc_create_sgl_pool(struct device *dev,
 	for (j = 0; j < i; j++) {
 		dma_free_coherent(dev, block_size, block[j].sgl,
 				  block[j].sgl_dma);
-		memset(block + j, 0, sizeof(*block));
 	}
-	kfree(pool);
+	kfree_sensitive(pool);
 	return ERR_PTR(-ENOMEM);
 }
 EXPORT_SYMBOL_GPL(hisi_acc_create_sgl_pool);

Thanks for your patch.

There is no sensitive data in the pool, so memset zero can be deleted directly.

Thanks,
Weili