netdev - Re: [PATCHv3 net] ipv6: no need to return rt->dst.error if it is prohibit entry

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 27 Jul 2017 21:48:23 +0800
From:   Hangbin Liu <liuhangbin@...il.com>
To:     David Ahern <dsahern@...il.com>
Cc:     netdev@...r.kernel.org, Cong Wang <xiyou.wangcong@...il.com>,
        Roopa Prabhu <roopa@...ulusnetworks.com>
Subject: Re: [PATCHv3 net] ipv6: no need to return rt->dst.error if it is
 prohibit entry

On Wed, Jul 26, 2017 at 11:09:39AM -0600, David Ahern wrote:
> On 7/26/17 3:20 AM, Hangbin Liu wrote:
> > After commit 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib
> > result when requested"). When we get a prohibit ertry, we will return
> > -EACCES directly.
> > 
> > Before:
> 
> Do you mean "Before commit 18c3a61c4264?"
> 
> > + ip netns exec client ip -6 route get 2003::1
> > prohibit 2003::1 dev lo table unspec proto kernel src 2001::1 metric
> > 4294967295 error -13
> > 
> > After:
> 
> And "After commit 18c3a61c4264?"

Yes. Sorry I didn't make it clear.

> 
> > + ip netns exec server ip -6 route get 2002::1
> > RTNETLINK answers: Permission denied
> > 
> > Fix this by add prohibit and blk hole check.
> > 
> > At the same time, after commit
> > 2f460933f58e ("ipv6: initialize route null entry in addrconf_init()") and
> > 242d3a49a2a1 ("ipv6: reorder ip6_route_dev_notifier after ipv6_dev_notf")
> > We will init rt6i_idev correctly. So we could dump ip6_null_entry
> > (unreachable route entry) safely now.
> > 
> > Fixes: 18c3a61c4264 ("net: ipv6: RTM_GETROUTE: return matched fib...")
> > Signed-off-by: Hangbin Liu <liuhangbin@...il.com>
> 
> This is what I see with your patch:
> 
> # ip -6 ro ls vrf red
> 2001:db8:1::/120 dev eth1 proto kernel metric 256  pref medium
> prohibit 5000::/120 dev lo metric 1024  error -13 pref medium
> fe80::/64 dev eth1 proto kernel metric 256  pref medium
> ff00::/8 dev eth1 metric 256  pref medium
> unreachable default dev lo metric 8192  error -113 pref medium
> 
> ie., I added a prohibit route for 5000:/120
> 
> and then running:
> # ip -6 ro get vrf red 5000::1
> RTNETLINK answers: Permission denied
> 
> Which is the behavior without your patch.
> 
> Now if I delete just the first bit:
> 
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index 4d30c96a819d..8fc52de40175 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -3637,12 +3637,6 @@ static int inet6_rtm_getroute(struct sk_buff
> *in_skb, struct nlmsghdr *nlh,
>                 dst = ip6_route_lookup(net, &fl6, 0);
> 
>         rt = container_of(dst, struct rt6_info, dst);
> -       if (rt->dst.error) {
> -               err = rt->dst.error;
> -               ip6_rt_put(rt);
> -               goto errout;
> -       }
> -
>         if (rt == net->ipv6.ip6_null_entry) {
>                 err = rt->dst.error;
>                 ip6_rt_put(rt);
> 
> Then I get:
> 
> # ip -6 ro get vrf red 5000::1
> prohibit 5000::1 from :: dev lo table red src 2001:db8::2 metric 1024
> error -13 pref medium
> 
> which seems to be your objective.

Yes
>
>> I don't understand why you are focused on the built-in null and prohibit
>> route entries.
>
>I see. You are using fib rules for the prohibit entry; I am using an
>explicit route entry.

Yes, This is my fault. I should put my steps in the commit.

# ip -6 rule add to 2003::1/64 table 100 prohibit
# ip -6 route get 2003::1
RTNETLINK answers: Permission denied

>
>If I run 'ip ro get fibmatch' for the latter I want to see that route
>entry since it is a route in the FIB:
>
># ip -6 ro get fibmatch vrf red 5000::1
>prohibit 5000::/120 dev lo table red metric 1024 error -13 pref medium
>
>So there are multiple cases to verify.

Yeah... there are more stuff I need to learn.

Thanks
Hangbin