netdev - Re: [PATCH net] net: gso: fix ownership in __udp_gso

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <174059040728.99819.2585982147439431813@kwain>
Date: Wed, 26 Feb 2025 18:20:07 +0100
From: Antoine Tenart <atenart@...nel.org>
To: davem@...emloft.net, edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com
Cc: netdev@...r.kernel.org, willemdebruijn.kernel@...il.com, pshelar@....org
Subject: Re: [PATCH net] net: gso: fix ownership in __udp_gso_segment

Quoting Antoine Tenart (2025-02-26 18:13:42)
> In __udp_gso_segment the skb destructor is removed before segmenting the
> skb but the socket reference is kept as-is. This is an issue if the
> original skb is later orphaned as we can hit the following bug:
> 
>   kernel BUG at ./include/linux/skbuff.h:3312!  (skb_orphan)
>   RIP: 0010:ip_rcv_core+0x8b2/0xca0
>   Call Trace:
>    ip_rcv+0xab/0x6e0
>    __netif_receive_skb_one_core+0x168/0x1b0
>    process_backlog+0x384/0x1100
>    __napi_poll.constprop.0+0xa1/0x370
>    net_rx_action+0x925/0xe50
> 
> The above can happen following a sequence of events when using
> OpenVSwitch, when an OVS_ACTION_ATTR_USERSPACE action precedes an
> OVS_ACTION_ATTR_OUTPUT action:
> 
> 1. OVS_ACTION_ATTR_USERSPACE is handled (in do_execute_actions): the skb
>    goes through queue_gso_packets and then __udp_gso_segment, where its
>    destructor is removed.
> 2. The segments' data are copied and sent to userspace.
> 3. OVS_ACTION_ATTR_OUTPUT is handled (in do_execute_actions) and the
>    same original skb is sent to its path.
> 4. If it later hits skb_orphan, we hit the bug.

While AFAIU removing the skb destructor while not removing the socket
reference is not often wanted (we could add an helper as it's not the
first time we hit this?), I also looked at the OvS side for
completeness. The following also fixes the above,

--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -1407,15 +1407,19 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
                        break;
                }
 
-               case OVS_ACTION_ATTR_USERSPACE:
-                       output_userspace(dp, skb, key, a, attr,
-                                                    len, OVS_CB(skb)->cutlen);
+               case OVS_ACTION_ATTR_USERSPACE: {
+                       struct sk_buff *clone;
+
+                       clone = nla_is_last(a, rem) ? skb : skb_clone(skb, GFP_ATOMIC);
+                       if (clone)
+                               output_userspace(dp, clone, key, a, attr, len,
+                                                OVS_CB(skb)->cutlen);
                        OVS_CB(skb)->cutlen = 0;
-                       if (nla_is_last(a, rem)) {
-                               consume_skb(skb);
+                       consume_skb(clone);
+                       if (nla_is_last(a, rem))
                                return 0;
-                       }
                        break;
+               }

output_userspace could be seen as a different path where cloning is
wanted, but at the same time it's only copying the packet content and I
didn't see a reason not to allow reusing the original skb after calling
__skb_gso_segment in the GSO case.

Thoughts?

Thanks!
Antoine