Re: [PATCH net v3 11/14] afs: Create a server appdata key

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260713081022.2186481-8-dhowells@xxxxxxxxxx?part=11
---
--- Patch [11]: [PATCH net v3 11/14] afs: Create a server appdata key ---
commit dc88fd6982b57fe321cbc39b2f060f5b1d06bb8e
Author: David Howells <dhowells@xxxxxxxxxx>

afs: Create a server appdata key

This commit creates an appdata key for each fileserver the AFS filesystem
talks to with RxGK. It attaches the key to the afs_server record to be
used for generating application data in response to CHALLENGE packets.

Link: https://sashiko.dev/#/patchset/20260624163819.3017002-1-dhowells%40redhat.com

> diff --git a/fs/afs/cm_security.c b/fs/afs/cm_security.c
> index 103168c70dd4d..36907a04efd0b 100644
> --- a/fs/afs/cm_security.c
> +++ b/fs/afs/cm_security.c
[ ... ]
> @@ -23,6 +25,236 @@ static int afs_create_yfs_cm_token(struct sk_buff *challenge,
> +	/* struct RXGK_Token {
> +	 *	rxgk_key		K0;
> +	 *	RXGK_Level		level;
> +	 *	rxgkTime		starttime;
> +	 *	afs_int32		lifetime;
> +	 *	afs_int32		bytelife;
> +	 *	rxgkTime		expirationtime;
> +	 *	struct RXGK_AuthName	identities<>;
> +	 * };
> +	 */
> +	toksize = keysize + 8 + 4 + 4 + 8 + xdr_len_object(authsize);

[Severity: High]
Does this toksize calculation omit the 4 bytes required for the level field? 

It looks like the sizes added up correspond to starttime (8), lifetime (4),
bytelife (4), and expirationtime (8).

If the level field is omitted, toksize will be 4 bytes too small.

[ ... ]
> +	*xdr++ = htonl(RXRPC_SECURITY_ENCRYPT);	/* token.level */
> +	*xdr++ = htonl(0);			/* token.starttime */
> +	*xdr++ = htonl(0);			/* " */
> +	*xdr++ = htonl(0);			/* token.lifetime */
> +	*xdr++ = htonl(0);			/* token.bytelife */
> +	*xdr++ = htonl(0);			/* token.expirationtime */
> +	*xdr++ = htonl(0);			/* " */
[ ... ]
> +	xdr = encbase + xdr_round_up(encsize);
> +
> +	if ((unsigned long)xdr - (unsigned long)appdata != adatasize)
> +		pr_err("Appdata size incorrect %lx != %zx\n",
> +		       (unsigned long)xdr - (unsigned long)appdata, adatasize);

[Severity: High]
Can this manual advance of the xdr pointer hide the token size mismatch?

Because xdr is advanced based on the miscalculated encsize rather than the
actual number of fields written, it appears this will bypass the adatasize
sanity check on the next line.

[ ... ]
> +	ret = crypto_krb5_encrypt(token_krb5, aead, &sg, 1, encsize, offset, toksize, false);

[Severity: High]
Since toksize is smaller than the actual plaintext being constructed, will
this corrupt the token?

When crypto_krb5_encrypt() executes, it seems it will only process the
truncated toksize bytes of plaintext. This would leave the trailing 4 bytes
of the serialized XDR unencrypted and overwritten by the appended Kerberos
checksum. 

The fileserver would then reject the appdata token when XDR parsing fails.