* [PATCH net 1/4] rxrpc: Fix memory leaks in rxkad_verify_response()
2026-04-20 14:58 [PATCH net 0/4] rxrpc: Miscellaneous fixes David Howells
@ 2026-04-20 14:58 ` David Howells
2026-04-20 14:58 ` [PATCH net 2/4] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets David Howells
` (2 subsequent siblings)
3 siblings, 0 replies; 7+ messages in thread
From: David Howells @ 2026-04-20 14:58 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Jeffrey Altman, stable
Fix rxkad_verify_response() to free ticket by using a __free() construct
rather than explicitly freeing it.
Also fix rxkad_verify_response() to free the server key by using a __free()
construct.
Fixes: 57af281e5389 ("rxrpc: Tidy up abort generation infrastructure")
Fixes: ec832bd06d6f ("rxrpc: Don't retain the server key in the connection")
Closes: https://sashiko.dev/#/patchset/20260408121252.2249051-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@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
---
include/linux/key.h | 2 +
net/rxrpc/rxkad.c | 133 +++++++++++++++-----------------------------
2 files changed, 48 insertions(+), 87 deletions(-)
diff --git a/include/linux/key.h b/include/linux/key.h
index 81b8f05c6898..1cafbc3827c2 100644
--- a/include/linux/key.h
+++ b/include/linux/key.h
@@ -304,6 +304,8 @@ extern void key_put(struct key *key);
extern bool key_put_tag(struct key_tag *tag);
extern void key_remove_domain(struct key_tag *domain_tag);
+DEFINE_FREE(key_put, struct key *, if (!IS_ERR(_T)) key_put(_T))
+
static inline struct key *__key_get(struct key *key)
{
refcount_inc(&key->usage);
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index eb7f2769d2b1..0acdc46f42c2 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -1131,21 +1131,20 @@ static int rxkad_decrypt_response(struct rxrpc_connection *conn,
static int rxkad_verify_response(struct rxrpc_connection *conn,
struct sk_buff *skb)
{
- struct rxkad_response *response;
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
struct rxrpc_crypt session_key;
- struct key *server_key;
time64_t expiry;
- void *ticket;
u32 version, kvno, ticket_len, level;
__be32 csum;
int ret, i;
_enter("{%d}", conn->debug_id);
- server_key = rxrpc_look_up_server_security(conn, skb, 0, 0);
+ struct key *server_key __free(key_put) =
+ rxrpc_look_up_server_security(conn, skb, 0, 0);
if (IS_ERR(server_key)) {
ret = PTR_ERR(server_key);
+ server_key = NULL;
switch (ret) {
case -ENOKEY:
return rxrpc_abort_conn(conn, skb, RXKADUNKNOWNKEY, ret,
@@ -1160,16 +1159,15 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
}
ret = -ENOMEM;
- response = kzalloc_obj(struct rxkad_response, GFP_NOFS);
+ struct rxkad_response *response __free(kfree) =
+ kzalloc_obj(struct rxkad_response, GFP_NOFS);
if (!response)
goto temporary_error;
if (skb_copy_bits(skb, sizeof(struct rxrpc_wire_header),
- response, sizeof(*response)) < 0) {
- rxrpc_abort_conn(conn, skb, RXKADPACKETSHORT, -EPROTO,
- rxkad_abort_resp_short);
- goto protocol_error;
- }
+ response, sizeof(*response)) < 0)
+ return rxrpc_abort_conn(conn, skb, RXKADPACKETSHORT, -EPROTO,
+ rxkad_abort_resp_short);
version = ntohl(response->version);
ticket_len = ntohl(response->ticket_len);
@@ -1177,103 +1175,79 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
trace_rxrpc_rx_response(conn, sp->hdr.serial, version, kvno, ticket_len);
- if (version != RXKAD_VERSION) {
- rxrpc_abort_conn(conn, skb, RXKADINCONSISTENCY, -EPROTO,
- rxkad_abort_resp_version);
- goto protocol_error;
- }
+ if (version != RXKAD_VERSION)
+ return rxrpc_abort_conn(conn, skb, RXKADINCONSISTENCY, -EPROTO,
+ rxkad_abort_resp_version);
- if (ticket_len < 4 || ticket_len > MAXKRB5TICKETLEN) {
- rxrpc_abort_conn(conn, skb, RXKADTICKETLEN, -EPROTO,
- rxkad_abort_resp_tkt_len);
- goto protocol_error;
- }
+ if (ticket_len < 4 || ticket_len > MAXKRB5TICKETLEN)
+ return rxrpc_abort_conn(conn, skb, RXKADTICKETLEN, -EPROTO,
+ rxkad_abort_resp_tkt_len);
- if (kvno >= RXKAD_TKT_TYPE_KERBEROS_V5) {
- rxrpc_abort_conn(conn, skb, RXKADUNKNOWNKEY, -EPROTO,
- rxkad_abort_resp_unknown_tkt);
- goto protocol_error;
- }
+ if (kvno >= RXKAD_TKT_TYPE_KERBEROS_V5)
+ return rxrpc_abort_conn(conn, skb, RXKADUNKNOWNKEY, -EPROTO,
+ rxkad_abort_resp_unknown_tkt);
/* extract the kerberos ticket and decrypt and decode it */
ret = -ENOMEM;
- ticket = kmalloc(ticket_len, GFP_NOFS);
+ void *ticket __free(kfree) = kmalloc(ticket_len, GFP_NOFS);
if (!ticket)
- goto temporary_error_free_resp;
+ goto temporary_error;
if (skb_copy_bits(skb, sizeof(struct rxrpc_wire_header) + sizeof(*response),
- ticket, ticket_len) < 0) {
- rxrpc_abort_conn(conn, skb, RXKADPACKETSHORT, -EPROTO,
- rxkad_abort_resp_short_tkt);
- goto protocol_error;
- }
+ ticket, ticket_len) < 0)
+ return rxrpc_abort_conn(conn, skb, RXKADPACKETSHORT, -EPROTO,
+ rxkad_abort_resp_short_tkt);
ret = rxkad_decrypt_ticket(conn, server_key, skb, ticket, ticket_len,
&session_key, &expiry);
if (ret < 0)
- goto temporary_error_free_ticket;
+ goto temporary_error;
/* use the session key from inside the ticket to decrypt the
* response */
ret = rxkad_decrypt_response(conn, response, &session_key);
if (ret < 0)
- goto temporary_error_free_ticket;
+ goto temporary_error;
if (ntohl(response->encrypted.epoch) != conn->proto.epoch ||
ntohl(response->encrypted.cid) != conn->proto.cid ||
- ntohl(response->encrypted.securityIndex) != conn->security_ix) {
- rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
- rxkad_abort_resp_bad_param);
- goto protocol_error_free;
- }
+ ntohl(response->encrypted.securityIndex) != conn->security_ix)
+ return rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+ rxkad_abort_resp_bad_param);
csum = response->encrypted.checksum;
response->encrypted.checksum = 0;
rxkad_calc_response_checksum(response);
- if (response->encrypted.checksum != csum) {
- rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
- rxkad_abort_resp_bad_checksum);
- goto protocol_error_free;
- }
+ if (response->encrypted.checksum != csum)
+ return rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+ rxkad_abort_resp_bad_checksum);
for (i = 0; i < RXRPC_MAXCALLS; i++) {
u32 call_id = ntohl(response->encrypted.call_id[i]);
u32 counter = READ_ONCE(conn->channels[i].call_counter);
- if (call_id > INT_MAX) {
- rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
- rxkad_abort_resp_bad_callid);
- goto protocol_error_free;
- }
-
- if (call_id < counter) {
- rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
- rxkad_abort_resp_call_ctr);
- goto protocol_error_free;
- }
-
+ if (call_id > INT_MAX)
+ return rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+ rxkad_abort_resp_bad_callid);
+ if (call_id < counter)
+ return rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+ rxkad_abort_resp_call_ctr);
if (call_id > counter) {
- if (conn->channels[i].call) {
- rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
- rxkad_abort_resp_call_state);
- goto protocol_error_free;
- }
+ if (conn->channels[i].call)
+ return rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+ rxkad_abort_resp_call_state);
conn->channels[i].call_counter = call_id;
}
}
- if (ntohl(response->encrypted.inc_nonce) != conn->rxkad.nonce + 1) {
- rxrpc_abort_conn(conn, skb, RXKADOUTOFSEQUENCE, -EPROTO,
- rxkad_abort_resp_ooseq);
- goto protocol_error_free;
- }
+ if (ntohl(response->encrypted.inc_nonce) != conn->rxkad.nonce + 1)
+ return rxrpc_abort_conn(conn, skb, RXKADOUTOFSEQUENCE, -EPROTO,
+ rxkad_abort_resp_ooseq);
level = ntohl(response->encrypted.level);
- if (level > RXRPC_SECURITY_ENCRYPT) {
- rxrpc_abort_conn(conn, skb, RXKADLEVELFAIL, -EPROTO,
- rxkad_abort_resp_level);
- goto protocol_error_free;
- }
+ if (level > RXRPC_SECURITY_ENCRYPT)
+ return rxrpc_abort_conn(conn, skb, RXKADLEVELFAIL, -EPROTO,
+ rxkad_abort_resp_level);
conn->security_level = level;
/* create a key to hold the security data and expiration time - after
@@ -1281,30 +1255,15 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
* as for a client connection */
ret = rxrpc_get_server_data_key(conn, &session_key, expiry, kvno);
if (ret < 0)
- goto temporary_error_free_ticket;
-
- kfree(ticket);
- kfree(response);
+ goto temporary_error;
_leave(" = 0");
return 0;
-protocol_error_free:
- kfree(ticket);
-protocol_error:
- kfree(response);
- key_put(server_key);
- return -EPROTO;
-
-temporary_error_free_ticket:
- kfree(ticket);
-temporary_error_free_resp:
- kfree(response);
temporary_error:
/* Ignore the response packet if we got a temporary error such as
* ENOMEM. We just want to send the challenge again. Note that we
* also come out this way if the ticket decryption fails.
*/
- key_put(server_key);
return ret;
}
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH net 2/4] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets
2026-04-20 14:58 [PATCH net 0/4] rxrpc: Miscellaneous fixes David Howells
2026-04-20 14:58 ` [PATCH net 1/4] rxrpc: Fix memory leaks in rxkad_verify_response() David Howells
@ 2026-04-20 14:58 ` David Howells
2026-04-20 14:58 ` [PATCH net 3/4] rxgk: Fix potential integer overflow in length check David Howells
2026-04-20 14:58 ` [PATCH net 4/4] rxrpc: Fix rxkad crypto unalignment handling David Howells
3 siblings, 0 replies; 7+ messages in thread
From: David Howells @ 2026-04-20 14:58 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Jeffrey Altman, stable
The security operations that verify the RESPONSE packets decrypt bits of it
in place - however, the sk_buff may be shared with a packet sniffer, which
would lead to the sniffer seeing an apparently corrupt packet (actually
decrypted).
Fix this by unsharing the skbuff before handing it off to the specific
security handler.
Fixes: 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
Closes: https://sashiko.dev/#/patchset/20260408121252.2249051-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@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/ar-internal.h | 2 +-
net/rxrpc/conn_event.c | 12 ++++++++++--
net/rxrpc/io_thread.c | 15 +++------------
net/rxrpc/skbuff.c | 26 ++++++++++++++++++++++----
4 files changed, 36 insertions(+), 19 deletions(-)
diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 96ecb83c9071..fb04d2ffdb27 100644
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -1486,7 +1486,7 @@ int rxrpc_server_keyring(struct rxrpc_sock *, sockptr_t, int);
void rxrpc_kernel_data_consumed(struct rxrpc_call *, struct sk_buff *);
void rxrpc_new_skb(struct sk_buff *, enum rxrpc_skb_trace);
void rxrpc_see_skb(struct sk_buff *, enum rxrpc_skb_trace);
-void rxrpc_eaten_skb(struct sk_buff *, enum rxrpc_skb_trace);
+struct sk_buff *rxrpc_unshare_skb(struct sk_buff **_old, gfp_t gfp);
void rxrpc_get_skb(struct sk_buff *, enum rxrpc_skb_trace);
void rxrpc_free_skb(struct sk_buff *, enum rxrpc_skb_trace);
void rxrpc_purge_queue(struct sk_buff_head *);
diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
index 9a41ec708aeb..3d56a5d23369 100644
--- a/net/rxrpc/conn_event.c
+++ b/net/rxrpc/conn_event.c
@@ -244,8 +244,9 @@ static void rxrpc_call_is_secure(struct rxrpc_call *call)
* connection-level Rx packet processor
*/
static int rxrpc_process_event(struct rxrpc_connection *conn,
- struct sk_buff *skb)
+ struct sk_buff **_skb)
{
+ struct sk_buff *skb = *_skb;
struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
bool secured = false;
int ret;
@@ -270,6 +271,13 @@ static int rxrpc_process_event(struct rxrpc_connection *conn,
}
spin_unlock_irq(&conn->state_lock);
+ skb = rxrpc_unshare_skb(_skb, GFP_NOFS);
+ if (!skb)
+ return -ENOMEM;
+
+ /* If unshared, skb will have changed. */
+ sp = rxrpc_skb(skb);
+
ret = conn->security->verify_response(conn, skb);
if (ret < 0)
return ret;
@@ -371,7 +379,7 @@ static void rxrpc_do_process_connection(struct rxrpc_connection *conn)
* connection that each one has when we've finished with it */
while ((skb = skb_dequeue(&conn->rx_queue))) {
rxrpc_see_skb(skb, rxrpc_skb_see_conn_work);
- ret = rxrpc_process_event(conn, skb);
+ ret = rxrpc_process_event(conn, &skb);
switch (ret) {
case -ENOMEM:
case -EAGAIN:
diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
index 697956931925..0592ce644fc3 100644
--- a/net/rxrpc/io_thread.c
+++ b/net/rxrpc/io_thread.c
@@ -249,19 +249,10 @@ static bool rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff **_skb)
* decryption.
*/
if (sp->hdr.securityIndex != 0) {
- skb = skb_unshare(skb, GFP_ATOMIC);
- if (!skb) {
- rxrpc_eaten_skb(*_skb, rxrpc_skb_eaten_by_unshare_nomem);
- *_skb = NULL;
+ skb = rxrpc_unshare_skb(_skb, GFP_ATOMIC);
+ if (!skb)
return just_discard;
- }
-
- if (skb != *_skb) {
- rxrpc_eaten_skb(*_skb, rxrpc_skb_eaten_by_unshare);
- *_skb = skb;
- rxrpc_new_skb(skb, rxrpc_skb_new_unshared);
- sp = rxrpc_skb(skb);
- }
+ sp = rxrpc_skb(skb);
}
break;
diff --git a/net/rxrpc/skbuff.c b/net/rxrpc/skbuff.c
index 3bcd6ee80396..0dca9ca163f1 100644
--- a/net/rxrpc/skbuff.c
+++ b/net/rxrpc/skbuff.c
@@ -47,12 +47,30 @@ void rxrpc_get_skb(struct sk_buff *skb, enum rxrpc_skb_trace why)
}
/*
- * Note the dropping of a ref on a socket buffer by the core.
+ * Do the unsharing of a socket buffer, noting the event in the traces.
*/
-void rxrpc_eaten_skb(struct sk_buff *skb, enum rxrpc_skb_trace why)
+struct sk_buff *rxrpc_unshare_skb(struct sk_buff **_old, gfp_t gfp)
{
- int n = atomic_inc_return(&rxrpc_n_rx_skbs);
- trace_rxrpc_skb(skb, 0, n, why);
+ struct sk_buff *skb, *old = *_old;
+ int n, r = refcount_read(&old->users);
+
+ skb = skb_unshare(old, gfp);
+ if (!skb) {
+ n = atomic_dec_return(&rxrpc_n_rx_skbs);
+ trace_rxrpc_skb(old, r, n, rxrpc_skb_eaten_by_unshare_nomem);
+ *_old = NULL;
+ return skb;
+ }
+
+ if (skb != old) {
+ n = atomic_read(&rxrpc_n_rx_skbs);
+ trace_rxrpc_skb(old, r, n, rxrpc_skb_eaten_by_unshare);
+ trace_rxrpc_skb(skb, refcount_read(&skb->users), n,
+ rxrpc_skb_new_unshared);
+ *_old = skb;
+ }
+
+ return skb;
}
/*
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH net 4/4] rxrpc: Fix rxkad crypto unalignment handling
2026-04-20 14:58 [PATCH net 0/4] rxrpc: Miscellaneous fixes David Howells
` (2 preceding siblings ...)
2026-04-20 14:58 ` [PATCH net 3/4] rxgk: Fix potential integer overflow in length check David Howells
@ 2026-04-20 14:58 ` David Howells
3 siblings, 0 replies; 7+ messages in thread
From: David Howells @ 2026-04-20 14:58 UTC (permalink / raw)
To: netdev
Cc: David Howells, Marc Dionne, Jakub Kicinski, David S. Miller,
Eric Dumazet, Paolo Abeni, Simon Horman, linux-afs, linux-kernel,
Jeffrey Altman, stable
Fix handling of a packet with a misaligned crypto length. Also handle
non-ENOMEM errors from decryption by aborting. Further, remove the
WARN_ON_ONCE() so that it can't be remotely triggered (a trace line can
still be emitted).
Fixes: f93af41b9f5f ("rxrpc: Fix missing error checks for rxkad encryption/decryption failure")
Closes: https://sashiko.dev/#/patchset/20260408121252.2249051-1-dhowells%40redhat.com
Signed-off-by: David Howells <dhowells@redhat.com>
cc: Marc Dionne <marc.dionne@auristor.com>
cc: Jeffrey Altman <jaltman@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
---
include/trace/events/rxrpc.h | 1 +
net/rxrpc/rxkad.c | 9 +++++++--
2 files changed, 8 insertions(+), 2 deletions(-)
diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 578b8038b211..5820d7e41ea0 100644
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -37,6 +37,7 @@
EM(rxkad_abort_1_short_encdata, "rxkad1-short-encdata") \
EM(rxkad_abort_1_short_header, "rxkad1-short-hdr") \
EM(rxkad_abort_2_short_check, "rxkad2-short-check") \
+ EM(rxkad_abort_2_crypto_unaligned, "rxkad2-crypto-unaligned") \
EM(rxkad_abort_2_short_data, "rxkad2-short-data") \
EM(rxkad_abort_2_short_header, "rxkad2-short-hdr") \
EM(rxkad_abort_2_short_len, "rxkad2-short-len") \
diff --git a/net/rxrpc/rxkad.c b/net/rxrpc/rxkad.c
index 0acdc46f42c2..2c4697063ab2 100644
--- a/net/rxrpc/rxkad.c
+++ b/net/rxrpc/rxkad.c
@@ -510,6 +510,9 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
return rxrpc_abort_eproto(call, skb, RXKADSEALEDINCON,
rxkad_abort_2_short_header);
+ /* Don't let the crypto algo see a misaligned length. */
+ sp->len = round_down(sp->len, 8);
+
/* Decrypt the skbuff in-place. TODO: We really want to decrypt
* directly into the target buffer.
*/
@@ -543,8 +546,10 @@ static int rxkad_verify_packet_2(struct rxrpc_call *call, struct sk_buff *skb,
if (sg != _sg)
kfree(sg);
if (ret < 0) {
- WARN_ON_ONCE(ret != -ENOMEM);
- return ret;
+ if (ret == -ENOMEM)
+ return ret;
+ return rxrpc_abort_eproto(call, skb, RXKADSEALEDINCON,
+ rxkad_abort_2_crypto_unaligned);
}
/* Extract the decrypted packet length */
^ permalink raw reply related [flat|nested] 7+ messages in thread