Re: [PATCH net v10 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 1 potential
issue(s) to consider:

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

- [Medium] rxrpc_preparse_xdr_yfs_rxgk() charges the full token cost to
  prep->quotalen at net/rxrpc/key.c:209 (`prep->quotalen += datalen +…

--

rxrpc: Fix RxGK key parser to check enctype is supported

The RxGK key parser now looks up the userspace-supplied encryption type
with crypto_krb5_find_enctype() and rejects the token if the enctype is
unknown or if the supplied key length does not match the enctype.  The
RxGK-specific token handling is also made conditional on CONFIG_RXGK.

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 cbd26da449510..cdef609996d28 100644
> --- a/net/rxrpc/key.c
> +++ b/net/rxrpc/key.c

[ ... ]

> @@ -166,12 +167,14 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  				       size_t datalen,
>  				       const __be32 *xdr, unsigned int toklen)
>  {
> +	const struct krb5_enctype *enc;
>  	struct rxrpc_key_token *token, **pptoken;
>  	time64_t expiry;
>  	size_t plen;
>  	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 +232,17 @@ 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;
>  
> +	/* Check the enctype is supported. */
> +	enc = crypto_krb5_find_enctype(token->rxgk->enctype);
> +	if (!enc) {
> +		ret = -ENOPKG;
> +		goto reject_token;
> +	}

[Severity: Medium]
Can this new -ENOPKG path leave prep->quotalen charged for memory that
was freed?

Earlier in rxrpc_preparse_xdr_yfs_rxgk() the whole token cost is added to
the quota before any of these checks run:

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

with tktlen bounded only by AFSTOKEN_GK_TOKEN_MAX (16384).  The
reject_token label then does kfree(token->rxgk) and kfree(token) without
subtracting that amount again.

Every pre-existing reject path here returned -EKEYREJECTED, -EKEYEXPIRED
or -ENOMEM, and rxrpc_preparse_xdr() turns those into an abort:

		switch (ret2) {
		case 0:
			ret = 0;
			break;
		case -EPROTONOSUPPORT:
			break;
		case -ENOPKG:
			if (ret != 0)
				ret = -ENOPKG;
			break;
		default:
			ret = ret2;
			goto error;
		}

-ENOPKG is the soft case: if another token in the same payload (for
example an RXKAD token) parsed successfully, ret stays 0 and the key is
still instantiated, so __key_instantiate_and_link() ->
key_payload_reserve(key, prep->quotalen) commits the inflated value
against the owner's key quota:

security/keys/key.c:key_payload_reserve() {
	...
		if (delta > 0 &&
		    (key->user->qnbytes + delta > maxbytes ||
		     key->user->qnbytes + delta < key->user->qnbytes)) {
			ret = -EDQUOT;
	...
}

Since key_quota_maxbytes defaults to 20000 bytes, a single crafted
add_key("rxrpc", "afs@...", ...) payload containing one valid RXKAD token
plus an RxGK token with an unsupported enctype appears to be able to
charge roughly 16KB that is not actually held, making later add_key()
calls for that uid fail with -EDQUOT.

Would it be better to do the enctype lookup and key length check before
the prep->quotalen += datalen + plen accounting, or to subtract
datalen + plen on the -ENOPKG path?

[ ... ]

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