Re: [PATCH net v2] ipv6/addrconf: fix a null-ptr-deref bug for ip6_ptr

[Ziyang Xuan (William)]

> On 7/26/22 6:13 AM, Eric Dumazet wrote:
>> On Tue, Jul 26, 2022 at 1:50 PM Ziyang Xuan
>> <william.xuanziyang@xxxxxxxxxx> wrote:
>>>
>>> Change net device's MTU to smaller than IPV6_MIN_MTU or unregister
>>> device while matching route. That may trigger null-ptr-deref bug
>>> for ip6_ptr probability as following.
>>>
>>> =========================================================
>>> BUG: KASAN: null-ptr-deref in find_match.part.0+0x70/0x134
>>> Read of size 4 at addr 0000000000000308 by task ping6/263
>>>
>>> CPU: 2 PID: 263 Comm: ping6 Not tainted 5.19.0-rc7+ #14
>>> Call trace:
>>>  dump_backtrace+0x1a8/0x230
>>>  show_stack+0x20/0x70
>>>  dump_stack_lvl+0x68/0x84
>>>  print_report+0xc4/0x120
>>>  kasan_report+0x84/0x120
>>>  __asan_load4+0x94/0xd0
>>>  find_match.part.0+0x70/0x134
>>>  __find_rr_leaf+0x408/0x470
>>>  fib6_table_lookup+0x264/0x540
>>>  ip6_pol_route+0xf4/0x260
>>>  ip6_pol_route_output+0x58/0x70
>>>  fib6_rule_lookup+0x1a8/0x330
>>>  ip6_route_output_flags_noref+0xd8/0x1a0
>>>  ip6_route_output_flags+0x58/0x160
>>>  ip6_dst_lookup_tail+0x5b4/0x85c
>>>  ip6_dst_lookup_flow+0x98/0x120
>>>  rawv6_sendmsg+0x49c/0xc70
>>>  inet_sendmsg+0x68/0x94
>>>
>>> Reproducer as following:
>>> Firstly, prepare conditions:
>>> $ip netns add ns1
>>> $ip netns add ns2
>>> $ip link add veth1 type veth peer name veth2
>>> $ip link set veth1 netns ns1
>>> $ip link set veth2 netns ns2
>>> $ip netns exec ns1 ip -6 addr add 2001:0db8:0:f101::1/64 dev veth1
>>> $ip netns exec ns2 ip -6 addr add 2001:0db8:0:f101::2/64 dev veth2
>>> $ip netns exec ns1 ifconfig veth1 up
>>> $ip netns exec ns2 ifconfig veth2 up
>>> $ip netns exec ns1 ip -6 route add 2000::/64 dev veth1 metric 1
>>> $ip netns exec ns2 ip -6 route add 2001::/64 dev veth2 metric 1
>>>
>>> Secondly, execute the following two commands in two ssh windows
>>> respectively:
>>> $ip netns exec ns1 sh
>>> $while true; do ip -6 addr add 2001:0db8:0:f101::1/64 dev veth1; ip -6 route add 2000::/64 dev veth1 metric 1; ping6 2000::2; done
>>>
>>> $ip netns exec ns1 sh
>>> $while true; do ip link set veth1 mtu 1000; ip link set veth1 mtu 1500; sleep 5; done
>>>
>>> It is because ip6_ptr has been assigned to NULL in addrconf_ifdown() firstly,
>>> then ip6_ignore_linkdown() accesses ip6_ptr directly without NULL check.
>>>
>>>         cpu0                    cpu1
>>> fib6_table_lookup
>>> __find_rr_leaf
>>>                         addrconf_notify [ NETDEV_CHANGEMTU ]
>>>                         addrconf_ifdown
>>>                         RCU_INIT_POINTER(dev->ip6_ptr, NULL)
>>> find_match
>>> ip6_ignore_linkdown
>>>
>>> So we can add NULL check for ip6_ptr before using in ip6_ignore_linkdown() to
>>> fix the null-ptr-deref bug.
>>>
>>> Fixes: 6d3d07b45c86 ("ipv6: Refactor fib6_ignore_linkdown")
>>
>> If we need to backport, I guess dcd1f572954f ("net/ipv6: Remove fib6_idev")
>> already had the bug.
> 
> Yes, that is the right Fixes commit.

OK

> 
>>
>>> Signed-off-by: Ziyang Xuan <william.xuanziyang@xxxxxxxxxx>
>>>
>>> ---
>>> v2:
>>>   - Use NULL check in ip6_ignore_linkdown() but synchronize_net() in
>>>     addrconf_ifdown()
>>>   - Add timing analysis of the problem
>>>
>>> ---
>>>  include/net/addrconf.h | 3 +++
>>>  1 file changed, 3 insertions(+)
>>>
>>> diff --git a/include/net/addrconf.h b/include/net/addrconf.h
>>> index f7506f08e505..c04f359655b8 100644
>>> --- a/include/net/addrconf.h
>>> +++ b/include/net/addrconf.h
>>> @@ -405,6 +405,9 @@ static inline bool ip6_ignore_linkdown(const struct net_device *dev)
>>>  {
>>>         const struct inet6_dev *idev = __in6_dev_get(dev);
>>>
>>> +       if (unlikely(!idev))
>>> +               return true;
>>> +
> 
> Reviewed-by: David Ahern <dsahern@xxxxxxxxxx>
> 
>>
>> Note that we might read a non NULL pointer here, but read it again
>> later in rt6_score_route(),
>> since another thread could switch the pointer under us ?
>>

Yes, this patch just cover the problem I'm having.
I have checked the codes in kernel, there are some scenarios using __in6_dev_get()
without NULL check and rtnl_lock. There is a possibility of null-ptr-deref bug.
I will give a patch to fix them later.

> 
> for silly MTU games yes, that could happen.
> .
>