Skip to contentSkip to conversation

rxrpc: Miscellaneous fixes

PATCH Applied David Howells 3 participants 15 messages Apr 22 – Apr 23

Subsystems (4)

v2 of 12 Applied 6/6 Latest is v12 →

lore
Share & tools
lore

David Howells

author maintainer
#0 lore raw

v2 cover rxrpc: Miscellaneous fixes

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

(1) Fix leaks in rxkad_verify_response().

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

(3) Fix problem with unsharing DATA packets potentially causing a crash in the caller.

(4) Fix lack of unsharing of RESPONSE packets.

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

(6) Fix missing length check in RxKAD tickets.

David

The patches can be found here also:

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

Changes
=======
ver #2)
- Use of __free() constructs in networking code is disallowed, so rework the rxkad_verify_response() patch to just clean everything up at the end and cope with NULL pointers.
- Reworked the unsharing fix:
- Used skb_cloned() and skb_copy() directly rather than skb_unshare(). The problem with skb_unshare() is that it kills the source skbuff if it can't copy, which then has to be propagated up the call chain. Even so, the code still had an bug from this[1].
- Split into two patches, one for DATA and one for RESPONSE packets.
- Do the DATA unshare a lot further along.
- Imported a patch to add a length check on RxKAD tickets.

Anderson Nascimento (1):
rxrpc: Fix missing validation of ticket length in non-XDR key
preparsing

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

 include/trace/events/rxrpc.h |   5 +-
 net/rxrpc/ar-internal.h      |   1 -
 net/rxrpc/call_event.c       |  19 +++++-
 net/rxrpc/conn_event.c       |  29 ++++++++-
 net/rxrpc/io_thread.c        |  24 +-------
 net/rxrpc/key.c              |   4 ++
 net/rxrpc/rxgk_app.c         |   2 +-
 net/rxrpc/rxgk_common.h      |   1 +
 net/rxrpc/rxkad.c            | 112 +++++++++++++++--------------------
 net/rxrpc/skbuff.c           |   9 ---
 10 files changed, 106 insertions(+), 100 deletions(-)
author maintainer
#1 lore raw

PATCH v2 1/6 rxrpc: Fix memory leaks in rxkad_verify_response()

Fix rxkad_verify_response() to free the ticket and the server key under all circumstances by initialising the ticket pointer to NULL and then making all paths through the function after the first allocation has been done go through a single common epilogue that just releases everything - where all the releases skip on a NULL pointer.

Fixes 57af281e5389 ("rxrpc: Tidy up abort generation infrastructure")
Fixes ec832bd06d6f ("rxrpc: Don't retain the server key in the connection")
cc Marc Dionne <[email protected]>
cc Jeffrey Altman <[email protected]>
cc Eric Dumazet <[email protected]>
cc "David S. Miller" <[email protected]>
cc Jakub Kicinski <[email protected]>
cc Paolo Abeni <[email protected]>
cc Simon Horman <[email protected]>
---
 net/rxrpc/rxkad.c | 103 +++++++++++++++++++---------------------------
 1 file changed, 42 insertions(+), 61 deletions(-)
net/rxrpc/rxkad.c +42 -61
index eb7f2769d2b1..5a720222854f 100644
@@ -1136,7 +1136,7 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
 	struct rxrpc_crypt session_key;
 	struct key *server_key;
 	time64_t expiry;
-	void *ticket;
+	void *ticket = NULL;
 	u32 version, kvno, ticket_len, level;
 	__be32 csum;
 	int ret, i;
@@ -1162,13 +1162,13 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
 	ret = -ENOMEM;
 	response = kzalloc_obj(struct rxkad_response, GFP_NOFS);
 	if (!response)
-		goto temporary_error;
+		goto 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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADPACKETSHORT, -EPROTO,
+				       rxkad_abort_resp_short);
+		goto error;
 	}
 
 	version = ntohl(response->version);
