| Message ID | 20231208194523.312416-1-thinker.li@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | Fix dangling pointer at f6i->gc_link. | expand |
On 12/8/23 12:45 PM, thinker.li@gmail.com wrote: > From: Kui-Feng Lee <thinker.li@gmail.com> > > According to a report [1], f6i->gc_link may point to a free block > causing use-after-free. According the stacktraces in the report, it is > very likely that a f6i was added to the GC list after being removed > from the tree of a fib6_table. The reason this can happen is the > current implementation determines if a f6i is on a tree in a wrong > way. It believes a f6i is on a tree if f6i->fib6_table is not NULL. > However, f6i->fib6_table is not reset when f6i is removed from a tree. > > The new solution is to check if f6i->fib6_node is not NULL as well. > f6i->fib6_node is set/or reset when the f6i is added/or removed from > from a tree. It can be used to determines if a f6i is on a tree > reliably. > > The other change is to determine if a f6i is on a GC list. The > current implementation relies on RTF_EXPIRES on fib6_flags. It needs > to consider if a f6i is on a tree as well. The new solution is > checking hlist_unhashed() on f6i->gc_link, a clear evidence, instead. > > [1] https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/ > Eric: can you release the syzbot report for the backtrace listed in [1]. I would like to make "very likely that a f6i was added to the GC list after being removed from the tree of a fib6_table" more certain. Thanks,
On Wed, Dec 13, 2023 at 6:22 PM David Ahern <dsahern@kernel.org> wrote: > > On 12/8/23 12:45 PM, thinker.li@gmail.com wrote: > > From: Kui-Feng Lee <thinker.li@gmail.com> > > > > According to a report [1], f6i->gc_link may point to a free block > > causing use-after-free. According the stacktraces in the report, it is > > very likely that a f6i was added to the GC list after being removed > > from the tree of a fib6_table. The reason this can happen is the > > current implementation determines if a f6i is on a tree in a wrong > > way. It believes a f6i is on a tree if f6i->fib6_table is not NULL. > > However, f6i->fib6_table is not reset when f6i is removed from a tree. > > > > The new solution is to check if f6i->fib6_node is not NULL as well. > > f6i->fib6_node is set/or reset when the f6i is added/or removed from > > from a tree. It can be used to determines if a f6i is on a tree > > reliably. > > > > The other change is to determine if a f6i is on a GC list. The > > current implementation relies on RTF_EXPIRES on fib6_flags. It needs > > to consider if a f6i is on a tree as well. The new solution is > > checking hlist_unhashed() on f6i->gc_link, a clear evidence, instead. > > > > [1] https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/ > > > > Eric: can you release the syzbot report for the backtrace listed in [1]. > I would like to make "very likely that a f6i was added to the GC list > after being removed from the tree of a fib6_table" more certain. Thanks, I have one, not sure if the tree is recent enough.
On 12/13/23 10:32 AM, Eric Dumazet wrote:
> I have one, not sure if the tree is recent enough.
If syzbot is using radvd, can you send that config file?
From: Kui-Feng Lee <thinker.li@gmail.com> According to a report [1], f6i->gc_link may point to a free block causing use-after-free. According the stacktraces in the report, it is very likely that a f6i was added to the GC list after being removed from the tree of a fib6_table. The reason this can happen is the current implementation determines if a f6i is on a tree in a wrong way. It believes a f6i is on a tree if f6i->fib6_table is not NULL. However, f6i->fib6_table is not reset when f6i is removed from a tree. The new solution is to check if f6i->fib6_node is not NULL as well. f6i->fib6_node is set/or reset when the f6i is added/or removed from from a tree. It can be used to determines if a f6i is on a tree reliably. The other change is to determine if a f6i is on a GC list. The current implementation relies on RTF_EXPIRES on fib6_flags. It needs to consider if a f6i is on a tree as well. The new solution is checking hlist_unhashed() on f6i->gc_link, a clear evidence, instead. [1] https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/ --- Major changes from v1: - Split fib6_set_expires_locked() and fib6_clean_expires_locked() - Use hlist_unhashed() on gc_link instead of checking RTF_EXPIRES to determine if a f6i is on a GC list. - Add tests on toggling routes between permanent and temporary. v1: https://lore.kernel.org/all/20231207221627.746324-1-thinker.li@gmail.com/ Kui-Feng Lee (2): net/ipv6: insert a f6i to a GC list only if the f6i is in a fib6_table tree. selftests: fib_tests: Add tests for toggling between w/ and w/o expires. include/net/ip6_fib.h | 46 ++++++++++++---- net/ipv6/route.c | 6 +-- tools/testing/selftests/net/fib_tests.sh | 69 +++++++++++++++++++++++- 3 files changed, 105 insertions(+), 16 deletions(-)