Skip to contentSkip to conversation

crypto: caam - remove HMAC key hex dumps from hash_digest_key

PATCH Superseded Thorsten Blum 2 participants 7 messages Mar 6 – Mar 18

Subsystems (3)
Stable branch Crypto API Freescale CAAM (Cryptographic Acceleration and Assurance Module) driver

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Thorsten Blum

author
#0 lore raw

PATCH 1/1 crypto: caam - remove HMAC key hex dumps from hash_digest_key

Stop dumping sensitive HMAC key bytes (original and reduced keys) in hash_digest_key() to avoid leaking secrets when debug logging is enabled.

Fixes 045e36780f11 ("crypto: caam - ahash hmac support")
Fixes 3f16f6c9d632 ("crypto: caam/qi2 - add support for ahash algorithms")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/crypto/caam/caamalg_qi2.c | 5 -----
 drivers/crypto/caam/caamhash.c    | 6 ------
 2 files changed, 11 deletions(-)
drivers/crypto/caam/caamalg_qi2.c +0 -5
index 167372936ca7..b4a6e4637b79 100644
@@ -3269,8 +3269,6 @@ static int hash_digest_key(struct caam_hash_ctx *ctx, u32 *keylen, u8 *key,
 	dpaa2_fl_set_addr(out_fle, key_dma);
 	dpaa2_fl_set_len(out_fle, digestsize);
 
-	print_hex_dump_debug("key_in@" __stringify(__LINE__)": ",
-			     DUMP_PREFIX_ADDRESS, 16, 4, key, *keylen, 1);
 	print_hex_dump_debug("shdesc@" __stringify(__LINE__)": ",
 			     DUMP_PREFIX_ADDRESS, 16, 4, desc, desc_bytes(desc),
 			     1);

@@ -3289,9 +3287,6 @@ static int hash_digest_key(struct caam_hash_ctx *ctx, u32 *keylen, u8 *key,
 		/* in progress */
 		wait_for_completion(&result.completion);
 		ret = result.err;
-		print_hex_dump_debug("digested key@" __stringify(__LINE__)": ",
-				     DUMP_PREFIX_ADDRESS, 16, 4, key,
-				     digestsize, 1);
 	}
 
 	dma_unmap_single(ctx->dev, flc_dma, sizeof(flc->flc) + desc_bytes(desc),
drivers/crypto/caam/caamhash.c +0 -6
index 628c43a7efc4..70c252f167c5 100644
@@ -393,8 +393,6 @@ static int hash_digest_key(struct caam_hash_ctx *ctx, u32 *keylen, u8 *key,
 	append_seq_store(desc, digestsize, LDST_CLASS_2_CCB |
 			 LDST_SRCDST_BYTE_CONTEXT);
 
-	print_hex_dump_debug("key_in@"__stringify(__LINE__)": ",
-			     DUMP_PREFIX_ADDRESS, 16, 4, key, *keylen, 1);
 	print_hex_dump_debug("jobdesc@"__stringify(__LINE__)": ",
 			     DUMP_PREFIX_ADDRESS, 16, 4, desc, desc_bytes(desc),
 			     1);

@@ -407,10 +405,6 @@ static int hash_digest_key(struct caam_hash_ctx *ctx, u32 *keylen, u8 *key,
 		/* in progress */
 		wait_for_completion(&result.completion);
 		ret = result.err;
-
-		print_hex_dump_debug("digested key@"__stringify(__LINE__)": ",
-				     DUMP_PREFIX_ADDRESS, 16, 4, key,
-				     digestsize, 1);
 	}
 	dma_unmap_single(jrdev, key_dma, *keylen, DMA_BIDIRECTIONAL);
Signature · 2 lines
Thorsten Blum <[email protected]>
GPG: 1D60 735E 8AEF 3BE4 73B6  9D84 7336 78FD 8DFE EAD4
maintainer
#1 lore raw
Quoted 4 lines · Thorsten Blum

Stop dumping sensitive HMAC key bytes (original and reduced keys) in hash_digest_key() to avoid leaking secrets when debug logging is enabled.

Quoted 8 lines
Fixes 045e36780f11 ("crypto: caam - ahash hmac support")
Fixes 3f16f6c9d632 ("crypto: caam/qi2 - add support for ahash algorithms")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/crypto/caam/caamalg_qi2.c | 5 -----
 drivers/crypto/caam/caamhash.c    | 6 ------
 2 files changed, 11 deletions(-)

What is the rationale for this? When debugging is enabled, all sorts of things could be dumped, e.g., passwords.

Is there a scenario where production systems will run with debugging enabled in caam?

Cheers,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#2 lore raw
Quoted 14 lines · Herbert Xu
Quoted 4 lines · Thorsten Blum

Stop dumping sensitive HMAC key bytes (original and reduced keys) in hash_digest_key() to avoid leaking secrets when debug logging is enabled.

Quoted 8 lines
Fixes 045e36780f11 ("crypto: caam - ahash hmac support")
Fixes 3f16f6c9d632 ("crypto: caam/qi2 - add support for ahash algorithms")
Cc [email protected]
Signed-off-by Thorsten Blum <[email protected]>
---
 drivers/crypto/caam/caamalg_qi2.c | 5 -----
 drivers/crypto/caam/caamhash.c    | 6 ------
 2 files changed, 11 deletions(-)
Quoted 2 lines

What is the rationale for this? When debugging is enabled, all sorts of things could be dumped, e.g., passwords.

This is not specifically about caam, but (debug) logging of potentially sensitive key material should generally be avoided, imho. Some other recent examples:

https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/

Quoted 2 lines

Is there a scenario where production systems will run with debugging enabled in caam?

I don't know - possibly.

Thanks,
Thorsten

maintainer
#3 lore raw
Quoted 5 lines · Thorsten Blum

This is not specifically about caam, but (debug) logging of potentially sensitive key material should generally be avoided, imho. Some other recent examples:

Quoted 8 lines

https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/

Quoted 2 lines

Is there a scenario where production systems will run with debugging enabled in caam?

I don't know - possibly.

I think a better solution is to turn these sensitive printk's to pr_devel. That way you can still get them by recompiling the kernel but they won't be enabled in any distro kernels.

What do you think?

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#4 lore raw
Quoted 13 lines · Herbert Xu
Quoted 12 lines · Thorsten Blum

This is not specifically about caam, but (debug) logging of potentially sensitive key material should generally be avoided, imho. Some other recent examples:

https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/
https://lore.kernel.org/lkml/[email protected]/

Quoted 2 lines

Is there a scenario where production systems will run with debugging enabled in caam?

Quoted 7 lines
Quoted 1 line

I don't know - possibly.

I think a better solution is to turn these sensitive printk's to pr_devel. That way you can still get them by recompiling the kernel but they won't be enabled in any distro kernels.

What do you think?

Sounds reasonable. However, the code is already using the debug-gated print_hex_dump_debug(), which can also be enabled at runtime with CONFIG_DYNAMIC_DEBUG.

So I think the question is not necessarily print_hex_dump_debug() vs. pr_devel(), but whether we want to:

- keep the debug-only hex dumps
- remove the sensitive dumps

My main concern is that with CONFIG_DYNAMIC_DEBUG enabled, which doesn't require DEBUG, these raw key dumps can still be turned on at runtime in a deployed kernel.

If we want to keep the dumps for debug-only kernels, then #ifdef DEBUG plus print_hex_dump() might be a good compromise.

maintainer
#5 lore raw
Quoted 7 lines · Thorsten Blum

My main concern is that with CONFIG_DYNAMIC_DEBUG enabled, which doesn't require DEBUG, these raw key dumps can still be turned on at runtime in a deployed kernel.

If we want to keep the dumps for debug-only kernels, then #ifdef DEBUG plus print_hex_dump() might be a good compromise.

Exactly. Having sensitive information printed with DYNAMIC_DEBUG is arguably a problem, but putting them behind DEBUG is definitely OK.

Thanks,

Signature · 3 lines
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
author
#6 lore raw
Quoted 6 lines · Herbert Xu
Quoted 5 lines · Thorsten Blum

My main concern is that with CONFIG_DYNAMIC_DEBUG enabled, which doesn't require DEBUG, these raw key dumps can still be turned on at runtime in a deployed kernel.

Quoted 6 lines
Quoted 2 lines

If we want to keep the dumps for debug-only kernels, then #ifdef DEBUG plus print_hex_dump() might be a good compromise.

Exactly. Having sensitive information printed with DYNAMIC_DEBUG is arguably a problem, but putting them behind DEBUG is definitely OK.

Ok, thanks. I'll send a v2 soon using print_hex_dump() guarded by #ifdef DEBUG.