@@ -1178,62 +1178,62 @@ 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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADINCONSISTENCY, -EPROTO,
+				       rxkad_abort_resp_version);
+		goto error;
 	}
 
 	if (ticket_len < 4 || ticket_len > MAXKRB5TICKETLEN) {
-		rxrpc_abort_conn(conn, skb, RXKADTICKETLEN, -EPROTO,
-				 rxkad_abort_resp_tkt_len);
-		goto protocol_error;
+		ret = rxrpc_abort_conn(conn, skb, RXKADTICKETLEN, -EPROTO,
+				       rxkad_abort_resp_tkt_len);
+		goto error;
 	}
 
 	if (kvno >= RXKAD_TKT_TYPE_KERBEROS_V5) {
-		rxrpc_abort_conn(conn, skb, RXKADUNKNOWNKEY, -EPROTO,
-				 rxkad_abort_resp_unknown_tkt);
-		goto protocol_error;
+		ret = rxrpc_abort_conn(conn, skb, RXKADUNKNOWNKEY, -EPROTO,
+				       rxkad_abort_resp_unknown_tkt);
+		goto error;
 	}
 
 	/* extract the kerberos ticket and decrypt and decode it */
 	ret = -ENOMEM;
 	ticket = kmalloc(ticket_len, GFP_NOFS);
 	if (!ticket)
-		goto temporary_error_free_resp;
+		goto 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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADPACKETSHORT, -EPROTO,
+				       rxkad_abort_resp_short_tkt);
+		goto error;
 	}
 
 	ret = rxkad_decrypt_ticket(conn, server_key, skb, ticket, ticket_len,
 				   &session_key, &expiry);
 	if (ret < 0)
-		goto temporary_error_free_ticket;
+		goto 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 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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+				       rxkad_abort_resp_bad_param);
+		goto error;
 	}
 
 	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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+				       rxkad_abort_resp_bad_checksum);
+		goto error;
 	}
 
 	for (i = 0; i < RXRPC_MAXCALLS; i++) {
@@ -1241,38 +1241,38 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
 		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;
+			ret = rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+					       rxkad_abort_resp_bad_callid);
+			goto error;
 		}
 
 		if (call_id < counter) {
-			rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
-					 rxkad_abort_resp_call_ctr);
-			goto protocol_error_free;
+			ret = rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+					       rxkad_abort_resp_call_ctr);
+			goto error;
 		}
 
 		if (call_id > counter) {
 			if (conn->channels[i].call) {
-				rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
+				ret = rxrpc_abort_conn(conn, skb, RXKADSEALEDINCON, -EPROTO,
 						 rxkad_abort_resp_call_state);
-				goto protocol_error_free;
+				goto error;
 			}
 			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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADOUTOFSEQUENCE, -EPROTO,
+				       rxkad_abort_resp_ooseq);
+		goto error;
 	}
 
 	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;
+		ret = rxrpc_abort_conn(conn, skb, RXKADLEVELFAIL, -EPROTO,
+				       rxkad_abort_resp_level);
+		goto error;
 	}
 	conn->security_level = level;
 
@@ -1280,31 +1280,12 @@ static int rxkad_verify_response(struct rxrpc_connection *conn,
 	 * this the connection security can be handled in exactly the same way
 	 * 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);
-	_leave(" = 0");
-	return 0;
 
-protocol_error_free:
-	kfree(ticket);
-protocol_error:
-	kfree(response);
-	key_put(server_key);
-	return -EPROTO;
-
-temporary_error_free_ticket:
+error:
 	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);
+	_leave(" = %d", ret);
 	return ret;
 }
 
author maintainer
#2 lore raw

PATCH v2 2/6 rxrpc: Fix rxkad crypto unalignment handling

In reply to 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")
cc Marc Dionne <[email protected]>
cc Jeffrey Altman <[email protected]>
cc Eric Dumazet <[email protected]>
cc "David S. Miller" <[email protected]>
cc Jakub Kicinski <[email protected]>
cc Paolo Abeni <[email protected]>
cc Simon Horman <[email protected]>
---
 include/trace/events/rxrpc.h | 1 +
 net/rxrpc/rxkad.c            | 9 +++++++--
 2 files changed, 8 insertions(+), 2 deletions(-)
include/trace/events/rxrpc.h +1 -0
index 578b8038b211..5820d7e41ea0 100644
@@ -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")	\
net/rxrpc/rxkad.c +7 -2
index 5a720222854f..cba7935977f0 100644
@@ -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 */
author maintainer
#3 lore raw

PATCH v2 3/6 rxrpc: Fix potential UAF after skb_unshare() failure

In reply to David Howells

