* [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
@ 2017-02-28 0:07 Cong Wang
2017-02-28 0:14 ` David Ahern
2017-03-01 23:03 ` David Ahern
0 siblings, 2 replies; 10+ messages in thread
From: Cong Wang @ 2017-02-28 0:07 UTC (permalink / raw)
To: netdev; +Cc: andreyknvl, Cong Wang, David Ahern, Eric Dumazet
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(-)
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;
--
2.5.5
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-02-28 0:07 [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings() Cong Wang
@ 2017-02-28 0:14 ` David Ahern
2017-02-28 0:40 ` Cong Wang
2017-03-01 23:16 ` Martin KaFai Lau
2017-03-01 23:03 ` David Ahern
1 sibling, 2 replies; 10+ messages in thread
From: David Ahern @ 2017-02-28 0:14 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: andreyknvl, Eric Dumazet
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-02-28 0:14 ` David Ahern
@ 2017-02-28 0:40 ` Cong Wang
2017-03-01 23:16 ` Martin KaFai Lau
1 sibling, 0 replies; 10+ messages in thread
From: Cong Wang @ 2017-02-28 0:40 UTC (permalink / raw)
To: David Ahern
Cc: Linux Kernel Network Developers, Andrey Konovalov, Eric Dumazet
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-02-28 0:07 [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings() Cong Wang
2017-02-28 0:14 ` David Ahern
@ 2017-03-01 23:03 ` David Ahern
2017-03-02 20:45 ` David Miller
1 sibling, 1 reply; 10+ messages in thread
From: David Ahern @ 2017-03-01 23:03 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: andreyknvl, Eric Dumazet
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>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-02-28 0:14 ` David Ahern
2017-02-28 0:40 ` Cong Wang
@ 2017-03-01 23:16 ` Martin KaFai Lau
2017-03-01 23:22 ` David Ahern
2017-03-02 0:07 ` David Ahern
1 sibling, 2 replies; 10+ messages in thread
From: Martin KaFai Lau @ 2017-03-01 23:16 UTC (permalink / raw)
To: Cong Wang, David Ahern; +Cc: netdev, andreyknvl, Eric Dumazet
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-03-01 23:16 ` Martin KaFai Lau
@ 2017-03-01 23:22 ` David Ahern
2017-03-02 0:07 ` David Ahern
1 sibling, 0 replies; 10+ messages in thread
From: David Ahern @ 2017-03-01 23:22 UTC (permalink / raw)
To: Martin KaFai Lau, Cong Wang; +Cc: netdev, andreyknvl, Eric Dumazet
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-03-01 23:16 ` Martin KaFai Lau
2017-03-01 23:22 ` David Ahern
@ 2017-03-02 0:07 ` David Ahern
2017-03-02 0:42 ` Martin KaFai Lau
1 sibling, 1 reply; 10+ messages in thread
From: David Ahern @ 2017-03-02 0:07 UTC (permalink / raw)
To: Martin KaFai Lau, Cong Wang; +Cc: netdev, andreyknvl, Eric Dumazet
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'
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-03-02 0:07 ` David Ahern
@ 2017-03-02 0:42 ` Martin KaFai Lau
2017-03-02 5:06 ` David Ahern
0 siblings, 1 reply; 10+ messages in thread
From: Martin KaFai Lau @ 2017-03-02 0:42 UTC (permalink / raw)
To: David Ahern; +Cc: Cong Wang, netdev, andreyknvl, Eric Dumazet
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-03-02 0:42 ` Martin KaFai Lau
@ 2017-03-02 5:06 ` David Ahern
0 siblings, 0 replies; 10+ messages in thread
From: David Ahern @ 2017-03-02 5:06 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: Cong Wang, netdev, andreyknvl, Eric Dumazet
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings()
2017-03-01 23:03 ` David Ahern
@ 2017-03-02 20:45 ` David Miller
0 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2017-03-02 20:45 UTC (permalink / raw)
To: dsa; +Cc: xiyou.wangcong, netdev, andreyknvl, eric.dumazet
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2017-03-02 20:54 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-02-28 0:07 [Patch net v3] ipv6: check for ip6_null_entry in __ip6_del_rt_siblings() Cong Wang
2017-02-28 0:14 ` David Ahern
2017-02-28 0:40 ` Cong Wang
2017-03-01 23:16 ` Martin KaFai Lau
2017-03-01 23:22 ` David Ahern
2017-03-02 0:07 ` David Ahern
2017-03-02 0:42 ` Martin KaFai Lau
2017-03-02 5:06 ` David Ahern
2017-03-01 23:03 ` David Ahern
2017-03-02 20:45 ` David Miller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).