diff mbox series

crypto: lib/chachapoly - Drop dependency on CRYPTO_ALGAPI

Message ID 20250227123338.3033402-1-ardb+git@google.com (mailing list archive)
State Superseded
Delegated to: Herbert Xu
Headers show
Series crypto: lib/chachapoly - Drop dependency on CRYPTO_ALGAPI | expand

Commit Message

Ard Biesheuvel Feb. 27, 2025, 12:33 p.m. UTC
From: Ard Biesheuvel <ardb@kernel.org>

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 <arnd@arndb.de>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 lib/crypto/Kconfig            | 1 -
 lib/crypto/chacha20poly1305.c | 5 ++---
 2 files changed, 2 insertions(+), 4 deletions(-)

Comments

Arnd Bergmann Feb. 27, 2025, 12:59 p.m. UTC | #1
On Thu, Feb 27, 2025, at 13:33, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
>
> 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 <arnd@arndb.de>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Thanks for the fixup!

> ---
>  lib/crypto/Kconfig            | 1 -
>  lib/crypto/chacha20poly1305.c | 5 ++---
>  2 files changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
> index b01253cac70a..a759e6f6a939 100644
> --- a/lib/crypto/Kconfig
> +++ b/lib/crypto/Kconfig
> @@ -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
Eric Biggers Feb. 28, 2025, 3:24 a.m. UTC | #2
On Thu, Feb 27, 2025 at 01:33:38PM +0100, Ard Biesheuvel wrote:
> From: Ard Biesheuvel <ardb@kernel.org>
> 
> 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 <arnd@arndb.de>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Acked-by: Eric Biggers <ebiggers@kernel.org>

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/20250219182341.43961-1-ebiggers@kernel.org/
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.

> diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
> index b01253cac70a..a759e6f6a939 100644
> --- a/lib/crypto/Kconfig
> +++ b/lib/crypto/Kconfig
> @@ -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.

> diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
> index a839c0ac60b2..280a4925dd17 100644
> --- a/lib/crypto/chacha20poly1305.c
> +++ b/lib/crypto/chacha20poly1305.c
> @@ -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
Herbert Xu Feb. 28, 2025, 4:14 a.m. UTC | #3
On Thu, Feb 27, 2025 at 01:59:42PM +0100, Arnd Bergmann wrote:
>
> 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,
Herbert Xu Feb. 28, 2025, 4:19 a.m. UTC | #4
On Thu, Feb 27, 2025 at 01:33:38PM +0100, Ard Biesheuvel wrote:
>
> diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
> index b01253cac70a..a759e6f6a939 100644
> --- a/lib/crypto/Kconfig
> +++ b/lib/crypto/Kconfig
> @@ -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.

> diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
> index a839c0ac60b2..280a4925dd17 100644
> --- a/lib/crypto/chacha20poly1305.c
> +++ b/lib/crypto/chacha20poly1305.c
> @@ -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,
diff mbox series

Patch

diff --git a/lib/crypto/Kconfig b/lib/crypto/Kconfig
index b01253cac70a..a759e6f6a939 100644
--- a/lib/crypto/Kconfig
+++ b/lib/crypto/Kconfig
@@ -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
diff --git a/lib/crypto/chacha20poly1305.c b/lib/crypto/chacha20poly1305.c
index a839c0ac60b2..280a4925dd17 100644
--- a/lib/crypto/chacha20poly1305.c
+++ b/lib/crypto/chacha20poly1305.c
@@ -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);
 	}