Re: [PATCH net v3 13/14] 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.
https://sashiko.dev/#/patchset/20260713081022.2186481-8-dhowells@redhat.com?part=13
---
--- Patch [13]: [PATCH net v3 13/14] rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation ---
commit ebc5bb53c96c167e2d7c2726bed42c09fd3a97d1
Author: David Howells <dhowells@redhat.com>

rxrpc: Fix CHALLENGE packet overqueuing and simplify RESPONSE generation

This commit removes the out-of-band queueing mechanism for rxrpc challenge
packets, moving instead to a preemptively supplied application data key.
The event processor now parses the challenge and generates the response
directly using this key.

Link: https://sashiko.dev/#/patchset/20260624163819.3017002-1-dhowells%40redhat.com

> --- a/net/rxrpc/recvmsg.c
> +++ b/net/rxrpc/recvmsg.c
> @@ -405,7 +405,7 @@ int rxrpc_recvmsg(struct socket *sock, struct msghdr *msg, size_t len,
>  
>  	trace_rxrpc_recvmsg(0, rxrpc_recvmsg_enter, 0);
>  
> -	if (flags & (MSG_OOB | MSG_TRUNC))
> +	if (flags & MSG_TRUNC)
>  		return -EOPNOTSUPP;

[Severity: Medium]
Does removing MSG_OOB from this check cause out-of-band requests to silently
fall through and return in-band data?

By not returning -EOPNOTSUPP for MSG_OOB, the socket might violate standard
semantics by returning regular data when the caller explicitly requested
out-of-band data.

> --- a/net/rxrpc/rxgk.c
> +++ b/net/rxrpc/rxgk.c
> @@ -936,7 +937,7 @@ static int rxgk_construct_response(struct rxrpc_connection *conn,
>  	if (IS_ERR(gk))
>  		return PTR_ERR(gk);
>  
> -	auth_len = 20 + (4 + appdata->len) + 12 + (1 + 4) * 4;
> +	auth_len = 20 + (4 + appdata->datalen) + 12 + (1 + 4) * 4;

[Severity: Critical]
Is auth_len calculated incorrectly here by missing the padding for
appdata->datalen?

It looks like this unpadded length is used to calculate authx_len, which
determines the size of the skb and the scatterlist allocation.

Later, rxgk_construct_authenticator() uses rxgk_pad_out() to correctly
round up the written bytes:

ret = rxgk_pad_out(response, appdata->datalen, offset);

Since the written padded size is larger than the unpadded alloc_len
passed to rxgk_encrypt_authenticator(), will this cause the crypto layer
to write past the end of the scatterlist and corrupt memory?

[ ... ]

> @@ -1011,24 +1012,36 @@ static int rxgk_construct_response(struct rxrpc_connection *conn,
>   * Respond to a challenge packet.
>   */
>  static int rxgk_respond_to_challenge(struct rxrpc_connection *conn,
> -				     struct sk_buff *challenge,
> -				     struct krb5_buffer *appdata)
> +				     struct sk_buff *challenge)
>  {
> -	_enter("{%d,%x}", conn->debug_id, key_serial(conn->key));
> +	struct user_key_payload dummy = {}, *appdata = &dummy;
> +	int ret;
> +
> +	_enter("{%d,%u,%x,%x}",
> +	       conn->debug_id, conn->service_id,
> +	       key_serial(conn->key), key_serial(conn->bundle->app_data));
>  
>  	if (key_validate(conn->key) < 0)
>  		return rxrpc_abort_conn(conn, NULL, RXGK_EXPIRED, -EPROTO,
>  					rxgk_abort_chall_key_expired);
>  
> -	return rxgk_construct_response(conn, challenge, appdata);
> -}
> +	if (conn->bundle->app_data) {
> +		rcu_read_lock();
> +		appdata = (struct user_key_payload *)
> +			user_key_payload_rcu(conn->bundle->app_data);
> +		if (appdata && !refcount_inc_not_zero(&appdata->ref))
> +			appdata = NULL;
> +		rcu_read_unlock();

[Severity: Critical]
Is it safe to blindly cast the payload of conn->bundle->app_data to
a struct user_key_payload here?

If a non-user key like a keyring or logon key was somehow attached to
the call, this cast will interpret the alien key payload as a
user_key_payload. For example, if rxrpc_sendmsg_cmsg() in
net/rxrpc/sendmsg.c has an inverted type check that inadvertently accepts
non-user keys:

if (key_ref_to_ptr(key)->type == &key_type_user) {
    key_ref_put(key);
    return -EINVAL;
}

Calling refcount_inc_not_zero() on a mismatched type could corrupt memory
by incrementing whatever happens to be at that offset. Furthermore, reading
the data length from this corrupted structure could leak arbitrary kernel
memory into the network via the response packet.