Skip to contentSkip to conversation

crypto: lib/chachapoly - Drop dependency on CRYPTO_ALGAPI

PATCH Superseded Ard Biesheuvel 4 participants 5 messages Feb 27, 2025 – Feb 28, 2025

Subsystems (2)
Crypto API Crypto library

v1 of 2 Superseded Latest is v2 →

lore
Share & tools
lore

Ard Biesheuvel

author
#0 lore raw

PATCH 1/1 crypto: lib/chachapoly - Drop dependency on CRYPTO_ALGAPI

From: Ard Biesheuvel <[email protected]>

The ChaCha20-Poly1305 library code uses the sg_miter API to process input presented via scatterlists, except for the special case where the digest buffer is not covered entirely by the same scatterlist entry as the last byte of input. In that case, it uses scatterwalk_map_and_copy() to access the memory in the input scatterlist where the digest is stored.

This results in a dependency on crypto/scatterwalk.c and therefore on CONFIG_CRYPTO_ALGAPI, which is unnecessary, as the sg_miter API already provides this functionality via sg_copy_to_buffer(). So use that instead, and drop the CRYPTO_ALGAPI dependency.

Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
---
 lib/crypto/Kconfig            | 1 -
 lib/crypto/chacha20poly1305.c | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)
lib/crypto/Kconfig +0 -1
index b01253cac70a..a759e6f6a939 100644
@@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305
 	depends on CRYPTO
 	select CRYPTO_LIB_CHACHA
 	select CRYPTO_LIB_POLY1305
-	select CRYPTO_ALGAPI
 
 config CRYPTO_LIB_SHA1
 	tristate
lib/crypto/chacha20poly1305.c +2 -3
index a839c0ac60b2..280a4925dd17 100644
@@ -11,7 +11,6 @@
 #include <crypto/chacha20poly1305.h>
 #include <crypto/chacha.h>
 #include <crypto/poly1305.h>
-#include <crypto/scatterwalk.h>
 
 #include <linux/unaligned.h>
 #include <linux/kernel.h>

@@ -318,8 +317,8 @@ bool chacha20poly1305_crypt_sg_inplace(struct scatterlist *src,
 
 	if (unlikely(sl > -POLY1305_DIGEST_SIZE)) {
 		poly1305_final(&poly1305_state, b.mac[1]);
-		scatterwalk_map_and_copy(b.mac[encrypt], src, src_len,
-					 sizeof(b.mac[1]), encrypt);
+		sg_copy_buffer(src, sg_nents(src), b.mac[encrypt],
+			       sizeof(b.mac[1]), src_len, !encrypt);
 		ret = encrypt ||
 		      !crypto_memneq(b.mac[0], b.mac[1], POLY1305_DIGEST_SIZE);
 	}
Signature · 1 line
2.48.1.711.g2feabab25a-goog
#1 lore raw
Quoted 8 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

The ChaCha20-Poly1305 library code uses the sg_miter API to process input presented via scatterlists, except for the special case where the digest buffer is not covered entirely by the same scatterlist entry as the last byte of input. In that case, it uses scatterwalk_map_and_copy() to access the memory in the input scatterlist where the digest is stored.

Quoted 7 lines

This results in a dependency on crypto/scatterwalk.c and therefore on CONFIG_CRYPTO_ALGAPI, which is unnecessary, as the sg_miter API already provides this functionality via sg_copy_to_buffer(). So use that instead, and drop the CRYPTO_ALGAPI dependency.

Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>

Thanks for the fixup!

Quoted 10 lines
---
 lib/crypto/Kconfig            | 1 -
 lib/crypto/chacha20poly1305.c | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)
lib/crypto/Kconfig +0 -0
index b01253cac70a..a759e6f6a939 100644

lib/crypto/Kconfig config CRYPTO_LIB_CHACHA20POLY1305 · line 138

Quoted 4 lines
lib/crypto/Kconfig +0 -1
@@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305
 	depends on CRYPTO
 	select CRYPTO_LIB_CHACHA
 	select CRYPTO_LIB_POLY1305
-	select CRYPTO_ALGAPI

I think importantly, dropping the 'select CRYPTO_ALGAPI' means that the 'depends on CRYPTO' here can also be dropped:
CRYPTO_LIB_CHACHA20POLY1305 needs nothing else from the crypto subsystem. Moreover, CONFIG_WIREGUARD can now drop the 'select CRYPTO' because it seems to only need this in order to 'select CRYPTO_LIB_CHACHA20POLY1305'.

