Re: [PATCH net v3 09/11] rxrpc: Fix keyring reference count leak in rxrpc_setsockopt()
From: Anderson Nascimento <hidden>
Date: 2026-03-27 01:06:34
Also in:
lkml
On Thu, Mar 26, 2026 at 10:19 AM David Howells [off-list ref] wrote:
quoted hunk ↗ jump to hunk
From: Anderson Nascimento <redacted> 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 <redacted> Signed-off-by: David Howells <dhowells@redhat.com> Reviewed-by: Jeffrey Altman <redacted> 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