Message-ID: <0cde3fda-1b65-447c-a859-f583929da504@gmail.com>
Date: Sun, 17 Dec 2023 17:16:39 -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/15/23 11:12, Kui-Feng Lee wrote:
> 
> 
> On 12/13/23 22:11, David Ahern wrote:
>> On 12/13/23 2:37 PM, thinker.li@...il.com wrote:
>>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>>> index b132feae3393..dcaeb88d73aa 100644
>>> --- a/net/ipv6/route.c
>>> +++ b/net/ipv6/route.c
>>> @@ -3763,10 +3763,10 @@ static struct fib6_info 
>>> *ip6_route_info_create(struct fib6_config *cfg,
>>>           rt->dst_nocount = true;
>>>       if (cfg->fc_flags & RTF_EXPIRES)
>>> -        fib6_set_expires_locked(rt, jiffies +
>>> -                    clock_t_to_jiffies(cfg->fc_expires));
>>> +        __fib6_set_expires(rt, jiffies +
>>> +                   clock_t_to_jiffies(cfg->fc_expires));
>>>       else
>>> -        fib6_clean_expires_locked(rt);
>>> +        __fib6_clean_expires(rt);
>>
>> as Eric noted in a past comment, the clean is not needed in this
>> function since memory is initialized to 0 (expires is never set).
>>
>> Also, this patch set does not fundamentally change the logic, so it
>> cannot fix the bug reported in
>>
>> https://lore.kernel.org/all/20231205173250.2982846-1-edumazet@google.com/
>>
>> please hold off future versions of this set until the problem in that
>> stack traced is fixed. I have tried a few things using RA's, but have
>> not been able to recreate UAF.
> 
> 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.
> 
> Instructions:
>       - Preparation
>         - install ipv6toolkit on the host.
>         - run qemu with a patched kernel as a guest through
>           with the host through qemubr0 a bridge.
>         - On the guest
>           - stop systemd-networkd.service & systemd-networkd.socket if 
> there are.
>           - sysctl -w net.ipv6.conf.enp0s3.accept_ra=2
>           - sysctl -w net.ipv6.conf.enp0s3.accept_ra_rt_info_max_plen=127
>         - Assume the address of qemubr0 in the host is
>           fe80::4ce9:92ff:fe27:75df.
>       - Test
>         - ra6 -i qemubr0 -d ff02::1 -R 'fe82::/64#1#300' -t 300 # On the 
> host
>         - sleep 2; ip -6 route del fe82::/64                    # On the 
> guest
>         - ra6 -i qemubr0 -d ff02::1 -R 'fe82::/64#1#300' -t 300 # On the 
> host
>         - ra6 -i qemubr0 -d ff02::1 -R 'fe81::/64#1#300' -t 0   # On the 
> host
>       - The step 3 should start immediately after step 2 since we have
>         a gap of merely 3 seconds in the kernel.
> 

I did the same test with the fix patch along with this thread.
The following is the log I got.

# This is the log printed by the step 1.
# ffff888108066600 was add to the gc list.
[    4.596875] __ip6_ins_rt fe80::5054:ff:fe12:3456/128
[   38.925482] rt = 0000000000000000
[   38.925916] fib6_info_alloc: ffff888108066e00
[   38.926441] __ip6_ins_rt ::/0
[   38.926441] fib6_add: add ffff888108066e00 to gc list 
ffff888108066e38 pprev ffff888102a02800 next 0000000000000000
[   38.927084] rt = ffff888108066e00
[   38.927084] rt6_route_rcv
[   38.927084] fib6_info_alloc: ffff888108066600
[   38.929333] __ip6_ins_rt fe82::/64
[   38.929333] rt6_route_rcv: route info ffff888108066600, sleep 3s
[   40.948831] fib6_set_expires_locked: add ffff888108066600 to gc list 
ffff888108066638 pprev ffff888102a02800 next ffff888108066e38
[   41.932501] rt6_route_rcv: route info ffff888108066600, after release

# This is the log printed by the step 2 & 3.
# The entry (ffff888108066600) removed from the gc list by
# fib6_clean_expires_locked was not added back to the gc list again.
[   68.173441] rt = ffff888108066e00
[   68.173883] rt6_route_rcv
[   68.174245] rt6_route_rcv: route info ffff888108066600, sleep 3s
[   69.389112] fib6_clean_expires_locked: del ffff888108066600 from gc 
list pprev ffff888102a02800 next ffff888108066e38
[   70.900831] rt6_route_rcv: route info ffff888108066600, after release
[   71.182822] fib6_info_destroy_rcu ffff888108066600

# This is the log printed by the step 4.
[  109.017446] rt = ffff888108066e00
[  109.017893] __ip6_del_rt ::/0
[  109.018288] fib6_clean_expires_locked: del ffff888108066e00 from gc 
list pprev ffff888102a02800 next 0000000000000000
[  109.019471] rt6_route_rcv
[  109.019471] fib6_info_alloc: ffff888108066600
[  109.019471] __ip6_ins_rt fe81::/64
[  109.019471] rt6_route_rcv: route info ffff888108066600, sleep 3s
[  111.028831] fib6_set_expires_locked: add ffff888108066600 to gc list 
ffff888108066638 pprev ffff888102a02800 next 0000000000000000
[  112.023607] rt6_route_rcv: route info ffff888108066600, after release
[  112.031825] fib6_info_destroy_rcu ffff888108066e00

Powered by blists - more mailing lists