netdev - Re: [PATCH v4] ipv6:introduce function to find route for redirect

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]

Message-ID: <5225758F.8080604@cn.fujitsu.com>
Date:	Tue, 03 Sep 2013 13:37:19 +0800
From:	Duan Jiong <duanj.fnst@...fujitsu.com>
To:	hannes@...essinduktion.org
CC:	davem@...emloft.net, netdev@...r.kernel.org
Subject: Re: [PATCH v4] ipv6:introduce function to find route for redirect

于 2013年09月03日 03:50, Hannes Frederic Sowa 写道:
> On Mon, Sep 02, 2013 at 02:14:57PM +0800, Duan Jiong wrote:
>> +static struct rt6_info *__ip6_route_redirect(struct net *net,
>> +					     struct fib6_table *table,
>> +					     struct flowi6 *fl6,
>> +					     int flags)
>> +{
>> +	struct ip6rd_flowi *rdfl = (struct ip6rd_flowi *)fl6;
>> +	struct rt6_info *rt;
>> +	struct fib6_node *fn;
>> +
>> +	/* Get the "current" route for this destination and
>> +	 * check if the redirect has come from approriate router.
>> +	 *
>> +	 * RFC 4861 specifies that redirects should only be
>> +	 * accepted if they come from the nexthop to the target.
>> +	 * Due to the way the routes are chosen, this notion
>> +	 * is a bit fuzzy and one might need to check all possible
>> +	 * routes.
>> +	 */
>> +
>> +	read_lock_bh(&table->tb6_lock);
>> +	fn = fib6_lookup(&table->tb6_root, &fl6->daddr, &fl6->saddr);
>> +restart:
>> +	for (rt = fn->leaf; rt; rt = rt->dst.rt6_next) {
>> +		if (rt6_check_expired(rt))
>> +			continue;
>> +		if (rt->dst.error)
>> +			continue;
> 
> Sorry, I should have been more clear what I meant with failing early:
> 
> I considered a setup like this:
> 
> ip -6 r a default nexthop via fe80::1 dev eth0
> ip -6 r a prohibit 2002:1::/64
> 
> If the kernel receives a redirect for a destination e.g. 2002:1::1 we
> would backtrack above the prohibit rule and return the dst of the default
> route and would insert a new cached route which could circumvent the
> prohibit rule. We have to try to lock down the tree below 2002:1::/64
> in that case. A possible solution for that would be to do something
> like this:
> 
> 	/* We don't accept a redirect in case a more specific route is
> 	 * installed with dst.error and stop backtracking.
> 	 */
> 	if (rt->dst.error)
> 		break;
> 
> Either we have to replace the rt with net->ipv6.ip6_null_entry in that
> case or check dst->error before calling rt6_do_redirect below.
>

Thanks for you comment, i understand what you mean.
 
>> +		if (!(rt->rt6i_flags & RTF_GATEWAY))
>> +			continue;
>> +		if (fl6->flowi6_oif != rt->dst.dev->ifindex)
>> +			continue;
>> +		if (!ipv6_addr_equal(&rdfl->gateway, &rt->rt6i_gateway))
>> +			continue;
>> +		break;
>> +	}
>> +
>> +	if (!rt)
>> +		rt = net->ipv6.ip6_null_entry;
>> +	BACKTRACK(net, &fl6->saddr);
>> +out:
>> +	dst_hold(&rt->dst);
>> +
>> +	read_unlock_bh(&table->tb6_lock);
>> +
>> +	return rt;
>> +};
>>
>> [...]
>>
>> @@ -1171,9 +1238,8 @@ void ip6_redirect(struct sk_buff *skb, struct net *net, int oif, u32 mark)
>>  	fl6.saddr = iph->saddr;
>>  	fl6.flowlabel = ip6_flowinfo(iph);
>>  
>> -	dst = ip6_route_output(net, NULL, &fl6);
>> -	if (!dst->error)
>> -		rt6_do_redirect(dst, NULL, skb);
>> +	dst = ip6_route_redirect(net, &fl6, &ipv6_hdr(skb)->saddr);
>> +	rt6_do_redirect(dst, NULL, skb);
>>  	dst_release(dst);
>>  }
>>  EXPORT_SYMBOL_GPL(ip6_redirect);
>> @@ -1193,9 +1259,8 @@ void ip6_redirect_no_header(struct sk_buff *skb, struct net *net, int oif,
>>  	fl6.daddr = msg->dest;
>>  	fl6.saddr = iph->daddr;
>>  
>> -	dst = ip6_route_output(net, NULL, &fl6);
>> -	if (!dst->error)
>> -		rt6_do_redirect(dst, NULL, skb);
>> +	dst = ip6_route_redirect(net, &fl6, &iph->saddr);
>> +	rt6_do_redirect(dst, NULL, skb);
>>  	dst_release(dst);
>>  }
> 
> Btw. I still think it should be possible to eliminate
> ip6_redirect_no_header:
> 
> We could always use ip6_redirect_no_header and use the data of the redirected
> header option just for finding the socket to be notified. We can do the whole
> verification and route updating in ndisc layer and then just call into icmpv6
> layer if upper protocols need a notification of the redirect. But that should
> go into another patch. ;)
> 

I think this is good, but i have a question below:

  if the socket type is connection-based, the dst information is stored in related
sock struct, so there is no need to look up the route for redirect in ip6_redirect
or ip6_redirect_no_header, in this case, we do the verification and route 
updating in the upper protocols' err_handler is better. 

How do you think of this?

Thanks,
  Duan
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html