[NETFILTER]: nf_conntrack: fix use-after-free in helper destroy callback invocation

When the helper module is removed for a master connection that has a
fulfilled expectation, but has already timed out and got removed from
the hash tables, nf_conntrack_helper_unregister can't find the master
connection to unset the helper, causing a use-after-free when the
expected connection is destroyed and releases the last reference to
the master.

The helper destroy callback was introduced for the PPtP helper to clean
up expectations and expected connections when the master connection
times out, but doing this from destroy_conntrack only works for
unfulfilled expectations since expected connections hold a reference
to the master, preventing its destruction. Move the destroy callback to
the timeout function, which fixes both problems.

Reported/tested by Gabor Burjan <buga@buvoshetes.hu>.

Signed-off-by: Patrick McHardy <kaber@trash.net>
Signed-off-by: David S. Miller <davem@davemloft.net>

authored by Patrick McHardy and committed by David S. Miller 5397e97d c92b3a2f

+4 -4
+4 -4
net/netfilter/nf_conntrack_core.c
··· 298 destroy_conntrack(struct nf_conntrack *nfct) 299 { 300 struct nf_conn *ct = (struct nf_conn *)nfct; 301 - struct nf_conn_help *help = nfct_help(ct); 302 struct nf_conntrack_l4proto *l4proto; 303 typeof(nf_conntrack_destroyed) destroyed; 304 ··· 307 308 nf_conntrack_event(IPCT_DESTROY, ct); 309 set_bit(IPS_DYING_BIT, &ct->status); 310 - 311 - if (help && help->helper && help->helper->destroy) 312 - help->helper->destroy(ct); 313 314 /* To make sure we don't get any weird locking issues here: 315 * destroy_conntrack() MUST NOT be called with a write lock ··· 349 static void death_by_timeout(unsigned long ul_conntrack) 350 { 351 struct nf_conn *ct = (void *)ul_conntrack; 352 353 write_lock_bh(&nf_conntrack_lock); 354 /* Inside lock so preempt is disabled on module removal path.
··· 298 destroy_conntrack(struct nf_conntrack *nfct) 299 { 300 struct nf_conn *ct = (struct nf_conn *)nfct; 301 struct nf_conntrack_l4proto *l4proto; 302 typeof(nf_conntrack_destroyed) destroyed; 303 ··· 308 309 nf_conntrack_event(IPCT_DESTROY, ct); 310 set_bit(IPS_DYING_BIT, &ct->status); 311 312 /* To make sure we don't get any weird locking issues here: 313 * destroy_conntrack() MUST NOT be called with a write lock ··· 353 static void death_by_timeout(unsigned long ul_conntrack) 354 { 355 struct nf_conn *ct = (void *)ul_conntrack; 356 + struct nf_conn_help *help = nfct_help(ct); 357 + 358 + if (help && help->helper && help->helper->destroy) 359 + help->helper->destroy(ct); 360 361 write_lock_bh(&nf_conntrack_lock); 362 /* Inside lock so preempt is disabled on module removal path.