| Message ID | 1488222425-1256-1-git-send-email-xiyou.wangcong@gmail.com |
|---|---|
| State | RFC, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2017-02-27 at 11:07 -0800, 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> > Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> > --- > net/ipv6/route.c | 7 +++++-- > 1 file changed, 5 insertions(+), 2 deletions(-) > > diff --git a/net/ipv6/route.c b/net/ipv6/route.c > index f54f426..9da77e9 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; > > + if (rt == net->ipv6.ip6_null_entry) > + return -ENOENT; It looks the caller did a dst_hold(&rt->dst); So this new error path would leave a refcount leak. Note that I was not able to trigger the crash on old kernels, so it would be nice to get a precise idea of bug origin. Thanks.
On Mon, Feb 27, 2017 at 12:34 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Mon, 2017-02-27 at 11:07 -0800, 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> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> --- >> net/ipv6/route.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >> index f54f426..9da77e9 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; >> >> + if (rt == net->ipv6.ip6_null_entry) >> + return -ENOENT; > > It looks the caller did a dst_hold(&rt->dst); > > So this new error path would leave a refcount leak. Interesting, this error path is not new for __ip6_del_rt_siblings() so the leak was already there before mine, but you are probably right we have a leak here. I will send a separate patch to address this leak. > > Note that I was not able to trigger the crash on old kernels, so it > would be nice to get a precise idea of bug origin. Right, I miss: Fixes: 0ae8133586ad ("net: ipv6: Allow shorthand delete of all nexthops in multipath route") Thanks!
On 2/27/17 12:34 PM, Eric Dumazet wrote: > On Mon, 2017-02-27 at 11:07 -0800, 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> >> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >> --- >> net/ipv6/route.c | 7 +++++-- >> 1 file changed, 5 insertions(+), 2 deletions(-) >> >> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >> index f54f426..9da77e9 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; >> >> + if (rt == net->ipv6.ip6_null_entry) >> + return -ENOENT; > > It looks the caller did a dst_hold(&rt->dst); > > So this new error path would leave a refcount leak. > > Note that I was not able to trigger the crash on old kernels, so it > would be nice to get a precise idea of bug origin. > Cong: do you want to send a v2 catching the null entry in ip6_route_del before the refcnt? for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { + /* do not allow deletion of the null route */ + if (rt == net->ipv6.ip6_null_entry) + continue; Fixes: 0ae8133586ad net: ipv6: Allow shorthand delete of all nexthops in multipath route
On Mon, Feb 27, 2017 at 1:00 PM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 2/27/17 12:34 PM, Eric Dumazet wrote: >> On Mon, 2017-02-27 at 11:07 -0800, 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> >>> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> >>> --- >>> net/ipv6/route.c | 7 +++++-- >>> 1 file changed, 5 insertions(+), 2 deletions(-) >>> >>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c >>> index f54f426..9da77e9 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; >>> >>> + if (rt == net->ipv6.ip6_null_entry) >>> + return -ENOENT; >> >> It looks the caller did a dst_hold(&rt->dst); >> >> So this new error path would leave a refcount leak. >> >> Note that I was not able to trigger the crash on old kernels, so it >> would be nice to get a precise idea of bug origin. >> > > Cong: do you want to send a v2 catching the null entry in ip6_route_del > before the refcnt? Yeah, actually it is introduced by my patch because there is already an ip6_rt_put() in __ip6_del_rt_siblings(). So v2 is coming... > > for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { > + /* do not allow deletion of the null route */ > + if (rt == net->ipv6.ip6_null_entry) > + continue; > > Fixes: 0ae8133586ad net: ipv6: Allow shorthand delete of all nexthops in > multipath route Note, I moved the check into __ip6_del_rt_siblings() because __ip6_del_rt() has a same check.
On 2/27/17 1:04 PM, Cong Wang wrote: >> >> for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { >> + /* do not allow deletion of the null route */ >> + if (rt == net->ipv6.ip6_null_entry) >> + continue; >> >> Fixes: 0ae8133586ad net: ipv6: Allow shorthand delete of all nexthops in >> multipath route > > Note, I moved the check into __ip6_del_rt_siblings() because __ip6_del_rt() > has a same check. > that's b/c __ip6_del_rt has a second call path. __ip6_del_rt_siblings is new and is not expecting to see the null entry. Catching it before the dst_hold would be better.
On Mon, Feb 27, 2017 at 1:06 PM, David Ahern <dsa@cumulusnetworks.com> wrote: > On 2/27/17 1:04 PM, Cong Wang wrote: >>> >>> for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) { >>> + /* do not allow deletion of the null route */ >>> + if (rt == net->ipv6.ip6_null_entry) >>> + continue; >>> >>> Fixes: 0ae8133586ad net: ipv6: Allow shorthand delete of all nexthops in >>> multipath route >> >> Note, I moved the check into __ip6_del_rt_siblings() because __ip6_del_rt() >> has a same check. >> > > that's b/c __ip6_del_rt has a second call path. __ip6_del_rt_siblings is > new and is not expecting to see the null entry. Catching it before the > dst_hold would be better. Yeah, but it also depends on if we want to continue after the null entry, at least __ip6_del_rt () returns an error for null entry, which looks more correct than continuing to proceed after it.
diff --git a/net/ipv6/route.c b/net/ipv6/route.c index f54f426..9da77e9 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; + if (rt == net->ipv6.ip6_null_entry) + return -ENOENT; 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); @@ -2208,7 +2211,7 @@ static int __ip6_del_rt_siblings(struct rt6_info *rt, struct fib6_config *cfg) 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> Signed-off-by: Cong Wang <xiyou.wangcong@gmail.com> --- net/ipv6/route.c | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-)