| Message ID | 20191022165642.29698-1-fw@strlen.de |
|---|---|
| State | Accepted |
| Delegated to: | Pablo Neira |
| Headers | show |
| Series | [nf-next] netfilter: ecache: don't look for ecache extension on dying/unconfirmed conntracks | expand |
On Tue, Oct 22, 2019 at 06:56:42PM +0200, Florian Westphal wrote: > syzbot reported following splat: > BUG: KASAN: use-after-free in __nf_ct_ext_exist > include/net/netfilter/nf_conntrack_extend.h:53 [inline] > BUG: KASAN: use-after-free in nf_ct_deliver_cached_events+0x5c3/0x6d0 > net/netfilter/nf_conntrack_ecache.c:205 > nf_conntrack_confirm include/net/netfilter/nf_conntrack_core.h:65 [inline] > nf_confirm+0x3d8/0x4d0 net/netfilter/nf_conntrack_proto.c:154 > [..] > > While there is no reproducer yet, the syzbot report contains one > interesting bit of information: > > Freed by task 27585: > [..] > kfree+0x10a/0x2c0 mm/slab.c:3757 > nf_ct_ext_destroy+0x2ab/0x2e0 net/netfilter/nf_conntrack_extend.c:38 > nf_conntrack_free+0x8f/0xe0 net/netfilter/nf_conntrack_core.c:1418 > destroy_conntrack+0x1a2/0x270 net/netfilter/nf_conntrack_core.c:626 > nf_conntrack_put include/linux/netfilter/nf_conntrack_common.h:31 [inline] > nf_ct_resolve_clash net/netfilter/nf_conntrack_core.c:915 [inline] > ^^^^^^^^^^^^^^^^^^^ > __nf_conntrack_confirm+0x21ca/0x2830 net/netfilter/nf_conntrack_core.c:1038 > nf_conntrack_confirm include/net/netfilter/nf_conntrack_core.h:63 [inline] > nf_confirm+0x3e7/0x4d0 net/netfilter/nf_conntrack_proto.c:154 > > This is whats happening: > > 1. a conntrack entry is about to be confirmed (added to hash table). > 2. a clash with existing entry is detected. > 3. nf_ct_resolve_clash() puts skb->nfct (the "losing" entry). > 4. this entry now has a refcount of 0 and is freed to SLAB_TYPESAFE_BY_RCU > kmem cache. > > skb->nfct has been replaced by the one found in the hash. > Problem is that nf_conntrack_confirm() uses the old ct: > > static inline int nf_conntrack_confirm(struct sk_buff *skb) > { > struct nf_conn *ct = (struct nf_conn *)skb_nfct(skb); > int ret = NF_ACCEPT; > > if (ct) { > if (!nf_ct_is_confirmed(ct)) > ret = __nf_conntrack_confirm(skb); > if (likely(ret == NF_ACCEPT)) > nf_ct_deliver_cached_events(ct); /* This ct has refcount 0! */ > } > return ret; > } > > As of "netfilter: conntrack: free extension area immediately", we can't > access conntrack extensions in this case. > > To fix this, make sure we check the dying bit presence before attempting > to get the eache extension. Applied, thanks. > Reported-by: syzbot+c7aabc9fe93e7f3637ba@syzkaller.appspotmail.com > Fixes: 2ad9d7747c10d1 ("netfilter: conntrack: free extension area immediately") > Signed-off-by: Florian Westphal <fw@strlen.de> > --- > I plan to send a patch for nf tree to alter nf_conntrack_confirm() > to not cache the ct -- I think its a bug too, we should call > nf_ct_deliver_cached_events() on the ct that is assigned to skb *now*, > not the old one. This is the clash resolution that is triggering this path you describe in this note.
Pablo Neira Ayuso <pablo@netfilter.org> wrote: > > I plan to send a patch for nf tree to alter nf_conntrack_confirm() > > to not cache the ct -- I think its a bug too, we should call > > nf_ct_deliver_cached_events() on the ct that is assigned to skb *now*, > > not the old one. > > This is the clash resolution that is triggering this path you describe > in this note. Yes, its the clash resolution.
diff --git a/net/netfilter/nf_conntrack_ecache.c b/net/netfilter/nf_conntrack_ecache.c index 0d83c159671c..7956c9f19899 100644 --- a/net/netfilter/nf_conntrack_ecache.c +++ b/net/netfilter/nf_conntrack_ecache.c @@ -202,15 +202,15 @@ void nf_ct_deliver_cached_events(struct nf_conn *ct) if (notify == NULL) goto out_unlock; + if (!nf_ct_is_confirmed(ct) || nf_ct_is_dying(ct)) + goto out_unlock; + e = nf_ct_ecache_find(ct); if (e == NULL) goto out_unlock; events = xchg(&e->cache, 0); - if (!nf_ct_is_confirmed(ct) || nf_ct_is_dying(ct)) - goto out_unlock; - /* We make a copy of the missed event cache without taking * the lock, thus we may send missed events twice. However, * this does not harm and it happens very rarely. */
syzbot reported following splat: BUG: KASAN: use-after-free in __nf_ct_ext_exist include/net/netfilter/nf_conntrack_extend.h:53 [inline] BUG: KASAN: use-after-free in nf_ct_deliver_cached_events+0x5c3/0x6d0 net/netfilter/nf_conntrack_ecache.c:205 nf_conntrack_confirm include/net/netfilter/nf_conntrack_core.h:65 [inline] nf_confirm+0x3d8/0x4d0 net/netfilter/nf_conntrack_proto.c:154 [..] While there is no reproducer yet, the syzbot report contains one interesting bit of information: Freed by task 27585: [..] kfree+0x10a/0x2c0 mm/slab.c:3757 nf_ct_ext_destroy+0x2ab/0x2e0 net/netfilter/nf_conntrack_extend.c:38 nf_conntrack_free+0x8f/0xe0 net/netfilter/nf_conntrack_core.c:1418 destroy_conntrack+0x1a2/0x270 net/netfilter/nf_conntrack_core.c:626 nf_conntrack_put include/linux/netfilter/nf_conntrack_common.h:31 [inline] nf_ct_resolve_clash net/netfilter/nf_conntrack_core.c:915 [inline] ^^^^^^^^^^^^^^^^^^^ __nf_conntrack_confirm+0x21ca/0x2830 net/netfilter/nf_conntrack_core.c:1038 nf_conntrack_confirm include/net/netfilter/nf_conntrack_core.h:63 [inline] nf_confirm+0x3e7/0x4d0 net/netfilter/nf_conntrack_proto.c:154 This is whats happening: 1. a conntrack entry is about to be confirmed (added to hash table). 2. a clash with existing entry is detected. 3. nf_ct_resolve_clash() puts skb->nfct (the "losing" entry). 4. this entry now has a refcount of 0 and is freed to SLAB_TYPESAFE_BY_RCU kmem cache. skb->nfct has been replaced by the one found in the hash. Problem is that nf_conntrack_confirm() uses the old ct: static inline int nf_conntrack_confirm(struct sk_buff *skb) { struct nf_conn *ct = (struct nf_conn *)skb_nfct(skb); int ret = NF_ACCEPT; if (ct) { if (!nf_ct_is_confirmed(ct)) ret = __nf_conntrack_confirm(skb); if (likely(ret == NF_ACCEPT)) nf_ct_deliver_cached_events(ct); /* This ct has refcount 0! */ } return ret; } As of "netfilter: conntrack: free extension area immediately", we can't access conntrack extensions in this case. To fix this, make sure we check the dying bit presence before attempting to get the eache extension. Reported-by: syzbot+c7aabc9fe93e7f3637ba@syzkaller.appspotmail.com Fixes: 2ad9d7747c10d1 ("netfilter: conntrack: free extension area immediately") Signed-off-by: Florian Westphal <fw@strlen.de> --- I plan to send a patch for nf tree to alter nf_conntrack_confirm() to not cache the ct -- I think its a bug too, we should call nf_ct_deliver_cached_events() on the ct that is assigned to skb *now*, not the old one. net/netfilter/nf_conntrack_ecache.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)