| Message ID | 1488240463-12000-1-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | Accepted, archived |
| Delegated to: | David Miller |
| Headers | show |
On 2/27/17 4:07 PM, Cong Wang wrote: > Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() > -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is > because ip6_null_entry is returned in this path since ip6_null_entry > is kinda default for a ipv6 route table root node. Quote from Missed this earlier. The issue here is an attempt to delete the NULL route, not that the null_entry is being returned as happens during a route lookup. This will also hit the bug: ip -6 ro del ::/0
On Mon, Feb 27, 2017 at 4:14 PM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 2/27/17 4:07 PM, Cong Wang wrote: >> Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() >> -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is >> because ip6_null_entry is returned in this path since ip6_null_entry >> is kinda default for a ipv6 route table root node. Quote from > > > Missed this earlier. The issue here is an attempt to delete the NULL > route, not that the null_entry is being returned as happens during a > route lookup. This will also hit the bug: > ip -6 ro del ::/0 By "returned" I mean it is in the fn->leaf list.
On 2/27/17 4:07 PM, Cong Wang wrote: > Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() > -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is > because ip6_null_entry is returned in this path since ip6_null_entry > is kinda default for a ipv6 route table root node. Quote from > David Ahern: > > ip6_null_entry is the root of all ipv6 fib tables making it integrated > into the table ... > > We should ignore any attempt of trying to delete it, like we do in > __ip6_del_rt() path and several others. > > Reported-by: Andrey Konovalov <andreyknvl@google.com> > Fixes: 0ae8133586ad ("net: ipv6: Allow shorthand delete of all nexthops in multipath route") > Cc: David Ahern <dsa@cumulusnetworks.com> > Cc: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/ipv6/route.c | 14 +++++++++----- > 1 file changed, 9 insertions(+), 5 deletions(-) Acked-by: David Ahern <dsa@cumulusnetworks.com>
On Mon, Feb 27, 2017 at 04:14:04PM -0800, David Ahern wrote: > On 2/27/17 4:07 PM, Cong Wang wrote: > > Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() > > -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is > > because ip6_null_entry is returned in this path since ip6_null_entry > > is kinda default for a ipv6 route table root node. Quote from > > > Missed this earlier. The issue here is an attempt to delete the NULL > route, You meant rt == NULL or rt->rt6i_table == NULL when rt == ip6_null_entry? > not that the null_entry is being returned as happens during a > route lookup. This will also hit the bug: > ip -6 ro del ::/0 I also found the commit log a bit confusing. By reading the message, my first thought was an ip6_null_entry is returned because a route cannot be found. Thanks for this particular test case. It seems fn is NULL here for all random routes except 'ip -6 r del xyz::/0' which happens to match ip6_null_entry. [ An unrelated topic. I wonder ip -6 r del xyz::/0 would delete the gateway route...] The patch LGTM.
On 3/1/17 3:16 PM, Martin KaFai Lau wrote: > On Mon, Feb 27, 2017 at 04:14:04PM -0800, David Ahern wrote: >> On 2/27/17 4:07 PM, Cong Wang wrote: >>> Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() >>> -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is >>> because ip6_null_entry is returned in this path since ip6_null_entry >>> is kinda default for a ipv6 route table root node. Quote from >> >> >> Missed this earlier. The issue here is an attempt to delete the NULL >> route, > You meant rt == NULL or rt->rt6i_table == NULL when rt == ip6_null_entry? ip6_null_entry > >> not that the null_entry is being returned as happens during a >> route lookup. This will also hit the bug: >> ip -6 ro del ::/0 > I also found the commit log a bit confusing. By reading the message, > my first thought was an ip6_null_entry is returned because a route cannot > be found. Thanks for this particular test case. It seems fn is NULL > here for all random routes except 'ip -6 r del xyz::/0' which happens > to match ip6_null_entry. yes, that was my point.
On 3/1/17 3:16 PM, Martin KaFai Lau wrote: > [ An unrelated topic. I wonder ip -6 r del xyz::/0 would delete > the gateway route...] a very related question ... ip -6 r del x::/0 comes down to the kernel as delete '::/0' (plen is 0, so cfg->fc_dst is 0). It ends up on the null_entry from fib6_locate b/c of the 0 prefix length, so it is another variant of 'ip -6 ro del ::/0'
On Wed, Mar 01, 2017 at 04:07:38PM -0800, David Ahern wrote: > On 3/1/17 3:16 PM, Martin KaFai Lau wrote: > > [ An unrelated topic. I wonder ip -6 r del xyz::/0 would delete > > the gateway route...] > > a very related question ... > > ip -6 r del x::/0 comes down to the kernel as delete '::/0' (plen is 0, > so cfg->fc_dst is 0). It ends up on the null_entry from fib6_locate b/c > of the 0 prefix length, so it is another variant of 'ip -6 ro del ::/0' Agree on the plen == 0 part. I actually meant if 'ip -6 r del xyz::/0' would delete any _default_ _gateway_ also. By looking at ip6_route_del() alone, it seems it only checks ipv6_addr_equal(&cfg->fc_gateway, &rt->rt6i_gateway) if (cfg->fc_flags & RTF_GATEWAY) is true. Your test case makes me think about this possibility but it is not related to what this patch is trying to fix. I should have started another thread for this :p
On 3/1/17 4:42 PM, Martin KaFai Lau wrote: > On Wed, Mar 01, 2017 at 04:07:38PM -0800, David Ahern wrote: >> On 3/1/17 3:16 PM, Martin KaFai Lau wrote: >>> [ An unrelated topic. I wonder ip -6 r del xyz::/0 would delete >>> the gateway route...] >> >> a very related question ... >> >> ip -6 r del x::/0 comes down to the kernel as delete '::/0' (plen is 0, >> so cfg->fc_dst is 0). It ends up on the null_entry from fib6_locate b/c >> of the 0 prefix length, so it is another variant of 'ip -6 ro del ::/0' > Agree on the plen == 0 part. > > I actually meant if 'ip -6 r del xyz::/0' would delete any _default_ > _gateway_ also. By looking at ip6_route_del() alone, it seems it only Per rtm_to_fib6_config, plen of 0 means fc_dst is 0. Meaning xyz::/0 == ::/0
From: David Ahern <dsa@cumulusnetworks.com> Date: Wed, 1 Mar 2017 15:03:45 -0800 > On 2/27/17 4:07 PM, Cong Wang wrote: >> Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() >> -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is >> because ip6_null_entry is returned in this path since ip6_null_entry >> is kinda default for a ipv6 route table root node. Quote from >> David Ahern: >> >> ip6_null_entry is the root of all ipv6 fib tables making it integrated >> into the table ... >> >> We should ignore any attempt of trying to delete it, like we do in >> __ip6_del_rt() path and several others. >> >> Reported-by: Andrey Konovalov <andreyknvl@google.com> >> Fixes: 0ae8133586ad ("net: ipv6: Allow shorthand delete of all nexthops in multipath route") >> Cc: David Ahern <dsa@cumulusnetworks.com> >> Cc: Eric Dumazet <eric.dumazet@gmail.com> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> --- >> net/ipv6/route.c | 14 +++++++++----- >> 1 file changed, 9 insertions(+), 5 deletions(-) > > > Acked-by: David Ahern <dsa@cumulusnetworks.com> Applied, thanks.
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index f54f426..77c7ce7 100644 --- a/net/ipv6/route.c +++ b/net/ipv6/route.c @@ -2169,10 +2169,13 @@ int ip6_del_rt(struct rt6_info *rt) static int __ip6_del_rt_siblings(struct rt6_info *rt, struct fib6_config *cfg) { struct nl_info *info = &cfg->fc_nlinfo; + struct net *net = info->nl_net; struct sk_buff *skb = NULL; struct fib6_table *table; - int err; + int err = -ENOENT; + if (rt == net->ipv6.ip6_null_entry) + goto out_put; table = rt->rt6i_table; write_lock_bh(&table->tb6_lock); @@ -2184,7 +2187,7 @@ static int __ip6_del_rt_siblings(struct rt6_info *rt, struct fib6_config *cfg) if (skb) { u32 seq = info->nlh ? info->nlh->nlmsg_seq : 0; - if (rt6_fill_node(info->nl_net, skb, rt, + if (rt6_fill_node(net, skb, rt, NULL, NULL, 0, RTM_DELROUTE, info->portid, seq, 0) < 0) { kfree_skb(skb); @@ -2198,17 +2201,18 @@ static int __ip6_del_rt_siblings(struct rt6_info *rt, struct fib6_config *cfg) rt6i_siblings) { err = fib6_del(sibling, info); if (err) - goto out; + goto out_unlock; } } err = fib6_del(rt, info); -out: +out_unlock: write_unlock_bh(&table->tb6_lock); +out_put: ip6_rt_put(rt); if (skb) { - rtnl_notify(skb, info->nl_net, info->portid, RTNLGRP_IPV6_ROUTE, + rtnl_notify(skb, net, info->portid, RTNLGRP_IPV6_ROUTE, info->nlh, gfp_any()); } return err;
Andrey reported a NULL pointer deref bug in ipv6_route_ioctl() -> ip6_route_del() -> __ip6_del_rt_siblings() code path. This is because ip6_null_entry is returned in this path since ip6_null_entry is kinda default for a ipv6 route table root node. Quote from David Ahern: ip6_null_entry is the root of all ipv6 fib tables making it integrated into the table ... We should ignore any attempt of trying to delete it, like we do in __ip6_del_rt() path and several others. Reported-by: Andrey Konovalov <andreyknvl@google.com> Fixes: 0ae8133586ad ("net: ipv6: Allow shorthand delete of all nexthops in multipath route") Cc: David Ahern <dsa@cumulusnetworks.com> Cc: Eric Dumazet <eric.dumazet@gmail.com> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/ipv6/route.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-)