netdev - Re: [PATCH v2 net-next] neighbor: Improve garbage collection

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: <5c0e2a107caee_16112ae4fe0d10d42b@olga.notmuch>
Date:   Mon, 10 Dec 2018 09:55:44 +0100
From:   Wolfgang Bumiller <w.bumiller@...xmox.com>
To:     Eric Dumazet <eric.dumazet@...il.com>,
        David Miller <davem@...emloft.net>, dsahern@...nel.org
Cc:     netdev@...r.kernel.org, roopa@...ulusnetworks.com,
        dsahern@...il.com
Subject: Re: [PATCH v2 net-next] neighbor: Improve garbage collection

Eric Dumazet wrote:
> 
> 
> On 12/07/2018 04:03 PM, David Miller wrote:
> > From: David Ahern <dsahern@...nel.org>
> > Date: Fri,  7 Dec 2018 12:24:57 -0800
> > 
> >> From: David Ahern <dsahern@...il.com>
> >>
> >> The existing garbage collection algorithm has a number of problems:
> >  ...
> >> This patch addresses these problems as follows:
> >>
> >> 1. Use of a separate list_head to track entries that can be garbage
> >>    collected along with a separate counter. PERMANENT entries are not
> >>    added to this list.
> >>
> >>    The gc_thresh parameters are only compared to the new counter, not the
> >>    total entries in the table. The forced_gc function is updated to only
> >>    walk this new gc_list looking for entries to evict.
> >>
> >> 2. Entries are added to the list head at the tail and removed from the
> >>    front.
> >>
> >> 3. Entries are only evicted if they were last updated more than 5 seconds
> >>    ago, adhering to the original intent of gc_thresh2.
> >>
> >> 4. Forced gc is stopped once the number of gc_entries drops below
> >>    gc_thresh2.
> >>
> >> 5. Since gc checks do not apply to PERMANENT entries, gc levels are skipped
> >>    when allocating a new neighbor for a PERMANENT entry. By extension this
> >>    means there are no explicit limits on the number of PERMANENT entries
> >>    that can be created, but this is no different than FIB entries or FDB
> >>    entries.
> >>
> >> Signed-off-by: David Ahern <dsahern@...il.com>
> >> ---
> >> v2
> >> - remove on_gc_list boolean in favor of !list_empty
> >> - fix neigh_alloc to add new entry to tail of list_head
> > 
> > Again, looks great, applied.
> > 
> 
> What protects gc_list linkage ?
> 
> We can not use list_del_init(&n->gc_list);                    or 
>                list_add_tail(&n->gc_list, &n->tbl->gc_list);
> 
> if tbl->lock is not held.
> 
> It seems to me this patch needs more care.
> 

This seems to be problematic in neigh_create() which uses neigh_alloc()
before grabbing the lock (which uses list_add_tail()), as seen in the
syzbot traces:
-> KASAN: use-after-free Read in ___neigh_create
   syzbot+e4d42eb35f6a27b0a628@...kaller.appspotmail.com
-> KASAN: slab-out-of-bounds Read in ___neigh_create
   syzbot+3ddead5619658537909b@...kaller.appspotmail.com
-> BUG: corrupted list in ___neigh_create
   syzbot+b354d1fb59091ea73c37@...kaller.appspotmail.com