[PATCH net 0/4] rxrpc: Miscellaneous fixes

[PATCH net 0/4] rxrpc: Miscellaneous fixes

[David Howells]

Here are some fixes for rxrpc, as found by Sashiko[1]:

 (1) Fix leaks in rxkad_verify_response().

 (2) Fix lack of unsharing of RESPONSE packets.

 (3) Fix integer overflow in RxGK ticket length check.

 (4) Fix handling of rxkad-encrypted packets with crypto-misaligned
     lengths.

David

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=rxrpc-fixes

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

David Howells (4):
  rxrpc: Fix memory leaks in rxkad_verify_response()
  rxrpc: Fix conn-level packet handling to unshare RESPONSE packets
  rxgk: Fix potential integer overflow in length check
  rxrpc: Fix rxkad crypto unalignment handling

 include/linux/key.h          |   2 +
 include/trace/events/rxrpc.h |   1 +
 net/rxrpc/ar-internal.h      |   2 +-
 net/rxrpc/conn_event.c       |  12 ++-
 net/rxrpc/io_thread.c        |  15 +---
 net/rxrpc/rxgk_app.c         |   2 +-
 net/rxrpc/rxgk_common.h      |   1 +
 net/rxrpc/rxkad.c            | 142 +++++++++++++----------------------
 net/rxrpc/skbuff.c           |  26 ++++++-
 9 files changed, 94 insertions(+), 109 deletions(-)

[PATCH net 1/4] rxrpc: Fix memory leaks in rxkad_verify_response()

[David Howells]

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;
 }
 

Re: [PATCH net 1/4] rxrpc: Fix memory leaks in rxkad_verify_response()

[Simon Horman]

On Mon, Apr 20, 2026 at 03:58:54PM +0100, David Howells wrote:
> 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>

...

> index eb7f2769d2b1..0acdc46f42c2 100644
> --- a/net/rxrpc/rxkad.c
> +++ b/net/rxrpc/rxkad.c

...

> @@ -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;
>  

Hi David,

This goto, combined with the use of __free in the declaration
of ticket below results in a compile error for x86_64 allmodconfig
with clang 21.1.8.

  net/rxrpc/rxkad.c:1165:3: error: cannot jump from this goto statement to its label
   1165 |                 goto temporary_error;
        |                 ^
  net/rxrpc/rxkad.c:1192:8: note: jump bypasses initialization of variable with __attribute__((cleanup))
   1192 |         void *ticket __free(kfree) = kmalloc(ticket_len, GFP_NOFS);
        |               ^

Moreover, the use of this construct is discouraged in Networking code:

  1.7.3. Using device-managed and cleanup.h constructs¶

  Netdev remains skeptical about promises of all “auto-cleanup” APIs,
  including even devm_ helpers, historically. They are not the preferred
  style of implementation, merely an acceptable one.

  Use of guard() is discouraged within any function longer than 20 lines,
  scoped_guard() is considered more readable. Using normal lock/unlock is
  still (weakly) preferred.

  Low level cleanup constructs (such as __free()) can be used when building
  APIs and helpers, especially scoped iterators. However, direct use of
  __free() within networking core and drivers is discouraged. Similar
  guidance applies to declaring variables mid-function.

  https://docs.kernel.org/process/maintainer-netdev.html#using-device-managed-and-cleanup-h-constructs

And to round things out, Sashiko also points out problems with
the use of __free() in this patch.

...

