netdev - Re: nf_conntrack_in() - is there a leak here?

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>] [day] [month] [year] [list]

Message-ID: <20191007220847.s73l3x5tt74bzdxf@salvia>
Date:   Tue, 8 Oct 2019 00:08:47 +0200
From:   Pablo Neira Ayuso <pablo@...filter.org>
To:     Maciej Żenczykowski <zenczykowski@...il.com>
Cc:     Eric Dumazet <edumazet@...gle.com>,
        Linux NetDev <netdev@...r.kernel.org>
Subject: Re: nf_conntrack_in() - is there a leak here?

Hi,

On Mon, Oct 07, 2019 at 07:10:37AM -0700, Maciej Żenczykowski wrote:
> unsigned int
> nf_conntrack_in(struct sk_buff *skb, const struct nf_hook_state *state)
> {
>         enum ip_conntrack_info ctinfo;
>         struct nf_conn *ct, *tmpl;
>         u_int8_t protonum;
>         int dataoff, ret;
> 
>         tmpl = nf_ct_get(skb, &ctinfo);
>                           <-----------
>         if (tmpl || ctinfo == IP_CT_UNTRACKED) {
>                 /* Previously seen (loopback or untracked)?  Ignore. */
>                 if ((tmpl && !nf_ct_is_template(tmpl)) ||
>                      ctinfo == IP_CT_UNTRACKED) {
>                         NF_CT_STAT_INC_ATOMIC(state->net, ignore);
>                         return NF_ACCEPT;
>                              <----------
>                 }
>                 skb->_nfct = 0;
>         }
> 
>         /* rcu_read_lock()ed by nf_hook_thresh */
>         dataoff = get_l4proto(skb, skb_network_offset(skb), state->pf,
> &protonum);
>         if (dataoff <= 0) {
>                 pr_debug("not prepared to track yet or error occurred\n");
>                 NF_CT_STAT_INC_ATOMIC(state->net, error);
>                 NF_CT_STAT_INC_ATOMIC(state->net, invalid);
>                 ret = NF_ACCEPT;
>                 goto out;
>         }
> 
> ...
> 
> out:
>         if (tmpl)
>                 nf_ct_put(tmpl);
>                        <---------
> 
>         return ret;
> }
> EXPORT_SYMBOL_GPL(nf_conntrack_in);
> 
> ---
> 
> Do we leak a nf_ct_get() on tmpl at that first 'return NF_ACCEPT' ?
> ie. should it be 'ret = NF_ACCEPT; goto out;'

This patch only entered for loopback and untracked traffic, in such
case the special handling for the template is not required (because
there is no template conntrack in place).

> I'm confused by:
>   include/net/netfilter/nf_conntrack.h:65:
>   * beware nf_ct_get() is different and don't inc refcnt.

Yes, this call has this semantics since the very beginning IIRC.

> (internal reference b/141976661 & b/135110479 where we're getting kmemleak
> complaints on 4.14 LTS,
>  which would possibly be shut up by this 4.17 'silence fix', but:)
> 
> I have this gut feeling that:
>   commit 114aa35d06d4920c537b72f9fa935de5dd205260
>   'netfilter: conntrack: silent a memory leak warning'
> is bogus...
> 
> By my understanding of kmemleak, such gymnastics shouldn't be needed.
> And there's no other users in the network stack of kmemleak_not_leak()
> [except for 2 staging drivers].

Probably, are you observing a memleak there in conntrack? I see you
searching for reason :-)