Re: [PATCH net v9 11/14] afs: Fix creation of RxGK CM channel token to have right size

[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: 0 · Low: 1 (1)

- [Low] Changelog of the stable-tagged fix does not state the user-
  visible symptom of the off-by-4 `toksize` in afs_create_yfs_cm_token()…

Pre-existing issues:
- [Low] afs_create_yfs_cm_token() keeps a hand-maintained field-by-field
  size expression (`toksize`, fs/afs/cm_security.c:238) whose only guard…

--

afs: Fix creation of RxGK CM channel token to have right size

This corrects the token size arithmetic in afs_create_yfs_cm_token() by
adding the missing 4 bytes for the RXGK_Level field.  The size feeds the
encrypted token container and the appdata buffer published as
server->cm_rxgk_appdata.

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

> diff --git a/fs/afs/cm_security.c b/fs/afs/cm_security.c
> index 103168c70dd4d..5eeeef761cf31 100644
> --- a/fs/afs/cm_security.c
> +++ b/fs/afs/cm_security.c
> @@ -235,7 +235,7 @@ static int afs_create_yfs_cm_token(struct sk_buff *challenge,
>  	 *	struct RXGK_AuthName	identities<>;
>  	 * };
>  	 */
> -	toksize = keysize + 8 + 4 + 4 + 8 + xdr_len_object(authsize);
> +	toksize = keysize + 4 + 8 + 4 + 4 + 8 + xdr_len_object(authsize);

[Severity: Low]
Could the changelog say what the observable symptom of the old arithmetic
was?  The field count matches what I see written after the K0 blob in
afs_create_yfs_cm_token():

	level(4) + starttime(8) + lifetime(4) + bytelife(4) +
	expirationtime(8) + identities.count(4) + kind(4) +
	data.len(4) + uuid(16) + display.len(4) = 60

and the pre-patch expression accounted for 56, so the arithmetic fix looks
right.

What is not stated is where the missing 4 bytes went.  toksize is passed as
data_len to crypto_krb5_encrypt(), and the rfc3961 encrypt profile places
the checksum at conf_len + data_len:

crypto/krb5/rfc3961_simplified.c:krb5_aead_encrypt() {
	...
	base_len	= krb5->conf_len + data_len;
	...
	cksum_offset	= secure_len;
	if (WARN_ON(cksum_offset + krb5->cksum_len > sg_len))
		return -EFAULT;
	...
}

So with the short toksize, the trailing word written here:

	*xdr++ = htonl(0);			/* token.identities[0].display.len */

sat outside the encrypted region and was then overwritten by the first
4 bytes of the HMAC, producing a truncated/malformed RXGK_Token in the
callback-channel appdata.  Since the token key enctype is fixed to
AES128-CTS-HMAC-SHA1-96 (cksum_len 12), the extra bytes stayed inside the
kzalloc'd buffer, so there was no out-of-bounds access.

Would it help stable backporters to state that this was a silent
protocol-encoding defect with no memory-safety impact and no kernel log
message?

[Severity: Low]
This isn't a bug introduced by this patch, but while the toksize term is
being corrected, is the existing size self-check in
afs_create_yfs_cm_token() able to detect this class of error at all?

Just before the check, the write cursor is thrown away and recomputed from
encsize:

	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);

encsize comes from crypto_krb5_how_much_buffer(..., toksize, &offset) and
adatasize is derived from contsize, which is derived from encsize, so both
operands move together with any error in toksize.  That is presumably why
the check stayed balanced with the pre-patch 4-byte-short token and nothing
was logged.

The check is also non-fatal: execution falls through to

	sg_init_one(&sg, encbase, encsize);
	ret = crypto_krb5_encrypt(token_krb5, aead, &sg, 1, encsize, offset, toksize, false);

and then to publication into server->cm_rxgk_appdata.

Would comparing the post-write cursor against encbase + offset + toksize
and failing be preferable, in the style of the decoder counterpart?

net/rxrpc/rxgk_app.c:rxgk_yfs_decode_ticket() {
	...
	if (WARN_ON((unsigned long)q != (unsigned long)ticket)) {
		ret = -EIO;
		goto error;
	}
	...
}

As it stands, a future field addition or reordering in the token can
silently regenerate the same malformed-token result.  I could not find a
way to overflow the buffer today: keysize (4 + xdr_len_object(key_len))
exactly matches the three K0 writes, and every other emitted field is a
compile-time constant, so this is a hardening observation rather than a
live defect.

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