netdev - Re: linux 3.4.43 : kernel crash at __nf_conntrack

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

Message-ID: <20151018214050.GD4386@breakpoint.cc>
Date:	Sun, 18 Oct 2015 23:40:50 +0200
From:	Florian Westphal <fw@...len.de>
To:	Ani Sinha <ani@...sta.com>
Cc:	Florian Westphal <fw@...len.de>, Patrick McHardy <kaber@...sh.net>,
	"David S. Miller" <davem@...emloft.net>,
	netfilter-devel@...r.kernel.org, netfilter@...r.kernel.org,
	coreteam@...filter.org,
	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Subject: Re: linux 3.4.43 : kernel crash at __nf_conntrack_confirm

Ani Sinha <ani@...sta.com> wrote:
> Indeed. So it seems to me that we have run into one another such case.
> In patch c6825c0976fa7893692, I see we have added an additional check (along with comparing tuple and zone) to verify that if the conntrack is confirmed.
>  
> +       return nf_ct_tuple_equal(tuple, &h->tuple) &&
> +               nf_ct_zone(ct) == zone &&
> +               nf_ct_is_confirmed(ct);
> 
> This is necessary since it's possible that a conntrack can be recreated with the same zone.
> Unfortunately, we leave a hole open in __nf_conntrack_confirm() because this routine _is_ responsible
> for confirming the conntrack. We cannot use the same logic here.

Hmm, why?

I don't understand why we need to change __nf_conntrack_confirm(), can
you elaborate?

At __nf_conntrack_confirm call time, only one cpu can see this nfct entry.
Other cpus on read-side can see it due to object re-use but any of the
following tests should fail:

1. different tuples
2. differnet zones
3. CONFIRMED not set

So they would skip entry and restart lookup (NULs value mismatch).

> Should I send a patch along the lines of :
>  
> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
> index 71935fc..6ff4088 100644
> --- a/net/netfilter/nf_conntrack_core.c
> +++ b/net/netfilter/nf_conntrack_core.c
> @@ -535,6 +535,12 @@ __nf_conntrack_confirm(struct sk_buff *skb)
>  		    zone == nf_ct_zone(nf_ct_tuplehash_to_ctrack(h)))
>  			goto out;
>  
> +	/* we might be racing against a case where the conntrack was deleted 
> +	   and a new conntrack was initialized with the exact same zone. We
> +	   need to make sure that the conntrack node is in the hashtable */

?

The conntrack is NOT in the hashtable at this point.  Its not even on
the unconfirmed list since we already removed it in preparation of
hashtable insertion.

> +	if (hlist_nulls_unhashed(&ct->tuplehash[IP_CT_DIR_ORIGINAL].hnnode))
> +	  goto out;

That would be a bug, how can ->nfct be confirmed twice?

If you're talking about IPS_CONFIRMED getting set -- that should be
harmless.  In some theoretical condition we could indeed observe this
nfct on another cpu, just before we actually insert this but this does
not cause a problem on the read-side since the conntrack matches the
tuple exactly and all extensions have been initialized.

And if we create two conntracks with identical tuples on different CPUs
which is possible regardless of RCU this will be detected during
confirm step (we search ht for a colliding tuple).

So, if there is a problem please describe in more detail, I don't see
anything wrong so far.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html