Re: [PATCH net v3 12/14] rxrpc: Pass appdata key to rxrpc_call and thence to rxrpc_bundle

[Simon Horman]

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@xxxxxxxxxx?part=12
---
--- Patch [12]: [PATCH net v3 12/14] rxrpc: Pass appdata key to rxrpc_call and thence to rxrpc_bundle ---
commit 6a78aabb95a0cb45b9da0216dba46c2925209a22
Author: David Howells <dhowells@xxxxxxxxxx>

rxrpc: Pass appdata key to rxrpc_call and thence to rxrpc_bundle

Currently, when a CHALLENGE packet comes in, it's queued in an OOB queue on
the AF_RXRPC socket that generated one of the calls on that connection for
the application (which might be in userspace) to service. The application
then picks up the CHALLENGE and requests a RESPONSE packet be generated,
allowing the app to include app-specific data in it if appropriate.

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

> diff --git a/net/rxrpc/conn_client.c b/net/rxrpc/conn_client.c
> index 48519f0de1853..5cbfa7b223e01 100644
> --- a/net/rxrpc/conn_client.c
> +++ b/net/rxrpc/conn_client.c
> @@ -81,6 +81,7 @@ static struct rxrpc_bundle *rxrpc_alloc_bundle(struct rxrpc_call *call,
>  		bundle->local		= call->local;
>  		bundle->peer		= rxrpc_get_peer(call->peer, rxrpc_peer_get_bundle);
>  		bundle->key		= key_get(call->key);
> +		bundle->app_data	= key_get(call->app_data);

[Severity: Medium]
Will this bundle lookup logic reuse stale app_data keys for new calls?

When rxrpc_look_up_bundle() compares connection parameters, it does not seem
to check bundle->app_data.

If a new call provides a different app_data key but otherwise identical
parameters, won't it match an existing bundle and inherit the previous call's
stale app_data instead of establishing a new connection bundle for the new
data?

>  		bundle->security	= call->security;
>  		bundle->exclusive	= test_bit(RXRPC_CALL_EXCLUSIVE, &call->flags);
>  		bundle->upgrade		= test_bit(RXRPC_CALL_UPGRADE, &call->flags);

[ ... ]

> diff --git a/net/rxrpc/sendmsg.c b/net/rxrpc/sendmsg.c
> index 71343998b87d1..cdf35440317d6 100644
> --- a/net/rxrpc/sendmsg.c
> +++ b/net/rxrpc/sendmsg.c
> @@ -614,6 +617,22 @@ static int rxrpc_sendmsg_cmsg(struct msghdr *msg, struct rxrpc_send_params *p)
>  				return -ERANGE;
>  			break;
>  
> +		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);
> +			if (key_ref_to_ptr(key)->type == &key_type_user) {
> +				key_ref_put(key);
> +				return -EINVAL;
> +			}

[Severity: High]
Does this inverted check reject valid user keys and cause a regression with
memory parsing?

The user API expects a user key here for the RESPONSE app data. However,
if a local user sends an RXRPC_RESPONSE_APPDATA control message via sendmsg
and provides a valid user key, this check rejects it with -EINVAL.

Conversely, it accepts any non-user key.

When the subsystem later attempts to parse this key's payload, won't it assume
the memory layout of a user key payload, leading to out-of-bounds reads or
memory corruption?

> +			p->call.app_data = key_ref_to_ptr(key);
> +			break;
> +
>  		default:
>  			return -EINVAL;
>  		}