Re: [PATCH net-next v2 1/5] net/tcp-ao: Drop support for most non-RFC-specified algorithms

[Simon Horman <horms@kernel.org>]

From: 'Simon Horman' <horms@kernel.org>

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
Subject: net/tcp-ao: Drop support for most non-RFC-specified algorithms

This change restricts the algorithms accepted by setsockopt(TCP_AO_ADD_KEY)
to cmac(aes128)/cmac(aes)+keylen=16, hmac(sha1) and hmac(sha256), returning
-ENOENT for anything else.  The selftest algorithm table and its Kconfig
fragment are updated to drop FIPS detection and rmd160.

A few questions inline below.

> diff --git a/net/ipv4/tcp_ao.c b/net/ipv4/tcp_ao.c
> index a97cdf3e6af4..b21bd69b4e82 100644
> --- a/net/ipv4/tcp_ao.c
> +++ b/net/ipv4/tcp_ao.c
> @@ -1563,6 +1563,10 @@ static struct tcp_ao_key *tcp_ao_key_alloc(struct sock *sk,
>  	/* RFC5926, 3.1.1.2. KDF_AES_128_CMAC */
>  	if (!strcmp("cmac(aes128)", algo))
>  		algo = "cmac(aes)";
> +	else if (strcmp("hmac(sha1)", algo) &&
> +		 strcmp("hmac(sha256)", algo) &&
> +		 (strcmp("cmac(aes)", algo) || cmd->keylen != 16))
> +		return ERR_PTR(-ENOENT);

Is this a uAPI regression for existing users of TCP_AO_ADD_KEY?

Before this change, tcp_ao_add::alg_name accepted any crypto_ahash
algorithm name.  The commit message itself notes "arbitrary HMAC algorithms
probably do work", and the selftest prior to this patch exercised
hmac(sha512), hmac(sha384), hmac(sha224), hmac(sha3-512), hmac(rmd160) and
hmac(md5), which demonstrates those configurations were functional in
practice.

A setsockopt call that previously succeeded with e.g. alg_name =
"hmac(sha512)" now returns -ENOENT after a kernel upgrade, with no
deprecation window, no Kconfig opt-in, and no runtime toggle.  HMAC-SHA512
in particular seems like a plausible choice that a deployment may already
rely on.

Would it make sense to keep hmac(sha512) (and possibly hmac(sha384)) in the
accepted list, or to gate the removal behind a Kconfig/sysctl while warning
for a few releases?

Also, -ENOENT is the same error returned when the crypto module for an
otherwise-accepted algorithm is simply not built in.  Should a distinct
errno (for example -EOPNOTSUPP) be used here so userspace can tell
"algorithm rejected by policy" apart from "algorithm module missing"?

> diff --git a/tools/testing/selftests/net/tcp_ao/config b/tools/testing/selftests/net/tcp_ao/config
> index f22148512365..47228a7d0b90 100644
> --- a/tools/testing/selftests/net/tcp_ao/config
> +++ b/tools/testing/selftests/net/tcp_ao/config
> @@ -1,6 +1,5 @@
>  CONFIG_CRYPTO_CMAC=y
>  CONFIG_CRYPTO_HMAC=y
> -CONFIG_CRYPTO_RMD160=y
>  CONFIG_CRYPTO_SHA1=y
>  CONFIG_IPV6=y
>  CONFIG_IPV6_MULTIPLE_TABLES=y

Should CONFIG_CRYPTO_SHA256=y (and possibly CONFIG_CRYPTO_AES=y) be added
here?

> diff --git a/tools/testing/selftests/net/tcp_ao/key-management.c b/tools/testing/selftests/net/tcp_ao/key-management.c
> index 69d9a7a05d5c..d86bb380b79f 100644
> --- a/tools/testing/selftests/net/tcp_ao/key-management.c
> +++ b/tools/testing/selftests/net/tcp_ao/key-management.c

[ ... ]

> -const char *test_algos[] = {
> -	"cmac(aes128)",
> -	"hmac(sha1)", "hmac(sha512)", "hmac(sha384)", "hmac(sha256)",
> -	"hmac(sha224)", "hmac(sha3-512)",
> -	/* only if !CONFIG_FIPS */
> -#define TEST_NON_FIPS_ALGOS	2
> -	"hmac(rmd160)", "hmac(md5)"
> -};
> +const char *test_algos[] = { "cmac(aes128)", "hmac(sha1)", "hmac(sha256)" };

With the FIPS/algorithm-probing logic removed and roughly one third of
generated keys now using hmac(sha256), and with the kernel side now
returning -ENOENT for non-whitelisted algorithms, tcp_sigpool_alloc_ahash()
will fail for every hmac(sha256) key on a kernel built from this config
unless CRYPTO_SHA256 is selected.

CRYPTO_SHA256 is not implied by CRYPTO_HMAC or TCP_AO, so on a kernel built
strictly from tools/testing/selftests/net/tcp_ao/config the tests would
appear to fail rather than be skipped.  Can the config fragment be updated
to match the new required algorithm set?

One more question, on the commit message and documentation rather than the
diff: Documentation/networking/tcp_ao.rst still describes TCP-AO as "May
support any hashing algorithm" and does not mention the newly enforced
whitelist or the -ENOENT failure mode.  Should tcp_ao.rst be updated in
this patch to list the accepted algorithm strings and the rationale (e.g.
the 20-byte TCP option MAC cap), so userspace has a documented contract?