Date: Sun, 17 Dec 2023 17:05:28 -0800
From: Kui-Feng Lee <sinquersw@...il.com>
To: David Ahern <dsahern@...nel.org>, thinker.li@...il.com,
 netdev@...r.kernel.org, martin.lau@...ux.dev, kernel-team@...a.com,
 davem@...emloft.net, kuba@...nel.org, pabeni@...hat.com, edumazet@...gle.com
Cc: kuifeng@...a.com, syzbot+c15aa445274af8674f41@...kaller.appspotmail.com
Subject: Re: [PATCH net-next v3 1/2] net/ipv6: insert a f6i to a GC list only
 if the f6i is in a fib6_table tree.



On 12/16/23 10:36, David Ahern wrote:
> On 12/15/23 11:12 AM, Kui-Feng Lee wrote:
>>
>> I tried to reproduce the issue yesterday, according to the hypothesis
>> behind the patch of this thread. The following is the instructions
>> to reproduce the UAF issue. However, this instruction doesn't create
>> a crash at the place since the memory is still available even it has
>> been free. But, the log shows a UAF.
>>
>> The patch at the end of this message is required to reproduce and
>> show UAF. The most critical change in the patch is to insert
>> a 'mdelay(3000)' to sleep 3s in rt6_route_rcv(). That gives us
>> a chance to manipulate the kernel to reproduce the UAF.
>>
>> Here is my conclusion. There is no protection between finding
>> a route and changing the route in rt6_route_rcv(), including inserting
>> the entry to the gc list. It is possible to insert an entry that will be
>> free later to the gc list, causing a UAF. There is more explanations
>> along with the following logs.
> 
> TL;DR: I think 3dec89b14d37 should be reverted for 6.6 and 6.7
> (selftests should be valid with or without this change) and you try
> again for 6.9 (6.7 dev cycle is about to close).
> 
> ###
> 
> I was successful in triggering UAF twice yesterday, but a slightly
> different code path that in Eric's trace:
> 
> This is the WARN_ON for !hlist_unhashed in fib6_info_release:
> 
> [   46.926339] ------------[ cut here ]------------
> [   46.926368] WARNING: CPU: 3 PID: 0 at include/net/ip6_fib.h:332
> fib6_info_release+0x2a/0x43
> [   46.926384] Modules linked in:
> [   46.926393] CPU: 3 PID: 0 Comm: swapper/3 Not tainted
> 6.7.0-rc4-debug+ #16
> [   46.926399] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> 1.15.0-1 04/01/2014
> [   46.926404] RIP: 0010:fib6_info_release+0x2a/0x43
> [   46.926409] Code: 48 85 ff 74 3d 55 48 89 e5 53 48 89 fb 48 8d 7f 2c
> e8 3e ff ff ff 84 c0 74 25 48 8d 7b 40 e8 33 11 8b ff 48 83 7b 40 00 74
> 02 <0f> 0b 48 8d bb a0 00 00 00 48 c7 c6 93 ea ae 81 e8 3f 00 66 ff 5b
> [   46.926416] RSP: 0018:ffffc900002a8390 EFLAGS: 00010282
> [   46.926422] RAX: 1ffff11002048b00 RBX: ffff888010245c00 RCX:
> ffffffff81af8c9e
> [   46.926426] RDX: dffffc0000000000 RSI: 0000000000000004 RDI:
> ffff888010245c40
> [   46.926431] RBP: ffffc900002a8398 R08: ffffed1002048b86 R09:
> 0000000000000001
> [   46.926435] R10: ffffffff81af8be4 R11: ffff888010245c2f R12:
> ffff88800aeec000
> [   46.926439] R13: 0000000000000000 R14: ffff88801ab7c200 R15:
> 0000000000000020
> [   46.926444] FS:  0000000000000000(0000) GS:ffff88806c000000(0000)
> knlGS:0000000000000000
> [   46.926448] CS:  0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> [   46.926452] CR2: 00007f897cc95fb0 CR3: 000000001d69d000 CR4:
> 0000000000750ef0
> [   46.926458] PKRU: 55555554
> [   46.926462] Call Trace:
> [   46.926466]  <IRQ>
> [   46.926470]  ? show_regs+0x5c/0x60
> [   46.926478]  ? fib6_info_release+0x2a/0x43
> [   46.926483]  ? __warn+0xcb/0x19c
> [   46.926489]  ? fib6_info_release+0x2a/0x43
> [   46.926495]  ? report_bug+0x114/0x186
> [   46.926504]  ? handle_bug+0x40/0x6b
> [   46.926510]  ? exc_invalid_op+0x19/0x41
> [   46.926515]  ? asm_exc_invalid_op+0x1b/0x20
> [   46.926524]  ? refcount_dec_and_test+0x15/0x43
> [   46.926530]  ? fib6_info_release+0x23/0x43
> [   46.926536]  ? fib6_info_release+0x2a/0x43
> [   46.926542]  ndisc_router_discovery+0xf41/0xfa6
> 
> 
> UAF here:
> 
> [   47.941928]
> ==================================================================
> [   47.942548] BUG: KASAN: slab-use-after-free in
> fib6_gc_all.constprop.0+0x19b/0x294
> [   47.943178] Read of size 8 at addr ffff888010245c38 by task swapper/3/0
> 
> [   47.943866] CPU: 3 PID: 0 Comm: swapper/3 Tainted: G        W
>   6.7.0-rc4-debug+ #16
> [   47.944548] Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS
> 1.15.0-1 04/01/2014
> [   47.945204] Call Trace:
> [   47.945416]  <IRQ>
> [   47.945595]  dump_stack_lvl+0x5b/0x82
> [   47.945912]  print_address_description.constprop.0+0x7a/0x2eb
> [   47.946391]  print_report+0x106/0x1e0
> [   47.946703]  ? virt_to_slab+0x9/0x1a
> [   47.947007]  ? kmem_cache_debug_flags+0x13/0x1f
> [   47.947392]  ? kasan_complete_mode_report_info+0x19e/0x1a1
> [   47.947850]  ? fib6_gc_all.constprop.0+0x19b/0x294
> [   47.948254]  kasan_report+0x99/0xc4
> [   47.948554]  ? fib6_gc_all.constprop.0+0x19b/0x294
> [   47.948962]  __asan_load8+0x77/0x79
> [   47.949262]  fib6_gc_all.constprop.0+0x19b/0x294
> 
> 
> for a route added here:
> 
> [   47.970092] Allocated by task 0:
> [   47.970366]  stack_trace_save+0x8d/0xbb
> [   47.970373]  kasan_save_stack+0x26/0x46
> [   47.970379]  kasan_set_track+0x25/0x2b
> [   47.970385]  kasan_save_alloc_info+0x1e/0x21
> [   47.970392]  ____kasan_kmalloc+0x6f/0x7b
> [   47.970398]  __kasan_kmalloc+0x9/0xb
> [   47.970404]  __kmalloc+0x91/0xbf
> [   47.970411]  kzalloc+0xf/0x11
> [   47.970416]  fib6_info_alloc+0x26/0xa1
> [   47.970422]  ip6_route_info_create+0x266/0x6c5
> [   47.970428]  ip6_route_add+0x14/0x46
> [   47.970433]  rt6_add_dflt_router+0x123/0x1bd
> [   47.970439]  ndisc_router_discovery+0x5a4/0xfa6
> [   47.970447]  ndisc_rcv+0x1a2/0x1b5
> 
> This is just aggressive RA's and aggressive gc with a hack to the stack
> to toggle lifetime or metric on every RA (something that can be scripted
> with an ra command - set and reset lifetime in every other RA and change
> the metric in every RA).
> 
> I was targeting this code path because I noticed rt6_add_dflt_router
> sets RTF_EXPIRES but does not pass in the expires value. There are races
> (like the one you found with a different code path) in the handling of
> RTF_EXPIRES, setting the timer, running gc and adding the route entry to
> the gc list.

I am not sure what you mean a race in rt6_add_dflt_router().
In this function, it calls ip6_route_add() to add a default route
with RTF_EXPIRES. In ip6_route_add(), it calls ip6_route_info_create()
to create a f6i, and calls __ip6_ins_rt() to add the entry to gc list.

Although there is a gap between ip6_route_info_create() and
__ip6_ins_rt() without any protection, it should not cause a race.
The newly created entry is not available to the rest of the system
until __ip6_ins_rt() adds it to the tree.  And, adding the entry
to the gc list and adding the entry to the tree are performed
atomically, being protected by table->tb6_lock. So, it should not have
a race between adding to tree and adding to gc list if this is what
you mean.

By the way, do you happen to have a chance to try the patch here in
you tests?  It fixed the issue for my test scenario. According to
your stacktraces, it should be the same issue happening in my test.
Somewhere adds a entry to gc list by faultily believing that
the entry is still on a tree. And, the patch here fixes this
faultily believe.

> 
> In short there are a number of places in the RA path that need the
> lifetime handling cleaned up first to make it all consistent with the
> idea of using a linked list to track entries with an expires tag.

Powered by blists - more mailing lists