Arnd

maintainer
#2 lore raw
Quoted 7 lines · Arnd Bergmann

I think importantly, dropping the 'select CRYPTO_ALGAPI' means that the 'depends on CRYPTO' here can also be dropped:
CRYPTO_LIB_CHACHA20POLY1305 needs nothing else from the crypto subsystem. Moreover, CONFIG_WIREGUARD can now drop the 'select CRYPTO' because it seems to only need this in order to 'select CRYPTO_LIB_CHACHA20POLY1305'.

Yes you can drop it from CHACHA20POLY1305, but you must keep the 'select CRYPTO' on the individual CHACHA and POLY1305 LIB options as otherwise the arch options cannot be selected at all since they're all under CRYPTO.

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
maintainer Acked-by on v1 1/1
#3 lore raw
In reply to Ard Biesheuvel
Quoted 8 lines · Ard Biesheuvel

From: Ard Biesheuvel <[email protected]>

The ChaCha20-Poly1305 library code uses the sg_miter API to process input presented via scatterlists, except for the special case where the digest buffer is not covered entirely by the same scatterlist entry as the last byte of input. In that case, it uses scatterwalk_map_and_copy() to access the memory in the input scatterlist where the digest is stored.

Quoted 7 lines

This results in a dependency on crypto/scatterwalk.c and therefore on CONFIG_CRYPTO_ALGAPI, which is unnecessary, as the sg_miter API already provides this functionality via sg_copy_to_buffer(). So use that instead, and drop the CRYPTO_ALGAPI dependency.

Reported-by Arnd Bergmann <[email protected]>
Signed-off-by Ard Biesheuvel <[email protected]>
Acked-by Eric Biggers <[email protected]>

There's indeed redundancy between crypto/scatterwalk.c and lib/scatterlist.c, and switching to lib/scatterlist.c makes sense here. I do think that the implementation in crypto/scatterwalk.c is slightly better (including being slightly more efficient), especially if my patchset
https://lore.kernel.org/linux-crypto/[email protected]/
gets applied. It would be a good future project to unify them into a single version, which of course would not depend on CRYPTO. But for now, just using the one that already does not depend on CRYPTO here is the right choice.

lib/crypto/Kconfig config CRYPTO_LIB_CHACHA20POLY1305 · line 138

Quoted 9 lines
lib/crypto/Kconfig +0 -1
index b01253cac70a..a759e6f6a939 100644
@@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305
 	depends on CRYPTO
 	select CRYPTO_LIB_CHACHA
 	select CRYPTO_LIB_POLY1305
-	select CRYPTO_ALGAPI

As Arnd mentioned, 'depends on CRYPTO' should be dropped.

lib/crypto/chacha20poly1305.c line 15

Quoted 10 lines
lib/crypto/chacha20poly1305.c +0 -1
index a839c0ac60b2..280a4925dd17 100644
@@ -11,7 +11,6 @@
 #include <crypto/algapi.h>
 #include <crypto/chacha20poly1305.h>
 #include <crypto/chacha.h>
 #include <crypto/poly1305.h>
-#include <crypto/scatterwalk.h>

Also replace the include of <crypto/algapi.h> with <crypto/utils.h>, for consistency with the removal of the selection of CRYPTO_ALGAPI.

- Eric

maintainer
#4 lore raw
In reply to Ard Biesheuvel

lib/crypto/Kconfig config CRYPTO_LIB_CHACHA20POLY1305 · line 138

Quoted 10 lines · Ard Biesheuvel
lib/crypto/Kconfig +0 -1
index b01253cac70a..a759e6f6a939 100644
@@ -135,7 +135,6 @@ config CRYPTO_LIB_CHACHA20POLY1305
 	depends on CRYPTO
 	select CRYPTO_LIB_CHACHA
 	select CRYPTO_LIB_POLY1305
-	select CRYPTO_ALGAPI

You should add CRYPTO_LIB_UTILS. It's coming indirectly through CRYPTO but as Arnd suggested that can now be removed.

lib/crypto/chacha20poly1305.c line 14

Quoted 9 lines
lib/crypto/chacha20poly1305.c +0 -1
index a839c0ac60b2..280a4925dd17 100644
@@ -11,7 +11,6 @@
 #include <crypto/chacha20poly1305.h>
 #include <crypto/chacha.h>
 #include <crypto/poly1305.h>
-#include <crypto/scatterwalk.h>

Please also replace algapi.h with utils.h.

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