netdev - Re: [net 4/9] net/mlx5e: Fix refcount leak on kTLS RX resync

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: <0c929b0f-750a-3618-3891-4fa40dd14104@nvidia.com>
Date:   Thu, 5 Nov 2020 12:15:04 +0200
From:   Maxim Mikityanskiy <maximmi@...dia.com>
To:     Jakub Kicinski <kuba@...nel.org>,
        Saeed Mahameed <saeedm@...dia.com>
CC:     "David S. Miller" <davem@...emloft.net>, <netdev@...r.kernel.org>,
        "Maxim Mikityanskiy" <maximmi@...lanox.com>,
        Tariq Toukan <tariqt@...dia.com>
Subject: Re: [net 4/9] net/mlx5e: Fix refcount leak on kTLS RX resync

On 2020-11-05 00:59, Jakub Kicinski wrote:
> On Tue, 3 Nov 2020 11:18:25 -0800 Saeed Mahameed wrote:
>> From: Maxim Mikityanskiy <maximmi@...lanox.com>
>>
>> On resync, the driver calls inet_lookup_established
>> (__inet6_lookup_established) that increases sk_refcnt of the socket. To
>> decrease it, the driver set skb->destructor to sock_edemux. However, it
>> didn't work well, because the TCP stack also sets this destructor for
>> early demux, and the refcount gets decreased only once
> 
> Why is the stack doing early_demux if there is already a socket
> assigned? Or is it not early_demux but something else?
> Can you point us at the code?

I thought this was the code that was in conflict with setting 
skb->destructor in the driver:

void skb_set_owner_w(struct sk_buff *skb, struct sock *sk)
{
         skb_orphan(skb);
         skb->sk = sk;
#ifdef CONFIG_INET
         if (unlikely(!sk_fullsock(sk))) {
                 skb->destructor = sock_edemux;
                 sock_hold(sk);
                 return;
         }
#endif

However, after taking another look, it seems that the root cause is 
somewhere else. This piece of code actually calls skb_orphan before 
reassigning the destructor.

I'll debug it further to try to find where the destructor is replaced or 
just not called.

> IPv4:
> 	if (net->ipv4.sysctl_ip_early_demux &&
> 	    !skb_dst(skb) &&
> 	    !skb->sk &&                              <============
> 	    !ip_is_fragment(iph)) {
> 		const struct net_protocol *ipprot;
> 		int protocol = iph->protocol;
> 
> 		ipprot = rcu_dereference(inet_protos[protocol]);
> 		if (ipprot && (edemux = READ_ONCE(ipprot->early_demux))) {
> 			err = INDIRECT_CALL_2(edemux, tcp_v4_early_demux,
> 					      udp_v4_early_demux, skb);
> 			if (unlikely(err))
> 				goto drop_error;
> 			/* must reload iph, skb->head might have changed */
> 			iph = ip_hdr(skb);
> 		}
> 	}
> 
> IPv6:
> 	if (net->ipv4.sysctl_ip_early_demux && !skb_dst(skb) && skb->sk == NULL) {
>                                                                  ~~~~~~~~~~~~~~~
> 		const struct inet6_protocol *ipprot;
> 
> 		ipprot = rcu_dereference(inet6_protos[ipv6_hdr(skb)->nexthdr]);
> 		if (ipprot && (edemux = READ_ONCE(ipprot->early_demux)))
> 			INDIRECT_CALL_2(edemux, tcp_v6_early_demux,
> 					udp_v6_early_demux, skb);
> 	}
>