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

[netdev-bot+sashiko]

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

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

- [Medium] Commit message gives a factually incorrect impact analysis for
  the bug it fixes.

--

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

This adds the missing 4 bytes for the RXGK_Level field to the toksize
calculation in afs_create_yfs_cm_token(), so that the size of the
blob-to-be-encrypted matches the number of bytes actually written.  The size
arithmetic installed by the patch looks right; the question below is only
about the impact analysis in the commit message.

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

> afs: Fix creation of RxGK CM channel token to have right size
>
> Fix afs_create_yfs_cm_token() so that it calculates the token size
> correctly, remembering to add in the 4 bytes of the level.
>
> As it happens, this bug has no effect because crypto_krb5_how_much_buffer()
> rounds encsize up to a multiple of the crypto block size (16 or 32) before
> adding on the checksum size - and so there's actually 8 bytes of unused
> space allocated within the blob-to-be-encrypted and 4 bytes of that gets
> used.

[Severity: Medium]
Is this description of the old behaviour accurate?  I can't find the
rounding it refers to.  crypto_krb5_how_much_buffer() in
crypto/krb5/krb5_api.c returns the sum with no block-size rounding:

	case KRB5_ENCRYPT_MODE:
		*_offset = krb5->conf_len;
		return krb5->conf_len + data_size + krb5->cksum_len;

and krb5_aead_encrypt() in crypto/krb5/rfc3961_simplified.c never pads
either, placing the MIC immediately after the data:

	secure_offset	= 0;
	base_len	= krb5->conf_len + data_len;
	pad_len		= 0;
	secure_len	= base_len + pad_len;
	cksum_offset	= secure_len;

So rather than 8 bytes of spare padding existing inside the blob, wasn't
the effect that the last word written by afs_create_yfs_cm_token():

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

landed exactly at cksum_offset, i.e. it was left outside the encrypted and
authenticated region and then overwritten by the checksum, because toksize
is also what gets handed to the encryption as data_len?

> 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);
>  
>  	offset = 0;
>  	encsize = crypto_krb5_how_much_buffer(token_krb5, KRB5_ENCRYPT_MODE, toksize, &offset);

Same toksize is passed as data_len further down in the same function:

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

There's no out-of-bounds write in either case, since cksum_len is 12, 16 or
24 for the supported enctypes, so the extra word stayed within the allocated
blob.  The reason the truncation went unnoticed looks to be that the only
consumer, rxgk_yfs_decode_ticket() in net/rxrpc/rxgk_app.c, only checks:

	if (ticket_len < 10 * sizeof(__be32))
		return rxrpc_abort_conn(conn, skb, RXGK_INCONSISTENCY, -EPROTO,
					rxgk_abort_resp_short_yfs_tkt);

and never parses the identities<> field at all.

Given the Fixes: tag and the cc: to stable, would it be worth correcting the
rationale so that backporters and anyone auditing stricter third-party token
decoders get the right picture?

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