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 for Android: free password hash cracker in your pocket

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

Message-ID: <67c099b8e7293_37f92929454@willemb.c.googlers.com.notmuch>
Date: Thu, 27 Feb 2025 11:58:32 -0500
From: Willem de Bruijn <willemdebruijn.kernel@...il.com>
To: Antoine Tenart <atenart@...nel.org>, 
 davem@...emloft.net, 
 kuba@...nel.org, 
 pabeni@...hat.com, 
 edumazet@...gle.com
Cc: Antoine Tenart <atenart@...nel.org>, 
 netdev@...r.kernel.org, 
 willemdebruijn.kernel@...il.com, 
 pshelar@....org
Subject: Re: [PATCH net] net: gso: fix ownership in __udp_gso_segment

Antoine Tenart wrote:
> 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.
> 
> Fix this by also removing the reference to the socket in
> __udp_gso_segment.
> 
> Fixes: ad405857b174 ("udp: better wmem accounting on gso")
> Signed-off-by: Antoine Tenart <atenart@...nel.org>
> ---
>  net/ipv4/udp_offload.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/net/ipv4/udp_offload.c b/net/ipv4/udp_offload.c
> index a5be6e4ed326..ecfca59f31f1 100644
> --- a/net/ipv4/udp_offload.c
> +++ b/net/ipv4/udp_offload.c
> @@ -321,13 +321,17 @@ struct sk_buff *__udp_gso_segment(struct sk_buff *gso_skb,
>  
>  	/* clear destructor to avoid skb_segment assigning it to tail */
>  	copy_dtor = gso_skb->destructor == sock_wfree;
> -	if (copy_dtor)
> +	if (copy_dtor) {
>  		gso_skb->destructor = NULL;
> +		gso_skb->sk = NULL;
> +	}
>  
>  	segs = skb_segment(gso_skb, features);
>  	if (IS_ERR_OR_NULL(segs)) {
> -		if (copy_dtor)
> +		if (copy_dtor) {
>  			gso_skb->destructor = sock_wfree;
> +			gso_skb->sk = sk;
> +		}
>  		return segs;
>  	}
>  
> -- 
> 2.48.1
> 

I'm not very familiar with the details of the OVS datapath. And how
the gso_skb can be accessed in this state, which is intended to be
temporary and local to this function.

But in light of the skb_orphan sanity check (below), this patch looks
fine to me.

static inline void skb_orphan(struct sk_buff *skb)
{
	if (skb->destructor) {
		skb->destructor(skb);
		skb->destructor = NULL;
		skb->sk         = NULL;
	} else {
		BUG_ON(skb->sk);
	}
}

commit 376c7311bdb6efea3322310333576a04d73fbe4c
Author: Eric Dumazet <edumazet@...gle.com>
Date:   Thu Aug 1 11:43:08 2013 -0700

    net: add a temporary sanity check in skb_orphan()
    
    David suggested to add a BUG_ON() to catch if some layer
    sets skb->sk pointer without a corresponding destructor.
    
    As skb can sit in a queue, it's mandatory to make sure the
    socket cannot disappear, and it's usually done by taking a
    reference on the socket, then releasing it from the skb
    destructor.
    
    This patch is a follow-up to commit c34a761231b5
    ("net: skb_orphan() changes") and will be reverted after
    catching all possible offenders if any.