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

[netdev-bot+sashiko]

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

Critical: 0 · High: 0 · Medium: 2 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch moves the RxGK quota charge in
  rxrpc_preparse_xdr_yfs_rxgk() (net/rxrpc/key.c) to the success path,…
- [Medium] The patch adds a new -EKEYREJECTED return to
  rxrpc_preparse_xdr_yfs_rxgk() when `raw_keylen != enc->key_len`.

--

rxrpc: Fix RxGK key parser to check enctype is supported

The RxGK XDR key parser now looks up the enctype with
crypto_krb5_find_enctype(). It also rejects keys whose length does not
match that enctype. The RxGK key handling is put under CONFIG_RXGK, and
both parsers now charge a token to the quota only once the token is
attached.

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

[ ... ]

> @@ -229,6 +226,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;
> +	}
> +	if (raw_keylen != enc->key_len) {
> +		ret = -EKEYREJECTED;
> +		goto reject_token;
> +	}

[Severity: Medium]
How does this new -EKEYREJECTED return interact with the server-side
ticket path?

rxgk_yfs_decode_ticket() in net/rxrpc/rxgk_app.c builds an XDR payload
from the decrypted ticket. It copies the ticket's enctype and key length
as-is, then calls key_instantiate_and_link(), which reaches this code:

rxgk_extract_token()
  rxgk_yfs_decode_ticket()
    key_instantiate_and_link()
      rxrpc_preparse()
        rxrpc_preparse_xdr()
          rxrpc_preparse_xdr_yfs_rxgk()

rxrpc_preparse_xdr() passes -EKEYREJECTED back up. The cant_get_token
switch in rxgk_extract_token() only handles three error codes:

net/rxrpc/rxgk_app.c:rxgk_extract_token() {
    ...
cant_get_token:
	switch (ret) {
	case -ENOMEM:
		goto temporary_error;
	case -EINVAL:
		return rxrpc_abort_conn(conn, skb, RXGK_NOTAUTH, -EKEYREJECTED,
					rxgk_abort_resp_tok_internal_error);
	case -ENOPKG:
		return rxrpc_abort_conn(conn, skb, KRB5_PROG_KEYTYPE_NOSUPP,
					-EKEYREJECTED, rxgk_abort_resp_tok_nopkg);
	}

temporary_error:
	/* Ignore the response packet if we got a temporary error such as
	 * ENOMEM.  We just want to send the challenge again.  ...
}

So -EKEYREJECTED falls through to temporary_error. rxgk_verify_response()
then does goto out without an abort, and rxrpc_do_process_connection()
ignores the return value of rxrpc_process_event().

Before this patch, a ticket whose K0 length did not match its enctype
still got through key creation. It failed later instead:

- in rxgk_generate_transport_key() or rxgk_set_up_ciphers(), aborted with
  RXGK_NOTAUTH
- or in authenticator decryption, aborted with RXGK_SEALEDINCON

With this patch, is the RESPONSE silently dropped, so the connection keeps
re-challenging until it times out instead of being aborted? The gap in
the switch was already there, but this patch adds a new way to reach it.

Should rxgk_extract_token() handle -EKEYREJECTED explicitly?

[ ... ]

> @@ -257,6 +265,8 @@ static int rxrpc_preparse_xdr_yfs_rxgk(struct key_preparsed_payload *prep,
>  	_debug("TICK: %*phN",
>  	       min_t(u32, token->rxgk->ticket.len, 32), token->rxgk->ticket.data);
>  
> +	prep->quotalen += sizeof(*token) + datalen + tktlen + keylen;

[Severity: Medium]
Is sizeof(*token->rxgk) meant to be left out here? The removed code
charged it:

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

The struct rxgk_key header is still allocated by
kzalloc_flex(struct rxgk_key, _key, raw_keylen), and it lives as long as
the key. That means each RxGK token is now under-charged by
sizeof(struct rxgk_key), which is 72 bytes on 64-bit.

In this same patch, the RxKAD parser still counts sizeof(*token->kad) in
plen. The commit message only says a token should be charged when it is
used. It does not mention charging less.

The datalen term does not make up the difference, because
rxrpc_preparse_xdr() only calls the per-token parsers once the remaining
datalen has reached 0. generic_key_instantiate() then passes
prep->quotalen directly to key_payload_reserve().

A key can hold up to AFSTOKEN_MAX tokens. Could an unprivileged user use
this to pin more memory in rxrpc keys than their key quota allows?

>  	/* count the number of tokens attached */
>  	prep->payload.data[1] = (void *)((unsigned long)prep->payload.data[1] + 1);
>

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