Re: [PATCH net v3 11/14] afs: Create a server appdata key
From: Simon Horman <horms@kernel.org>
Date: 2026-07-20 14:54:06
Also in:
lkml
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@redhat.com?part=11 ---
--- Patch [11]: [PATCH net v3 11/14] afs: Create a server appdata key ---commit dc88fd6982b57fe321cbc39b2f060f5b1d06bb8e Author: David Howells [off-list ref] 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
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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.