Re: [PATCH net 4/4] rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation

[Simon Horman <horms@kernel.org>]

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
---
Subject: rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation

This commit removes the out-of-band queueing mechanism for CHALLENGE
packets. It transitions the connection event processor to handle
CHALLENGE parsing and RESPONSE generation directly, while allowing
application data to be passed via a user-type key.

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..db2a3b30f0209 100644
> --- a/fs/afs/cm_security.c
> +++ b/fs/afs/cm_security.c
> @@ -295,13 +295,22 @@ static int afs_create_yfs_cm_token(struct sk_buff *challenge,
>  	if (ret < 0)
>  		goto out_aead;
>  
> -	server->cm_rxgk_appdata.len  = adatasize;
> -	server->cm_rxgk_appdata.data = appdata;
> -	appdata = NULL;
> +	appdata_key = key_alloc(&key_type_user, "rxrpc: afs rxgk appdata",
> +				GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, current_cred(),
> +				KEY_POS_VIEW | KEY_POS_SEARCH | KEY_USR_VIEW,
> +				KEY_ALLOC_NOT_IN_QUOTA, NULL);
> +	if (IS_ERR(appdata_key)) {
> +		ret = PTR_ERR(appdata_key);
> +		goto out_aead;
> +	}
> +
> +	ret = key_instantiate_and_link(appdata_key, appdata, adatasize, NULL, NULL);
> +	if (ret < 0) {
> +		key_put(appdata_key);
> +		goto out_aead;
> +	}
> +
> +	server->cm_rxgk_appdata = appdata_key;

[Severity: Medium]
Does this plain store require memory barriers? afs_make_call() appears to
read call->server->cm_rxgk_appdata concurrently without holding the
cm_token_lock. Without smp_store_release() here and a corresponding
smp_load_acquire() or READ_ONCE() on the read side, could the reader
observe the pointer before the key object's initialization is completely
visible to other CPUs?

> diff --git a/fs/afs/rxrpc.c b/fs/afs/rxrpc.c
> index 05fcb9b6adde7..ae48c15b3db94 100644
> --- a/fs/afs/rxrpc.c
> +++ b/fs/afs/rxrpc.c
> @@ -373,8 +373,25 @@ void afs_make_call(struct afs_call *call, gfp_t gfp)
>  		call->drop_ref = true;
>  	}
>  
> +	if (call->key && call->server) {
> +		u32 krb5_enctype = 0;
> +		u8 security_index = 0;
> +
> +		rxrpc_kernel_query_key(call->key, &security_index, &krb5_enctype);
> +		switch (security_index) {
> +#ifdef CONFIG_RXGK
> +		case RXRPC_SECURITY_YFS_RXGK:
> +			app_data = call->server->cm_rxgk_appdata;
> +			break;

[Severity: Medium]
Following up on the previous observation, is it safe to perform a plain
read of the cm_rxgk_appdata pointer here in afs_make_call() without
READ_ONCE() or a lock?

> diff --git a/net/rxrpc/rxgk.c b/net/rxrpc/rxgk.c
> index 77a67ace1d248..9ab766d5f0b6c 100644
> --- a/net/rxrpc/rxgk.c
> +++ b/net/rxrpc/rxgk.c
> @@ -981,36 +981,36 @@ static int rxgk_construct_response(struct rxrpc_connection *conn,
> [ ... ]
> +	if (conn->bundle->app_data) {
> +		rcu_read_lock();
> +		appdata = (struct user_key_payload *)
> +			user_key_payload_rcu(conn->bundle->app_data);

[Severity: High]
If an attacker passes the ID of a non-user key (like an asymmetric key
or keyring) during sendmsg(), could casting that key's payload to struct
user_key_payload and modifying its refcount cause memory corruption?

> +		if (!refcount_inc_not_zero(&appdata->ref))

[Severity: High]
Can user_key_payload_rcu() return NULL here if the user key has been
revoked by userspace via keyctl(KEYCTL_REVOKE)? If appdata evaluates to
NULL, would calling refcount_inc_not_zero(&appdata->ref) attempt to
dereference a low offset address and trigger a kernel panic?

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 4c754f78ece9a..225eec410bedc 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -590,6 +592,18 @@ static int rxrpc_sendmsg_cmsg(struct msghdr *msg, struct rxrpc_send_params *p)
> [ ... ]
> +		case RXRPC_RESPONSE_APPDATA:
> +			if (len != sizeof(key_serial_t))
> +				return -EINVAL;
> +			if (p->call.app_data)
> +				return -EINVAL;
> +			key_id = *(key_serial_t *)CMSG_DATA(cmsg);
> +			key = lookup_user_key(key_id, 0, KEY_NEED_SEARCH);
> +			if (IS_ERR(key))
> +				return PTR_ERR(key);
> +			p->call.app_data = key_ref_to_ptr(key);
> +			break;

[Severity: High]
Should there be a check here in rxrpc_sendmsg_cmsg() to verify that the
fetched key is actually a user-type key (e.g., key->type == &key_type_user)?
Without validation, any key type could be assigned to p->call.app_data,
leading to the invalid cast mentioned previously in
rxgk_respond_to_challenge().

> @@ -677 +716 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
> [ ... ]
>  	call = rxrpc_find_call_by_user_ID(rx, p.call.user_call_ID);
>  	if (!call) {
> [ ... ]
> +		call = rxrpc_new_client_call_for_sendmsg(rx, msg, &p);
> +		/* The socket is now unlocked... */
> +		if (IS_ERR(call))
> +			return PTR_ERR(call);

[Severity: High]
Does this direct return leak the key reference acquired by lookup_user_key()
and stored in p.call.app_data when rxrpc_new_client_call_for_sendmsg()
fails?

> @@ -803,6 +803,7 @@ int rxrpc_do_sendmsg(struct rxrpc_sock *rx, struct msghdr *msg, size_t len)
>  out_put_unlock:
>  	if (!dropped_lock)
>  		mutex_unlock(&call->user_mutex);
>  error_put:
>  	rxrpc_put_call(call, rxrpc_call_put_sendmsg);
>  	_leave(" = %d", ret);
>  	return ret;
>  
>  error_release_sock:
>  	release_sock(&rx->sk);
> +	key_put(p.call.app_data);
>  	return ret;
>  }

[Severity: High]
Do the out_put_unlock and error_put paths in rxrpc_do_sendmsg() also need
to call key_put(p.call.app_data)? It appears rxrpc_alloc_client_call()
takes its own reference on the key via key_get(), which means the original
reference acquired during rxrpc_sendmsg_cmsg() will be leaked on successful
operations or when failing after the socket lock is released.