If skb_unshare() fails to unshare a packet due to allocation failure in rxrpc_input_packet(), the skb pointer in the parent (rxrpc_io_thread()) will be NULL'd out. This will likely cause the call to trace_rxrpc_rx_done() to oops.

Fix this by moving the unsharing down to where rxrpc_input_call_event() calls rxrpc_input_call_packet(). There are a number of places prior to that where we ignore DATA packets for a variety of reasons (such as the call already being complete) for which an unshare is then avoided.

And with that, rxrpc_input_packet() doesn't need to take a pointer to the pointer to the packet, so change that to just a pointer.

Fixes 2d1faf7a0ca3 ("rxrpc: Simplify skbuff accounting in receive path")
cc Marc Dionne <[email protected]>
cc Jeffrey Altman <[email protected]>
cc Eric Dumazet <[email protected]>
cc "David S. Miller" <[email protected]>
cc Jakub Kicinski <[email protected]>
cc Paolo Abeni <[email protected]>
cc Simon Horman <[email protected]>
---
 include/trace/events/rxrpc.h |  4 ++--
 net/rxrpc/ar-internal.h      |  1 -
 net/rxrpc/call_event.c       | 19 ++++++++++++++++++-
 net/rxrpc/io_thread.c        | 24 ++----------------------
 net/rxrpc/skbuff.c           |  9 ---------
 5 files changed, 22 insertions(+), 35 deletions(-)
include/trace/events/rxrpc.h +2 -2
index 5820d7e41ea0..13b9d017f8e1 100644
@@ -162,8 +162,6 @@
 	E_(rxrpc_call_poke_timer_now,		"Timer-now")
 
 #define rxrpc_skb_traces \
-	EM(rxrpc_skb_eaten_by_unshare,		"ETN unshare  ") \
-	EM(rxrpc_skb_eaten_by_unshare_nomem,	"ETN unshar-nm") \
 	EM(rxrpc_skb_get_call_rx,		"GET call-rx  ") \
 	EM(rxrpc_skb_get_conn_secured,		"GET conn-secd") \
 	EM(rxrpc_skb_get_conn_work,		"GET conn-work") \
@@ -190,6 +188,7 @@
 	EM(rxrpc_skb_put_purge,			"PUT purge    ") \
 	EM(rxrpc_skb_put_purge_oob,		"PUT purge-oob") \
 	EM(rxrpc_skb_put_response,		"PUT response ") \
+	EM(rxrpc_skb_put_response_copy,		"PUT resp-cpy ") \
 	EM(rxrpc_skb_put_rotate,		"PUT rotate   ") \
 	EM(rxrpc_skb_put_unknown,		"PUT unknown  ") \
 	EM(rxrpc_skb_see_conn_work,		"SEE conn-work") \
@@ -198,6 +197,7 @@
 	EM(rxrpc_skb_see_recvmsg_oob,		"SEE recvm-oob") \
 	EM(rxrpc_skb_see_reject,		"SEE reject   ") \
 	EM(rxrpc_skb_see_rotate,		"SEE rotate   ") \
+	EM(rxrpc_skb_see_unshare_nomem,		"SEE unshar-nm") \
 	E_(rxrpc_skb_see_version,		"SEE version  ")
 
 #define rxrpc_local_traces \
net/rxrpc/ar-internal.h +0 -1
index 96ecb83c9071..27c2aa2dd023 100644
@@ -1486,7 +1486,6 @@ 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);
 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 *);
net/rxrpc/call_event.c +18 -1
index fec59d9338b9..cc8f9dfa44e8 100644
@@ -332,7 +332,24 @@ bool rxrpc_input_call_event(struct rxrpc_call *call)
 
 			saw_ack |= sp->hdr.type == RXRPC_PACKET_TYPE_ACK;
 
-			rxrpc_input_call_packet(call, skb);
+			if (sp->hdr.securityIndex != 0 &&
+			    skb_cloned(skb)) {
+				/* Unshare the packet so that it can be
+				 * modified by in-place decryption.
+				 */
+				struct sk_buff *nskb = skb_copy(skb, GFP_ATOMIC);
+
+				if (nskb) {
+					rxrpc_new_skb(nskb, rxrpc_skb_new_unshared);
+					rxrpc_input_call_packet(call, nskb);
+					rxrpc_free_skb(nskb, rxrpc_skb_put_call_rx);
+				} else {
+					/* OOM - Drop the packet. */
+					rxrpc_see_skb(skb, rxrpc_skb_see_unshare_nomem);
+				}
+			} else {
+				rxrpc_input_call_packet(call, skb);
+			}
 			rxrpc_free_skb(skb, rxrpc_skb_put_call_rx);
 			did_receive = true;
 		}
