Skip to contentSkip to conversation

ima: remove buggy support for asynchronous hashes

PATCH In review Eric Biggers 3 participants 4 messages Mar 12 – Mar 17

Subsystems (6)
lore
Share & tools
lore
#0 lore raw

PATCH 1/1 ima: remove buggy support for asynchronous hashes

IMA computes hashes using the crypto_shash or crypto_ahash API. The latter is used only when ima.ahash_minsize is set on the command line, and its purpose is ostensibly to make the hash computation faster.

However, going off the CPU to a crypto engine and back again is actually quite slow, especially compared with the acceleration that is built into modern CPUs and the kernel now enables by default for most algorithms. Typical performance results for SHA-256 on a modern platform can be found at https://lore.kernel.org/linux-crypto/20250615184638.GA1480@sol/

Partly for this reason, several other kernel subsystems have already dropped support for the crypto_ahash API.

The other problem with crypto_ahash is that bugs are also common, not just in the underlying drivers, but also in the code using it, since it is very difficult to use correctly. Just from a quick review, here are some of the bugs I noticed in IMA's ahash code:

- [Use after free] ima_alloc_atfm() isn't thread-safe and can trigger a use-after-free if multiple threads try to initialize the global ima_ahash_tfm at the same time.

- [Deadlock] If only one buffer is allocated and there is an error reading from the file, then ahash_wait() is executed twice, causing a deadlock in wait_for_completion().

- [Crash or incorrect hash computed] calc_buffer_ahash_atfm() is sometimes passed stack buffers which can be vmalloc addresses, but it puts them in a scatterlist assuming they are linear addresses. This causes the hashing to be done on the wrong physical address.

- [Truncation to 32-bit length] ima_alloc_pages() incorrectly assumes an loff_t value fits in an unsigned long. calc_buffer_ahash_atfm() incorrectly assumes that a loff_t value fits in an unsigned int.

So, not exactly a great track record so far, even disregarding driver bugs which are an even larger problem. Fortunately, in practice it's unlikely that many users are actually setting the ima.ahash_minsize kernel command-line parameter which enables this code. However, given that this code is almost certainly no longer useful (if it ever was), let's just remove it instead of attempting to fix all these issues.

---
 .../admin-guide/kernel-parameters.txt         |  17 -
 security/integrity/ima/ima_crypto.c           | 382 +-----------------
 2 files changed, 9 insertions(+), 390 deletions(-)
Documentation/admin-guide/kernel-parameters.txt +0 -17
index 55ffc0f8858a..a6e75f0a293b 100644
@@ -2393,27 +2393,10 @@ Kernel parameters
 
 	ima_template_fmt=
 			[IMA] Define a custom template format.
 			Format: { "field1|...|fieldN" }
 
-	ima.ahash_minsize= [IMA] Minimum file size for asynchronous hash usage
-			Format: <min_file_size>
-			Set the minimal file size for using asynchronous hash.
-			If left unspecified, ahash usage is disabled.
-
-			ahash performance varies for different data sizes on
-			different crypto accelerators. This option can be used
-			to achieve the best performance for a particular HW.
-
-	ima.ahash_bufsize= [IMA] Asynchronous hash buffer size
-			Format: <bufsize>
-			Set hashing buffer size. Default: 4k.
-
-			ahash performance varies for different chunk sizes on
-			different crypto accelerators. This option can be used
-			to achieve best performance for particular HW.
-
 	ima=		[IMA] Enable or disable IMA
 			Format: { "off" | "on" }
 			Default: "on"
 			Note that disabling IMA is limited to kdump kernel.
 
security/integrity/ima/ima_crypto.c +9 -373

470 lines not on this page · raw message

base-commit: 1f318b96cc84d7c2ab792fcc0bfd42a7ca890681

Signature · 1 line
2.53.0
maintainer
#1 lore raw
Quoted 4 lines · Eric Biggers

IMA computes hashes using the crypto_shash or crypto_ahash API.  The latter is used only when ima.ahash_minsize is set on the command line, and its purpose is ostensibly to make the hash computation faster.

Quoted 8 lines

However, going off the CPU to a crypto engine and back again is actually quite slow, especially compared with the acceleration that is built into modern CPUs and the kernel now enables by default for most algorithms. Typical performance results for SHA-256 on a modern platform can be found at https://lore.kernel.org/linux-crypto/20250615184638.GA1480@sol/

Partly for this reason, several other kernel subsystems have already dropped support for the crypto_ahash API.

The performance benefit was the ability of reading and filling a buffer from disk, which was slow, while the other buffer was sent to the crypto engine.

I'm all for removing extraneous code. I'll give it a couple of days, before queuing the patch in case there are any objections.

thanks,

Mimi

#2 lore raw
Quoted 11 lines · Mimi Zohar
Quoted 10 lines · Eric Biggers

IMA computes hashes using the crypto_shash or crypto_ahash API.  The latter is used only when ima.ahash_minsize is set on the command line, and its purpose is ostensibly to make the hash computation faster.

However, going off the CPU to a crypto engine and back again is actually quite slow, especially compared with the acceleration that is built into modern CPUs and the kernel now enables by default for most algorithms. Typical performance results for SHA-256 on a modern platform can be found at https://lore.kernel.org/linux-crypto/20250615184638.GA1480@sol/

Quoted 5 lines
Quoted 2 lines

Partly for this reason, several other kernel subsystems have already dropped support for the crypto_ahash API.

The performance benefit was the ability of reading and filling a buffer from disk, which was slow, while the other buffer was sent to the crypto engine.

On normal filesystems, sequential reads from a file already kick off async readahead. So the hashing and disk reads can already happen concurrently anyway.

- Eric

maintainer
#3 lore raw
Quoted 16 lines · Eric Biggers
Quoted 14 lines · Mimi Zohar
Quoted 12 lines · Eric Biggers

IMA computes hashes using the crypto_shash or crypto_ahash API. The latter is used only when ima.ahash_minsize is set on the command line, and its purpose is ostensibly to make the hash computation faster.

However, going off the CPU to a crypto engine and back again is actually quite slow, especially compared with the acceleration that is built into modern CPUs and the kernel now enables by default for most algorithms. Typical performance results for SHA-256 on a modern platform can be found at https://lore.kernel.org/linux-crypto/20250615184638.GA1480@sol/

Partly for this reason, several other kernel subsystems have already dropped support for the crypto_ahash API.

Quoted 8 lines
Quoted 2 lines

The performance benefit was the ability of reading and filling a buffer from disk, which was slow, while the other buffer was sent to the crypto engine.

On normal filesystems, sequential reads from a file already kick off async readahead. So the hashing and disk reads can already happen concurrently anyway.

- Eric

I think this is fine. It was developed to be used on OMAP processors a long time ago.
It can go away..

Ack