Re: [PATCH net v9 10/14] rxrpc: Fix RxGK key parser to check enctype is supported

[netdev-bot+sashiko@kernel.org]

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

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

- [High] Link/modpost failure in valid configurations: the newly added
  call to `crypto_krb5_find_enctype()` in…
- [Low] Key quota over-charge newly reachable: in
  `rxrpc_preparse_xdr_yfs_rxgk()` (net/rxrpc/key.c:206) the full token…

Pre-existing issues:
- [Low] `rxrpc_preparse_xdr_yfs_rxgk()` now resolves the krb5 enctype
  handle but discards it, so the user-supplied session-key length…

--

rxrpc: Fix RxGK key parser to check enctype is supported

The RxGK token parser in net/rxrpc/key.c now looks up the userspace-supplied
encryption type and rejects the token with -ENOPKG when the enctype is not
known.  The return value of rxrpc_preparse_xdr_yfs_rxgk() is changed from a
hardcoded -EKEYREJECTED to a new local variable so the new error code can be
propagated.

Link: https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824091645.415423-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/key.c b/net/rxrpc/key.c
> index a0aa78d892897..30d6db052c216 100644
> --- a/net/rxrpc/key.c
> +++ b/net/rxrpc/key.c
> @@ -172,6 +172,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	const __be32 *ticket, *key;
>  	s64 tmp;
>  	size_t raw_keylen, raw_tktlen, keylen, tktlen;
> +	int ret = -EKEYREJECTED;
>  
>  	_enter(",{%x,%x,%x,%x},%x",
>  	       ntohl(xdr[0]), ntohl(xdr[1]), ntohl(xdr[2]), ntohl(xdr[3]),
> @@ -229,6 +230,11 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	token->rxgk->key.data	= token->rxgk->_key;
>  	token->rxgk->ticket.len = raw_tktlen;
>  
> +	if (!crypto_krb5_find_enctype(token->rxgk->enctype)) {
> +		ret = -ENOPKG;
> +		goto reject_token;
> +	}
> +

[Severity: High]
Does this build with CONFIG_AF_RXRPC=y (or m) and CONFIG_RXGK=n?

key.o is built unconditionally, while crypto_krb5_find_enctype() only exists
when CRYPTO_KRB5 is built:

net/rxrpc/Makefile:
	rxrpc-y := \
		...
		key.o \
		...
	rxrpc-$(CONFIG_RXGK) += \
		rxgk.o \
		rxgk_app.o \
		rxgk_kdf.o

crypto/krb5/Makefile:
	obj-$(CONFIG_CRYPTO_KRB5) += krb5.o

net/rxrpc/Kconfig has AF_RXRPC selecting only CRYPTO, CRYPTO_LIB_DES if
RXKAD, KEYS and NET_UDP_TUNNEL; the "select CRYPTO_KRB5" lives in the
optional RXGK bool, which defaults to n.  So AF_RXRPC=y, RXGK=n,
CRYPTO_KRB5=n looks like a legal configuration, and the declaration in
include/crypto/krb5.h (reached via include/keys/rxrpc-type.h) has no
!CONFIG_CRYPTO_KRB5 stub, so the failure would only show up at link or
modpost time as an undefined crypto_krb5_find_enctype.  The same would apply
to AF_RXRPC=y with CRYPTO_KRB5=m.

The other rxrpc caller guards this exact call:

net/rxrpc/rxperf.c:
	#ifdef CONFIG_RXGK
	/*
	 * Add a yfs-rxgk key to the security keyring.
	 */
	static int rxperf_add_yfs_rxgk_key(struct key *keyring, u32 enctype)
	{
		const struct krb5_enctype *krb5 = crypto_krb5_find_enctype(enctype);

Should the new call site be wrapped in #ifdef CONFIG_RXGK too, or should
AF_RXRPC select CRYPTO_KRB5?  Neither net/rxrpc/Kconfig nor
net/rxrpc/Makefile is touched anywhere later in the series, and the patch
carries a cc: stable tag.

[Severity: Low]
Can this over-charge the caller's key quota?

Earlier in rxrpc_preparse_xdr_yfs_rxgk() the whole token cost is accounted
before any validation:

	plen = sizeof(*token) + sizeof(*token->rxgk) + tktlen + keylen;
	prep->quotalen += datalen + plen;

The new path frees the token and returns -ENOPKG without undoing that
addition, and rxrpc_preparse_xdr() treats -ENOPKG as a per-token error that
does not abort the key:

		case -ENOPKG:
			if (ret != 0)
				ret = -ENOPKG;
			break;

So a payload holding one parsable token (say rxkad) plus an RxGK token with
an unsupported enctype gets instantiated while prep->quotalen still includes
the discarded RxGK token.  Before this change every reject_token path
returned -EKEYREJECTED, which hit the default: case and failed the whole
instantiation, so the inflated value was never committed.  Would it be
simpler to do the enctype lookup before the prep->quotalen update, or to
subtract it again on this path?

[Severity: Low]
This is a pre-existing issue and not introduced by this patch, but now that
the enctype descriptor is looked up here, should raw_keylen also be checked
against krb5->key_len?  The returned pointer is discarded, so the
user-supplied key length (bounded only by AFSTOKEN_GK_KEY_MAX, and allowed
to be 0) is still unvalidated.

The server-key parser pairs the two checks:

net/rxrpc/rxgk.c:rxgk_preparse_server_key() {
	krb5 = crypto_krb5_find_enctype(enctype);
	if (!krb5)
		return -ENOPKG;
	...
}

At use time rxgk_derive_transport_key() passes &rxgk->key to
crypto_krb5_calc_PRFplus(), and the rfc8009 profile just does:

crypto/krb5/rfc8009_aes2.c:rfc8009_calc_KDF_HMAC_SHA2() {
	ret = crypto_shash_setkey(shash, key->data, key->len);
	...
}

HMAC accepts any length, so a transport key gets derived from a truncated or
empty session key, while the rfc3961/rfc3962 profiles fail later:

crypto/krb5/rfc3961_simplified.c:rfc3961_calc_DK() {
	if (inkey->len != keylength || result->len != keylength)
		return -EINVAL;
	...
}

All consumers honour key.len, so there is no out-of-bounds access, just
acceptance of malformed key material and inconsistent late failures.

>  	if (token->rxgk->endtime != 0) {
>  		expiry = rxrpc_s64_to_time64(token->rxgk->endtime);
>  		if (expiry < 0)
> @@ -280,7 +286,7 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	kfree(token->rxgk);
>  	kfree(token);
>  reject:
> -	return -EKEYREJECTED;
> +	return ret;
>  expired:
>  	kfree(token->rxgk);
>  	kfree(token);

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260907113743.1453210-1-dhowells%40redhat.com