rxrpc: Fix potential UAF after skb_unshare() failure

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")
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: Simon Horman <horms@kernel.org>
cc: linux-afs@lists.infradead.org
cc: stable@kernel.org
Link: https://patch.msgid.link/20260422161438.2593376-4-dhowells@redhat.com
Signed-off-by: Jakub Kicinski <kuba@kernel.org>

diff --git a/include/trace/events/rxrpc.h b/include/trace/events/rxrpc.h
index 5820d7e41ea09a00279e9904a8d43b32e64d2cba..13b9d017f8e177239c47e2ba425f8dcdafd48a2b 100644 (file)
--- a/include/trace/events/rxrpc.h
+++ b/include/trace/events/rxrpc.h
@@ -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 \

diff --git a/net/rxrpc/ar-internal.h b/net/rxrpc/ar-internal.h
index 96ecb83c90715352e687fa7ba953c21d7b26cee5..27c2aa2dd023c56197ec9afd438f1c68d72b6e50 100644 (file)
--- a/net/rxrpc/ar-internal.h
+++ b/net/rxrpc/ar-internal.h
@@ -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 *);

diff --git a/net/rxrpc/call_event.c b/net/rxrpc/call_event.c
index fec59d9338b9fb1a5f436ee816e55eea94ca511e..cc8f9dfa44e8a0f06c62467187d1ba44276caa72 100644 (file)
--- a/net/rxrpc/call_event.c
+++ b/net/rxrpc/call_event.c
@@ -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;
                }

diff --git a/net/rxrpc/io_thread.c b/net/rxrpc/io_thread.c
index 6979569319252199f3422ef9831436f8d2b385c2..dc5184a2fa9d1ac9d4e12624d4740b24176b421a 100644 (file)
--- a/net/rxrpc/io_thread.c
+++ b/net/rxrpc/io_thread.c
@@ -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);

diff --git a/net/rxrpc/skbuff.c b/net/rxrpc/skbuff.c
index 3bcd6ee803960ba3280baf0f77508905c57d6c42..e2169d1a14b5fd0b947be96020f195a6960c7ac6 100644 (file)
--- a/net/rxrpc/skbuff.c
+++ b/net/rxrpc/skbuff.c
@@ -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.
  */