Re: [PATCH net-next] xfrm: add ARIA CBC and CTR support

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 1 · Low: 1

- [Medium] Duplicated `.compat = "aria"` alias in `ealg_list[]` makes the
  new `rfc3686(ctr(aria))` descriptor's legacy alias permanently…
- [Low] Inconsistent default key sizes (`defkeybits`) between CBC and CTR
  modes for ARIA.

--

xfrm: add ARIA CBC and CTR support

This adds two XFRM encryption algorithm descriptors so ARIA can be
selected for ESP, one for cbc(aria) and one for rfc3686(ctr(aria)).
Both are marked pfkey_supported = 0, so they are reachable only through
the XFRM netlink interface.

> diff --git a/net/xfrm/xfrm_algo.c b/net/xfrm/xfrm_algo.c
> index 70434495f23f..16c70dc1a426 100644
> --- a/net/xfrm/xfrm_algo.c
> +++ b/net/xfrm/xfrm_algo.c
> @@ -512,6 +512,34 @@ static struct xfrm_algo_desc ealg_list[] = {
>  		.sadb_alg_maxbits = 256
>  	}
>  },
> +{
> +	.name = "cbc(aria)",
> +	.compat = "aria",
> +
> +	.uinfo = {
> +		.encr = {
> +			.geniv = "echainiv",
> +			.blockbits = 128,
> +			.defkeybits = 256,
> +		}
> +	},
> +
> +	.pfkey_supported = 0,
> +},
> +{
> +	.name = "rfc3686(ctr(aria))",
> +	.compat = "aria",

[Severity: Low]
The two new entries advertise different ARIA key strengths: the
cbc(aria) entry sets .defkeybits = 256, while the rfc3686(ctr(aria))
entry sets 160, which - going by the "/* 128-bit key + 32-bit nonce */"
comment on the rfc3686(ctr(aes)) entry a few lines further down - means
a 128-bit ARIA key plus a 32-bit nonce.  So the default for the CBC
variant is ARIA-256 and for the CTR variant ARIA-128.  Was that
deliberate?

For comparison, every other cbc(...) entry in ealg_list[] (cbc(aes),
cbc(camellia), cbc(serpent), cbc(twofish), cbc(sm4)) uses 128, and the
AES CTR counterpart uses 128 + 32.  If ARIA-256 is the intended default
here, should the CTR entry be 288 (256-bit key + 32-bit nonce) to keep
the two consistent?  If not, should the CBC entry be 128 like its
siblings?

As far as I can tell nothing breaks either way: attach_crypt() derives
the key length from ualg->alg_key_len rather than from uinfo.encr, and
with .pfkey_supported = 0 there is no sadb_alg_minbits/maxbits pair to
disagree with, so defkeybits is effectively documentation for these two
entries.  But since it is documentation, having two entries for the same
cipher in the same table imply different default key sizes is confusing
to read.  Please also add the "128-bit key + 32-bit nonce" style comment
on the 160 if that is indeed what the value encodes.

        ^^^^^^^^^^^^^^^^^^

[Severity: Medium]
Should the CTR entry carry a .compat alias at all, given that the
cbc(aria) entry added just above already claims "aria"?

xfrm_alg_name_match() accepts either the name or the compat string, with
no distinction between modes:

net/xfrm/xfrm_algo.c:xfrm_alg_name_match() {
	return name && (!strcmp(name, entry->name) ||
			(entry->compat && !strcmp(name, entry->compat)));
}

and xfrm_find_algo() only ever acts on the first matching entry:

net/xfrm/xfrm_algo.c:xfrm_find_algo() {
	for (i = 0; i < algo_list->entries; i++) {
		if (!match(list + i, data))
			continue;

		if (list[i].available)
			return &list[i];

		if (!probe)
			break;

		status = algo_list->find(list[i].name, 0, 0);
		if (!status)
			break;
	...
}

Since cbc(aria) comes first in ealg_list[], does a lookup of "aria"
ever reach the rfc3686(ctr(aria)) entry?  It looks like the .compat
field on the CTR entry can never be used, so the CTR variant is only
selectable by its full name via
attach_crypt()->xfrm_ealg_get_byname(ualg->alg_name, 1).

There also seems to be a second effect in a build where ARIA and CTR are
enabled but CBC is not (CRYPTO_ARIA=y, CRYPTO_CTR=y, CRYPTO_CBC=n).  The
probe of cbc(aria) fails, xfrm_find_algo() hits the break and returns
NULL, and attach_crypt() reports:

net/xfrm/xfrm_user.c:attach_crypt() {
	algo = xfrm_ealg_get_byname(ualg->alg_name, 1);
	if (!algo) {
		NL_SET_ERR_MSG(extack, "Requested CRYPT algorithm not found");
		return -ENOSYS;
	}
	...
}

so "enc aria" would fail with -ENOSYS even though a later entry
advertising the same alias is instantiable.  Is that intended?

For comparison, the existing rfc3686(ctr(aes)) entry in the same table
sets no .compat because "aes" is owned by cbc(aes), and the other
pfkey_supported = 0 entries here (cmac(aes), rfc7539esp(...)) omit
.compat as well.  Would dropping .compat from the rfc3686(ctr(aria))
entry match that convention?

> +
> +	.uinfo = {
> +		.encr = {
> +			.geniv = "seqiv",
> +			.blockbits = 128,
> +			.defkeybits = 160,
> +		}
> +	},
> +
> +	.pfkey_supported = 0,
> +},
>  {
>  	.name = "cbc(twofish)",
>  	.compat = "twofish",

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260909054530.7861-1-his1415%40pribit.com