net/rxrpc/io_thread.c +2 -22
index 697956931925..dc5184a2fa9d 100644
@@ -192,13 +192,12 @@ static bool rxrpc_extract_abort(struct sk_buff *skb)
 /*
  * Process packets received on the local endpoint
  */
-static bool rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff **_skb)
+static bool rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff *skb)
 {
 	struct rxrpc_connection *conn;
 	struct sockaddr_rxrpc peer_srx;
 	struct rxrpc_skb_priv *sp;
 	struct rxrpc_peer *peer = NULL;
-	struct sk_buff *skb = *_skb;
 	bool ret = false;
 
 	skb_pull(skb, sizeof(struct udphdr));
@@ -244,25 +243,6 @@ static bool rxrpc_input_packet(struct rxrpc_local *local, struct sk_buff **_skb)
 			return rxrpc_bad_message(skb, rxrpc_badmsg_zero_call);
 		if (sp->hdr.seq == 0)
 			return rxrpc_bad_message(skb, rxrpc_badmsg_zero_seq);
-
-		/* Unshare the packet so that it can be modified for in-place
-		 * 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;
-				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);
-			}
-		}
 		break;
 
 	case RXRPC_PACKET_TYPE_CHALLENGE:
@@ -494,7 +474,7 @@ int rxrpc_io_thread(void *data)
 			switch (skb->mark) {
 			case RXRPC_SKB_MARK_PACKET:
 				skb->priority = 0;
-				if (!rxrpc_input_packet(local, &skb))
+				if (!rxrpc_input_packet(local, skb))
 					rxrpc_reject_packet(local, skb);
 				trace_rxrpc_rx_done(skb->mark, skb->priority);
 				rxrpc_free_skb(skb, rxrpc_skb_put_input);
net/rxrpc/skbuff.c +0 -9
index 3bcd6ee80396..e2169d1a14b5 100644
@@ -46,15 +46,6 @@ void rxrpc_get_skb(struct sk_buff *skb, enum rxrpc_skb_trace why)
 	skb_get(skb);
 }
 
-/*
- * Note the dropping of a ref on a socket buffer by the core.
- */
-void rxrpc_eaten_skb(struct sk_buff *skb, enum rxrpc_skb_trace why)
-{
-	int n = atomic_inc_return(&rxrpc_n_rx_skbs);
-	trace_rxrpc_skb(skb, 0, n, why);
-}
-
 /*
  * Note the destruction of a socket buffer.
  */
author maintainer
#4 lore raw

PATCH v2 4/6 rxrpc: Fix conn-level packet handling to unshare RESPONSE packets

In reply to 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 handing a copy of the packet off to the specific security handler if the packet was cloned.

Fixes 17926a79320a ("[AF_RXRPC]: Provide secure RxRPC sockets for use by userspace and kernel both")
cc Marc Dionne <[email protected]>
cc Jeffrey Altman <[email protected]>
cc Eric Dumazet <[email protected]>
cc "David S. Miller" <[email protected]>
cc Jakub Kicinski <[email protected]>
cc Paolo Abeni <[email protected]>
cc Simon Horman <[email protected]>
---
 net/rxrpc/conn_event.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)
net/rxrpc/conn_event.c +28 -1
index 9a41ec708aeb..aee977291d90 100644
@@ -240,6 +240,33 @@ static void rxrpc_call_is_secure(struct rxrpc_call *call)
 		rxrpc_notify_socket(call);
 }
 
+static int rxrpc_verify_response(struct rxrpc_connection *conn,
+				 struct sk_buff *skb)
+{
+	int ret;
+
+	if (skb_cloned(skb)) {
+		/* Copy the packet if shared so that we can do in-place
+		 * decryption.
+		 */
+		struct sk_buff *nskb = skb_copy(skb, GFP_NOFS);
+
+		if (nskb) {
+			rxrpc_new_skb(nskb, rxrpc_skb_new_unshared);
+			ret = conn->security->verify_response(conn, nskb);
+			rxrpc_free_skb(nskb, rxrpc_skb_put_response_copy);
+		} else {
+			/* OOM - Drop the packet. */
+			rxrpc_see_skb(skb, rxrpc_skb_see_unshare_nomem);
+			ret = -ENOMEM;
+		}
+	} else {
+		ret = conn->security->verify_response(conn, skb);
+	}
+
+	return ret;
+}
+
 /*
  * connection-level Rx packet processor
  */
