| Message ID | 20220726115028.3055296-1-william.xuanziyang@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net,v2] ipv6/addrconf: fix a null-ptr-deref bug for ip6_ptr | expand |
On Tue, Jul 26, 2022 at 1:50 PM Ziyang Xuan <william.xuanziyang@huawei.com> 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. > Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> > > --- > 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; > + 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 ? > return !!idev->cnf.ignore_routes_with_linkdown; > } > > -- > 2.25.1 >
On 7/26/22 6:13 AM, Eric Dumazet wrote: > On Tue, Jul 26, 2022 at 1:50 PM Ziyang Xuan > <william.xuanziyang@huawei.com> 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. > >> Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> >> >> --- >> 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@kernel.org> > > 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 ? > for silly MTU games yes, that could happen.
> On 7/26/22 6:13 AM, Eric Dumazet wrote: >> On Tue, Jul 26, 2022 at 1:50 PM Ziyang Xuan >> <william.xuanziyang@huawei.com> 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@huawei.com> >>> >>> --- >>> 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@kernel.org> > >> >> 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. > . >
On Wed, 27 Jul 2022 17:11:14 +0800 Ziyang Xuan (William) wrote: > > Yes, that is the right Fixes commit. > > OK Please repost with that changed.
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; + return !!idev->cnf.ignore_routes_with_linkdown; }
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") Signed-off-by: Ziyang Xuan <william.xuanziyang@huawei.com> --- 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(+)