Re: [PATCH v4 08/11] rxrpc: Move key types to their own module

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [Low] rxrpc: update kernel-doc path from server_key.c to security.c in
  rxrpc.rst
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [High] rxrpc: memory leak of conn->key in rxrpc_process_event() [1]
--

commit 16a0f09dc6df360865624dc81f7bfb1b9505bec6
Author: Jarkko Sakkinen <jarkko@kernel.org>

rxrpc: Move key types to their own module

This commit splits the rxrpc and rxrpc_s key types and their preparse
logic into a separate rxrpc_keys.ko module and moves socket and
connection key helper functions into net/rxrpc/security.c so rxrpc.ko
can be unloaded while persistent keys remain in keyrings.

> diff --git a/net/rxrpc/security.c b/net/rxrpc/security.c
> index 2bfbf2b2bb37a..28883a70c9e85 100644
> --- a/net/rxrpc/security.c
> +++ b/net/rxrpc/security.c

[ ... ]

> +/*
> + * generate a server data key
> + */
> +int rxrpc_get_server_data_key(struct rxrpc_connection *conn,
> +			      const void *session_key,
> +			      time64_t expiry,
> +			      u32 kvno)
> +{
> +	const struct cred *cred = current_cred();
> +	struct {
> +		u32 kver;
> +		struct rxrpc_key_data_v1 v1;
> +	} data;
> +	struct key *key;
> +	int ret;
> +
> +	_enter("");
> +
> +	key = key_alloc(&key_type_rxrpc, "x",
> +			GLOBAL_ROOT_UID, GLOBAL_ROOT_GID, cred, 0,
> +			KEY_ALLOC_NOT_IN_QUOTA, NULL);
> +	if (IS_ERR(key)) {
> +		_leave(" = -ENOMEM [alloc %ld]", PTR_ERR(key));
> +		return -ENOMEM;
> +	}
> +
> +	_debug("key %d", key_serial(key));
> +
> +	data.kver = 1;
> +	data.v1.security_index = RXRPC_SECURITY_RXKAD;
> +	data.v1.ticket_length = 0;
> +	data.v1.expiry = rxrpc_time64_to_u32(expiry);
> +	data.v1.kvno = 0;
> +
> +	memcpy(&data.v1.session_key, session_key, sizeof(data.v1.session_key));
> +
> +	ret = key_instantiate_and_link(key, &data, sizeof(data), NULL, NULL);
> +	if (ret < 0)
> +		goto error;
> +
> +	conn->key = key;

[Severity: High]
This problem wasn't introduced by this patch, but can assigning conn->key
here leak an existing key [1] if a RESPONSE packet is replayed?

In rxrpc_process_event(), if rxrpc_verify_response() succeeds (allocating
and assigning conn->key via rxrpc_get_server_data_key()) but the subsequent
conn->security->init_connection_security() call fails, the function returns
early while conn->state remains RXRPC_CONN_SERVICE_CHALLENGING:

net/rxrpc/conn_event.c:rxrpc_process_event() {
    ...
	case RXRPC_PACKET_TYPE_RESPONSE:
		spin_lock_irq(&conn->state_lock);
		if (conn->state != RXRPC_CONN_SERVICE_CHALLENGING) {
			spin_unlock_irq(&conn->state_lock);
			return 0;
		}
		spin_unlock_irq(&conn->state_lock);

		ret = rxrpc_verify_response(conn, skb);
		if (ret < 0)
			return ret;

		ret = conn->security->init_connection_security(
			conn, conn->key->payload.data[0]);
		if (ret < 0)
			return ret;
    ...
}

A replayed RESPONSE packet will then run rxrpc_verify_response() again and
overwrite conn->key without calling key_put() on the previous key.

[ ... ]

> +/**
> + * rxrpc_sock_set_security_keyring - Set the security keyring for a kernel service
> + * @sk: The socket to set the keyring on
> + * @keyring: The keyring to set
> + *
> + * Set the server security keyring on an rxrpc socket.  This is used to provide
> + * the encryption keys for a kernel service.
> + *
> + * Return: %0 if successful and a negative error code otherwise.
> + */
> +int rxrpc_sock_set_security_keyring(struct sock *sk, struct key *keyring)
> +{

[ ... ]

> +/**
> + * rxrpc_sock_set_manage_response - Set the manage-response flag for a kernel service

[Severity: Low]
Moving rxrpc_sock_set_security_keyring() and rxrpc_sock_set_manage_response()
from net/rxrpc/server_key.c to net/rxrpc/security.c leaves no kernel-doc
comments in net/rxrpc/server_key.c, while Documentation/networking/rxrpc.rst
still has:

  .. kernel-doc:: net/rxrpc/server_key.c
     :internal:

Should that directive in Documentation/networking/rxrpc.rst be updated to
reference net/rxrpc/security.c so these functions remain in the generated API
documentation?

[1] https://sashiko.dev/#/bug/linux-93795e42-6eae-42ec-9f00-b78f9b947f35

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261107131512.3603168-1-jarkko@kernel.org?part=8