Skip to contentSkip to conversation

crypto: caam - Fix DMA corruption on long hmac keys

PATCH In review Herbert Xu 2 participants 4 messages Mar 9 – Mar 24

Subsystems (2)
Crypto API Freescale CAAM (Cryptographic Acceleration and Assurance Module) driver
lore
Share & tools
lore

Herbert Xu

author maintainer
#0 lore raw

PATCH 1/1 crypto: caam - Fix DMA corruption on long hmac keys

When a key longer than block size is supplied, it is copied and then hashed into the real key. The memory allocated for the copy needs to be rounded to DMA cache alignment, as otherwise the hashed key may corrupt neighbouring memory.

The rounding was performed, but never actually used for the allocation. Fix this by using the rounded value when calling kmemdup.

Fixes 199354d7fb6e ("crypto: caam - Remove GFP_DMA and add DMA alignment padding")
Reported-by Paul Bunyan <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
drivers/crypto/caam/caamhash.c +1 -1
index e0a23c55c10e..aded03c8601c 100644
@@ -441,7 +441,7 @@ static int ahash_setkey(struct crypto_ahash *ahash,
 		if (aligned_len < keylen)
 			return -EOVERFLOW;
 
-		hashed_key = kmemdup(key, keylen, GFP_KERNEL);
+		hashed_key = kmemdup(key, aligned_len, GFP_KERNEL);
 		if (!hashed_key)
 			return -ENOMEM;
 		ret = hash_digest_key(ctx, &keylen, hashed_key, digestsize);
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
maintainer
#1 lore raw
Quoted 6 lines · Herbert Xu

When a key longer than block size is supplied, it is copied and then hashed into the real key. The memory allocated for the copy needs to be rounded to DMA cache alignment, as otherwise the hashed key may corrupt neighbouring memory.

The rounding was performed, but never actually used for the allocation.

Thanks for catching this.

Quoted 1 line

Fix this by using the rounded value when calling kmemdup.

Unfortunately, kmemdup can't be used in this case.

Quoted 10 lines
Fixes 199354d7fb6e ("crypto: caam - Remove GFP_DMA and add DMA alignment padding")
Reported-by Paul Bunyan <[email protected]>
Signed-off-by Herbert Xu <[email protected]>
drivers/crypto/caam/caamhash.c +0 -0
index e0a23c55c10e..aded03c8601c 100644

drivers/crypto/caam/caamhash.c static int ahash_setkey(struct crypto_ahash *ahash, · line 444

Quoted 5 lines
drivers/crypto/caam/caamhash.c +1 -1
@@ -441,7 +441,7 @@ static int ahash_setkey(struct crypto_ahash *ahash,
  		if (aligned_len < keylen)
  			return -EOVERFLOW;
 
-		hashed_key = kmemdup(key, keylen, GFP_KERNEL);
+		hashed_key = kmemdup(key, aligned_len, GFP_KERNEL);

When aligned_len is bigger than keylen, kmemdup would go beyond the end of "key" buffer.

Looks like kzalloc + memcpy should be used instead of kmemdup.

Double checking the faulty commit, I've found the same pattern in ahash_setkey from drivers/crypto/caam/caamalg_qi2.c. Let me know if you'd send a fix for this or I should do it.

Thanks,
Horia

author maintainer
#2 lore raw
Quoted 14 lines · Horia Geantă

drivers/crypto/caam/caamhash.c static int ahash_setkey(struct crypto_ahash *ahash, · line 443

Quoted 10 lines
drivers/crypto/caam/caamhash.c +1 -1
index e0a23c55c10e..aded03c8601c 100644
@@ -441,7 +441,7 @@ static int ahash_setkey(struct crypto_ahash *ahash,
  		if (aligned_len < keylen)
  			return -EOVERFLOW;
-		hashed_key = kmemdup(key, keylen, GFP_KERNEL);
+		hashed_key = kmemdup(key, aligned_len, GFP_KERNEL);

When aligned_len is bigger than keylen, kmemdup would go beyond the end of "key" buffer.

Quoted 5 lines

Looks like kzalloc + memcpy should be used instead of kmemdup.

Double checking the faulty commit, I've found the same pattern in ahash_setkey from drivers/crypto/caam/caamalg_qi2.c. Let me know if you'd send a fix for this or I should do it.

Good catch. Please feel free to send a patch for both issues.

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 maintainer
#3 lore raw
In reply to Horia Geantă
Quoted 9 lines · Horia Geantă

When aligned_len is bigger than keylen, kmemdup would go beyond the end of "key" buffer.

Looks like kzalloc + memcpy should be used instead of kmemdup.

Double checking the faulty commit, I've found the same pattern in ahash_setkey from drivers/crypto/caam/caamalg_qi2.c. Let me know if you'd send a fix for this or I should do it.

Hi Horia:

While we should still fix this, it turns out that the original problem went beyond the memory allocation.

The root cause of the self-test failure is actually caused by the parallel self-test mechanism which became the default in 2024.

It appears the caam is unable to deal with this because the ahash_setkey function will fail if caam_jr_enqueue returns ENOSPC, which is bound to happen sooner or later in a parallel environment.

So it'll either need to deal with ENOSPC, or switch to a software fallback.

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