@@ -270,7 +297,7 @@ static int rxrpc_process_event(struct rxrpc_connection *conn,
 		}
 		spin_unlock_irq(&conn->state_lock);
 
-		ret = conn->security->verify_response(conn, skb);
+		ret = rxrpc_verify_response(conn, skb);
 		if (ret < 0)
 			return ret;
 
author maintainer
#5 lore raw

PATCH v2 5/6 rxgk: Fix potential integer overflow in length check

In reply to 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")
cc Marc Dionne <[email protected]>
cc Jeffrey Altman <[email protected]>
cc Eric Dumazet <[email protected]>
cc "David S. Miller" <[email protected]>
cc Jakub Kicinski <[email protected]>
cc Paolo Abeni <[email protected]>
cc Simon Horman <[email protected]>
---
 net/rxrpc/rxgk_app.c    | 2 +-
 net/rxrpc/rxgk_common.h | 1 +
 2 files changed, 2 insertions(+), 1 deletion(-)
net/rxrpc/rxgk_app.c +1 -1
index 30275cb5ba3e..5587639d60c5 100644
@@ -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);
net/rxrpc/rxgk_common.h +1 -0
index 80164d89e19c..1e257d7ab8ec 100644
@@ -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))
 
 /*
author maintainer
#6 lore raw

PATCH v2 6/6 rxrpc: Fix missing validation of ticket length in non-XDR key preparsing

In reply to David Howells

From: Anderson Nascimento <[email protected]>

In rxrpc_preparse(), there are two paths for parsing key payloads: the XDR path (for large payloads) and the non-XDR path (for payloads <= 28 bytes). While the XDR path (rxrpc_preparse_xdr_rxkad()) correctly validates the ticket length against AFSTOKEN_RK_TIX_MAX, the non-XDR path fails to do so.

This allows an unprivileged user to provide a very large ticket length. When this key is later read via rxrpc_read(), the total token size (toksize) calculation results in a value that exceeds AFSTOKEN_LENGTH_MAX, triggering a WARN_ON().

[ 2001.302904] WARNING: CPU: 2 PID: 2108 at net/rxrpc/key.c:778 rxrpc_read+0x109/0x5c0 [rxrpc]

Fix this by adding a check in the non-XDR parsing path of rxrpc_preparse() to ensure the ticket length does not exceed AFSTOKEN_RK_TIX_MAX, bringing it into parity with the XDR parsing logic.

Fixes 8a7a3eb4ddbe ("KEYS: RxRPC: Use key preparsing")
Fixes 84924aac08a4 ("rxrpc: Fix checker warning")
cc Marc Dionne <[email protected]>
cc Jeffrey Altman <[email protected]>
cc Eric Dumazet <[email protected]>
cc "David S. Miller" <[email protected]>
cc Jakub Kicinski <[email protected]>
cc Paolo Abeni <[email protected]>
cc Simon Horman <[email protected]>
---
 net/rxrpc/key.c | 4 ++++
 1 file changed, 4 insertions(+)
net/rxrpc/key.c +4 -0
index 6301d79ee35a..5ebb06d87cdd 100644
@@ -502,6 +502,10 @@ static int rxrpc_preparse(struct key_preparsed_payload *prep)
 	if (v1->security_index != RXRPC_SECURITY_RXKAD)
 		goto error;
 
+	ret = -EKEYREJECTED;
+	if(v1->ticket_length > AFSTOKEN_RK_TIX_MAX)
+		goto error;
+
 	plen = sizeof(*token->kad) + v1->ticket_length;
 	prep->quotalen += plen + sizeof(*token);
 
maintainer
#7 lore raw
In reply to David Howells
Quoted 7 lines · On Wed, 22 Apr 2026 17:14:29 +0100 David Howells wrote:

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

(1) Fix leaks in rxkad_verify_response().

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

Quoted 8 lines

(3) Fix problem with unsharing DATA packets potentially causing a crash in the caller.

(4) Fix lack of unsharing of RESPONSE packets.

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

(6) Fix missing length check in RxKAD tickets.

Have you had a chance to look thru sashiko run for this?

https://sashiko.dev/#/patchset/[email protected]

maintainer
#9 lore raw
Quoted 8 lines · On Thu, 23 Apr 2026 19:47:22 +0100 David Howells wrote:
Quoted 3 lines · Jakub Kicinski

Have you had a chance to look thru sashiko run for this?

https://sashiko.dev/#/patchset/[email protected]

It wasn't available this morning when I looked. I know there are a couple of things to fix from other checks.

It is now :) May I offer the following menu of options
- all the complaints are already taken into account / intentional / will be addressed later
- I need more time, let's wait until tomorrow
- some of it looks legit, apply patches ${list} only
- let me respin completely

maintainer
#10 lore raw
Quoted 10 lines · On Thu, 23 Apr 2026 11:58:53 -0700 Jakub Kicinski wrote:
Quoted 8 lines · On Thu, 23 Apr 2026 19:47:22 +0100 David Howells wrote:
Quoted 3 lines · Jakub Kicinski

Have you had a chance to look thru sashiko run for this?

https://sashiko.dev/#/patchset/[email protected]

It wasn't available this morning when I looked. I know there are a couple of things to fix from other checks.

Quoted 6 lines

It is now :) May I offer the following menu of options
- all the complaints are already taken into account / intentional / will be addressed later
- I need more time, let's wait until tomorrow
- some of it looks legit, apply patches ${list} only
- let me respin completely

I've been hacking up Sashiko on top of Claude in these past few days, and it's far less nit picky. For this series it doesn't flag a thing. I'll just pull this in, if you could LMK whether any of the "real Sashiko" reports are actually valid commentary on the current patches I'd greatly appreciate.

author maintainer
#11 lore raw
Quoted 5 lines · Jakub Kicinski

I've been hacking up Sashiko on top of Claude in these past few days, and it's far less nit picky. For this series it doesn't flag a thing. I'll just pull this in, if you could LMK whether any of the "real Sashiko" reports are actually valid commentary on the current patches I'd greatly appreciate.

They're all valid comments. I've expanded my series by two further patches and modified two others (one just a trivial missing space) and will post a v3 shortly.

David

maintainer
#13 lore raw
In reply to David Howells
Quoted 11 lines · On Thu, 23 Apr 2026 20:56:25 +0100 David Howells wrote:
Quoted 5 lines · Jakub Kicinski

I've been hacking up Sashiko on top of Claude in these past few days, and it's far less nit picky. For this series it doesn't flag a thing. I'll just pull this in, if you could LMK whether any of the "real Sashiko" reports are actually valid commentary on the current patches I'd greatly appreciate.

They're all valid comments. I've expanded my series by two further patches and modified two others (one just a trivial missing space) and will post a v3 shortly.

There goes my hope that we found a way to reduce the noise.

#14 lore raw
In reply to David Howells
This series was applied to netdev/net.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net.git (main)
by Jakub Kicinski <[email protected]>:

Quoted 8 lines · On Wed, 22 Apr 2026 17:14:29 +0100 you wrote:

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

(1) Fix leaks in rxkad_verify_response().

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

[...]

Here is the summary with links:
- [net,v2,1/6] rxrpc: Fix memory leaks in rxkad_verify_response()

    https://git.kernel.org/netdev/net/c/34f61a07e0cd
  - [net,v2,2/6] rxrpc: Fix rxkad crypto unalignment handling
    https://git.kernel.org/netdev/net/c/def304aae2ed
  - [net,v2,3/6] rxrpc: Fix potential UAF after skb_unshare() failure
    https://git.kernel.org/netdev/net/c/1f2740150f90
  - [net,v2,4/6] rxrpc: Fix conn-level packet handling to unshare RESPONSE packets
    https://git.kernel.org/netdev/net/c/24481a7f5733
  - [net,v2,5/6] rxgk: Fix potential integer overflow in length check
    https://git.kernel.org/netdev/net/c/6929350080f4
  - [net,v2,6/6] rxrpc: Fix missing validation of ticket length in non-XDR key preparsing
    https://git.kernel.org/netdev/net/c/ac33733b10b4

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html