Re: [PATCH net v5 07/10] afs: Create a server appdata key

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev/#/patchset/[email protected]?part=7
---
--- Patch [7]: [PATCH net v5 07/10] afs: Create a server appdata key ---
commit 46d1f7800458c15b841827dd597a26f2786367f4
Author: David Howells <[email protected]>
Subject: afs: Create a server appdata key

This patch introduces an appdata key for each AFS fileserver the client
communicates with using RxGK. It shifts the responsibility of generating
application data from responding to challenges in the OOB queue to creating
it when setting up the server record.

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..7f08ede12206a 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,
[ ... ]
> +	/* Store the appdata before the key pointer */
> +	smp_store_release(&server->yfs_rxgk_appdata, appdata_key);
> +
> +out_aead:
> +	crypto_free_aead(aead);
> +out_token:
> +	kfree(appdata);

Simon says:

Hi David,

I'm wondering if you could take a look into the issue flagged below.

[Severity: Medium]
Could this leave the plaintext cryptographic callback key (K0) in the slab
cache?

When creating an RxGK appdata key, afs_create_yfs_rxgk_cm_appdata()
allocates a temporary appdata buffer, generates a random cryptographic key
into it, encrypts it, and then frees the temporary buffer using kfree().

Can an unprivileged process or remote attacker who exploits an information leak
recover the plaintext key from the heap and forge RxGK callback connections?
Would kfree_sensitive() be more appropriate here to ensure the buffer is
securely zeroed?

> +out:
> +	return ret;
> +}
> +#endif /* CONFIG_RXGK */
[ ... ]

Simon says: But I don't think we need to consider the pre-existing
            issues flagged below in the context of this patch-set.

I noticed a couple of pre-existing issues while reviewing this code:

> fs/afs/cm_security.c:afs_respond_to_challenge()
>
> 			server = (struct afs_server *)peer_data;
> 			if (!server->cm_rxgk_appdata.data) {

[Severity: Critical]
This is a pre-existing issue, but is it safe to cast and dereference peer_data
here?

When a network CHALLENGE packet arrives and is processed in the OOB queue in
afs_respond_to_challenge(), it reads peer_data and casts it to an afs_server
pointer.

Since the code dereferences this pointer without holding rcu_read_lock() or
taking a reference on the server object, what happens if the server is
concurrently destroyed or updated?

If peer_data is set to 0, this causes a direct NULL dereference. If the
server memory is freed, could this result in a use-after-free triggered by a
remote attacker?


> fs/afs/server.c:afs_lookup_server()
>
> out:
> 	afs_put_addrlist(alist, afs_alist_trace_put_server_create);
> 	if (candidate) {
> 		kfree(rcu_access_pointer(server->endpoint_state));
> 		kfree(candidate);
> 		afs_dec_servers_outstanding(cell->net);
> 	}

[Severity: High]
This isn't a bug introduced by this patch, but could this cleanup path free
the active server's endpoint state?

When two threads concurrently execute afs_lookup_server() for a new server,
one thread installs its candidate and the other receives the installed server.
On the exit path (out:), if candidate is non-null, the second thread
erroneously calls kfree(rcu_access_pointer(server->endpoint_state)) instead
of freeing the candidate's state.

Also, if the first thread fails server creation, it sets server = NULL.
Would accessing server->endpoint_state here then trigger a direct NULL
pointer dereference?