Re: [PATCH net v10 11/14] afs: Fix creation of RxGK CM channel token to have right size
From: netdev-bot+sashiko@kernel.org
Date: 2026-09-15 22:57:45
Also in:
lkml
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?
quoted hunk ↗ jump to hunk
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