From: Anderson Nascimento <anderson@allelesecurity.com>

In rxrpc_setsockopt(), the code checks 'rx->key' when handling the
RXRPC_SECURITY_KEYRING option.  However, this appears to be a logic error.
The code should be checking 'rx->securities' to determine if a keyring has
already been defined for the socket.

Currently, if a user calls setsockopt(RXRPC_SECURITY_KEYRING) multiple
times on the same socket, the check 'if (rx->key)' fails to block
subsequent calls because 'rx->key' has not been defined by the function.
This results in a reference count leak on the keyring.

This patch changes the check to 'rx->securities' to correctly identify if
the socket security keyring has already been configured, returning -EINVAL
on subsequent attempts.

Before the patch:

It shows the keyring reference counter elevated.

$ cat /proc/keys | grep AFSkeys1
27aca8ae I--Q--- 24469721 perm 3f010000  1000  1000 keyring   AFSkeys1: empty
$

After the patch:

The keyring reference counter remains stable and subsequent calls return an
error:

$ ./poc
setsockopt: Invalid argument
$

Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Signed-off-by: Anderson Nascimento <anderson@allelesecurity.com>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Eric Dumazet <edumazet@google.com>
cc: "David S. Miller" <davem@davemloft.net>
cc: Jakub Kicinski <kuba@kernel.org>
cc: Paolo Abeni <pabeni@redhat.com>
cc: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: netdev@vger.kernel.org
cc: stable@kernel.org
---
 net/rxrpc/af_rxrpc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
index 0f90272ac254..0b7ed99a3025 100644
--- a/net/rxrpc/af_rxrpc.c
+++ b/net/rxrpc/af_rxrpc.c
@@ -665,7 +665,7 @@ static int rxrpc_setsockopt(struct socket *sock, int level, int optname,
 
 		case RXRPC_SECURITY_KEYRING:
 			ret = -EINVAL;
-			if (rx->key)
+			if (rx->securities)
 				goto error;
 			ret = -EISCONN;
 			if (rx->sk.sk_state != RXRPC_UNBOUND)

On Thu, Mar 26, 2026 at 10:19 AM David Howells <dhowells@redhat.com> wrote:
>
> From: Anderson Nascimento <anderson@allelesecurity.com>
>
> In rxrpc_setsockopt(), the code checks 'rx->key' when handling the
> RXRPC_SECURITY_KEYRING option.  However, this appears to be a logic error.
> The code should be checking 'rx->securities' to determine if a keyring has
> already been defined for the socket.
>
> Currently, if a user calls setsockopt(RXRPC_SECURITY_KEYRING) multiple
> times on the same socket, the check 'if (rx->key)' fails to block
> subsequent calls because 'rx->key' has not been defined by the function.
> This results in a reference count leak on the keyring.
>
> This patch changes the check to 'rx->securities' to correctly identify if
> the socket security keyring has already been configured, returning -EINVAL
> on subsequent attempts.
>
> Before the patch:
>
> It shows the keyring reference counter elevated.
>
> $ cat /proc/keys | grep AFSkeys1
> 27aca8ae I--Q--- 24469721 perm 3f010000  1000  1000 keyring   AFSkeys1: empty
> $
>
> After the patch:
>
> The keyring reference counter remains stable and subsequent calls return an
> error:
>
> $ ./poc
> setsockopt: Invalid argument
> $
>
> Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
> Signed-off-by: Anderson Nascimento <anderson@allelesecurity.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> Reviewed-by: Jeffrey Altman <jaltman@auristor.com>
> cc: Marc Dionne <marc.dionne@auristor.com>
> cc: Eric Dumazet <edumazet@google.com>
> cc: "David S. Miller" <davem@davemloft.net>
> cc: Jakub Kicinski <kuba@kernel.org>
> cc: Paolo Abeni <pabeni@redhat.com>
> cc: Simon Horman <horms@kernel.org>
> cc: linux-afs@lists.infradead.org
> cc: netdev@vger.kernel.org
> cc: stable@kernel.org
> ---
>  net/rxrpc/af_rxrpc.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/net/rxrpc/af_rxrpc.c b/net/rxrpc/af_rxrpc.c
> index 0f90272ac254..0b7ed99a3025 100644
> --- a/net/rxrpc/af_rxrpc.c
> +++ b/net/rxrpc/af_rxrpc.c
> @@ -665,7 +665,7 @@ static int rxrpc_setsockopt(struct socket *sock, int level, int optname,
>
>                 case RXRPC_SECURITY_KEYRING:
>                         ret = -EINVAL;
> -                       if (rx->key)
> +                       if (rx->securities)
>                                 goto error;
>                         ret = -EISCONN;
>                         if (rx->sk.sk_state != RXRPC_UNBOUND)
>

I am following this patchset along with sashiko's comments. This time,
it flagged this patch for an API order dependency. The setsockopt
RXRPC_SECURITY_KEY option checks for rx->securities before proceeding,
but after this change, the RXRPC_SECURITY_KEYRING option doesn't check
for rx->key. Consequently, it's possible to set both keys on a socket
depending on the setsockopt call order.

I noticed this and wondered about it before sending the patch, but I
decided to keep the patch as concise as possible rather than assuming
too much and potentially breaking code. To make the logic more
coherent, what if we check if (rx->key || rx->securities) in both
options and remove the rx->securities check from rxrpc_request_key()?

Based on my check, rxrpc_request_key() and rxrpc_server_keyring() are
only called by their setsockopt options. We could also add a comment
to rxrpc_request_key() and rxrpc_server_keyring() noting that key
validation should be handled by the caller. Is the -EINVAL error
appropriate for this case?

-- 
Anderson Nascimento
Allele Security Intelligence
https://www.allelesecurity.com

Anderson Nascimento <anderson@allelesecurity.com> wrote:

> To make the logic more coherent, what if we check if (rx->key ||
> rx->securities) in both options and remove the rx->securities check from
> rxrpc_request_key()?

You're allowed to have both a keyring (server) and a key (client).  You can
issue client calls on a server socket.  The in-kernel kafs filesystem does
this, for example - though it normally sets the outgoing key on individual
calls.

To parallel the kernel example, it might be worth my while adding a CMSG tag
to take a key ID or key description so the rxrpc_sendmsg() can do a
request_key() when setting up a call (the AF_RXRPC socket allows a different
key with each call dispatched), though the AFS command line tools tend only to
talk to a single cell at a time (you only need one key for comms with an
entire cell).

Davod

On Fri, Mar 27, 2026 at 4:24 AM David Howells <dhowells@redhat.com> wrote:
>
> Anderson Nascimento <anderson@allelesecurity.com> wrote:
>
> > To make the logic more coherent, what if we check if (rx->key ||
> > rx->securities) in both options and remove the rx->securities check from
> > rxrpc_request_key()?
>
> You're allowed to have both a keyring (server) and a key (client).  You can
> issue client calls on a server socket.  The in-kernel kafs filesystem does
> this, for example - though it normally sets the outgoing key on individual
> calls.
>

Understood, thanks.

> To parallel the kernel example, it might be worth my while adding a CMSG tag
> to take a key ID or key description so the rxrpc_sendmsg() can do a
> request_key() when setting up a call (the AF_RXRPC socket allows a different
> key with each call dispatched), though the AFS command line tools tend only to
> talk to a single cell at a time (you only need one key for comms with an
> entire cell).
>
> Davod
>

-- 
Anderson Nascimento
Allele Security Intelligence
https://www.allelesecurity.com