>  
>  	/* 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;

...

>  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;
>  }
>  
> 

-- 
pw-bot: changes-requested

[PATCH net 2/4] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

[David Howells]

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;
 }
 
 /*

Re: [PATCH net 2/4] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

[Simon Horman]

From: '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.
Full review at: https://sashiko.dev
---
Subject: rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

This commit modifies the RxRPC connection-level packet processor to unshare
RESPONSE packets before decryption. It introduces a new rxrpc_unshare_skb()
helper to safely unshare skbuffs passed via a double pointer and replaces
direct calls to skb_unshare().

Closes: https://sashiko.dev/#/patchset/20260408121252.2249051-1-dhowells%40redhat.com

> diff --git a/net/rxrpc/conn_event.c b/net/rxrpc/conn_event.c
> index 9a41ec708aeb9..3d56a5d233697 100644
> --- a/net/rxrpc/conn_event.c
> +++ b/net/rxrpc/conn_event.c

[ ... ]

> @@ -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:

Looking at the code immediately following this hunk in
rxrpc_do_process_connection():

		switch (ret) {
		case -ENOMEM:
		case -EAGAIN:
			skb_queue_head(&conn->rx_queue, skb);
			rxrpc_queue_conn(conn, rxrpc_conn_queue_retry_work);
			break;

If rxrpc_process_event() fails with -ENOMEM because rxrpc_unshare_skb()
failed, rxrpc_unshare_skb() will have set *_skb = NULL.

Does this mean skb is NULL when we reach skb_queue_head()?

Since skb_queue_head() dereferences the skb parameter, this appears to
result in a NULL pointer dereference. Furthermore, since skb_unshare()
frees the original skb on allocation failure, the packet is already gone
and requeuing it wouldn't be possible anyway.

> diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
> index 6979569319252..0592ce644fc3d 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;
> -			}

This isn't a new bug introduced by this patch, but since we are modifying
the unshare path here: when rxrpc_unshare_skb() fails, it sets *_skb = NULL
and returns just_discard.

Back in rxrpc_io_thread(), the packet is processed like this:

net/rxrpc/io_thread.c:rxrpc_io_thread() {
...
		while ((skb = __skb_dequeue(&rx_queue))) {
			struct rxrpc_skb_priv *sp = rxrpc_skb(skb);
			switch (skb->mark) {
			case RXRPC_SKB_MARK_PACKET:
				skb->priority = 0;
				if (!rxrpc_input_packet(local, &skb))
					rxrpc_reject_packet(local, skb);
				trace_rxrpc_rx_done(skb->mark, skb->priority);
...

If rxrpc_input_packet() sets skb to NULL and returns false (just_discard),
does this cause a NULL pointer dereference when trace_rxrpc_rx_done() tries
to access skb->mark?

Re: [PATCH net 2/4] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

[David Howells]

Simon Horman <horms@kernel.org> wrote:

> This isn't a new bug introduced by this patch, but since we are modifying
> the unshare path here: when rxrpc_unshare_skb() fails, it sets *_skb = NULL
> and returns just_discard.

Is there a way to avoid having skb_unshare() eat your ref to the source skbuff
if it fails?

David

[PATCH net 3/4] rxgk: Fix potential integer overflow in length check

[David Howells]

Fix potential integer overflow in rxgk_extract_token() when checking the
length of the ticket.  Rather than rounding up the value to be tested
(which might overflow), round down the size of the available data.

Fixes: 2429a1976481 ("rxrpc: Fix untrusted unsigned subtract")
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/rxgk_app.c    | 2 +-
 net/rxrpc/rxgk_common.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)

diff --git a/net/rxrpc/rxgk_app.c b/net/rxrpc/rxgk_app.c
index 30275cb5ba3e..5587639d60c5 100644
--- a/net/rxrpc/rxgk_app.c
+++ b/net/rxrpc/rxgk_app.c
@@ -214,7 +214,7 @@ int rxgk_extract_token(struct rxrpc_connection *conn, struct sk_buff *skb,
 	ticket_len	= ntohl(container.token_len);
 	ticket_offset	= token_offset + sizeof(container);
 
-	if (xdr_round_up(ticket_len) > token_len - sizeof(container))
+	if (ticket_len > xdr_round_down(token_len - sizeof(container)))
 		goto short_packet;
 
 	_debug("KVNO %u", kvno);
diff --git a/net/rxrpc/rxgk_common.h b/net/rxrpc/rxgk_common.h
index 80164d89e19c..1e257d7ab8ec 100644
--- a/net/rxrpc/rxgk_common.h
+++ b/net/rxrpc/rxgk_common.h
@@ -34,6 +34,7 @@ struct rxgk_context {
 };
 
 #define xdr_round_up(x) (round_up((x), sizeof(__be32)))
+#define xdr_round_down(x) (round_down((x), sizeof(__be32)))
 #define xdr_object_len(x) (4 + xdr_round_up(x))
 
 /*

[PATCH net 4/4] rxrpc: Fix rxkad crypto unalignment handling

[David Howells]

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 */