netdev - Re: [PATCH V3] ipv6: fix incorrect route 'expires' value passed to userspace

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:	Wed, 25 Jul 2012 15:33:33 +0800
From:	Li Wei <lw@...fujitsu.com>
To:	Eric Dumazet <eric.dumazet@...il.com>
CC:	David Miller <davem@...emloft.net>, David.Laight@...LAB.COM,
	netdev@...r.kernel.org, shemminger@...tta.com
Subject: Re: [PATCH V3] ipv6: fix incorrect route 'expires' value passed to
 userspace

On 07/25/2012 02:51 PM, Eric Dumazet wrote:
> On Wed, 2012-07-25 at 13:25 +0800, Li Wei wrote:
>> When userspace use RTM_GETROUTE to dump route table, with an already
>> expired route entry, we always got an 'expires' value(2147157)
>> calculated base on INT_MAX.
>>
>> The reason of this problem is in the following satement:
>> 	rt->dst.expires - jiffies < INT_MAX
>> gcc promoted the type of both sides of '<' to unsigned long, thus
>> a small negative value would be considered greater than INT_MAX.
>>
>> This patch fix this by use the same trick as time_after macro to
>> avoid the 'unsigned long' type promotion and deal with jiffies
>> wrapping.
>>
>> Also we should do some fix in rtnl_put_cacheinfo() which use
>> jiffies_to_clock_t(which take an unsigned long as parameter) to
>> convert jiffies to clock_t to handle the negative expires.
>>
>> With the help of David Laight, we can make the code a little clean.
>>
>> Signed-off-by: Li Wei <lw@...fujitsu.com>
>> ---
>>  net/core/rtnetlink.c |    3 ++-
>>  net/ipv6/route.c     |   11 ++++++-----
>>  2 files changed, 8 insertions(+), 6 deletions(-)
>>
>> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
>> index 334b930..2e96396 100644
>> --- a/net/core/rtnetlink.c
>> +++ b/net/core/rtnetlink.c
>> @@ -626,7 +626,8 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
>>  	};
>>  
>>  	if (expires)
>> -		ci.rta_expires = jiffies_to_clock_t(expires);
>> +		ci.rta_expires = expires > 0 ? jiffies_to_clock_t(expires)
>> +			: -jiffies_to_clock_t(-expires);
>>  
>>  	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
>>  }
>> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
>> index cf02cb9..6efeb28 100644
>> --- a/net/ipv6/route.c
>> +++ b/net/ipv6/route.c
>> @@ -2480,12 +2480,13 @@ static int rt6_fill_node(struct net *net,
>>  		goto nla_put_failure;
>>  	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
>>  		goto nla_put_failure;
>> -	if (!(rt->rt6i_flags & RTF_EXPIRES))
>> +	if (!(rt->rt6i_flags & RTF_EXPIRES)) {
>>  		expires = 0;
>> -	else if (rt->dst.expires - jiffies < INT_MAX)
>> -		expires = rt->dst.expires - jiffies;
>> -	else
>> -		expires = INT_MAX;
>> +	} else {
>> +		expires = (long)rt->dst.expires - (long)jiffies;
>> +		if (expires != (int)expires)
>> +			expires = expires > 0 ? INT_MAX : INT_MIN;
>> +	}
>>  
>>  	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
>>  		goto nla_put_failure;
> 
> All this sounds not very clean.
> 
> rtnl_put_cacheinfo( ... long expires ... )
> 
> Any out of bound checks should be done in rtnl_put_cacheinfo(), _after_
> conversion to clock_t.

Ok, I got it.

I tested the following patch, got the correct expires value and not found
any problem.

Thanks Eric :)

> 
> 
> diff --git a/net/core/rtnetlink.c b/net/core/rtnetlink.c
> index 334b930..c1c950b 100644
> --- a/net/core/rtnetlink.c
> +++ b/net/core/rtnetlink.c
> @@ -625,9 +625,13 @@ int rtnl_put_cacheinfo(struct sk_buff *skb, struct dst_entry *dst, u32 id,
>  		.rta_id =  id,
>  	};
>  
> -	if (expires)
> -		ci.rta_expires = jiffies_to_clock_t(expires);
> +	if (expires) {
> +		unsigned long clock;
>  
> +		clock = jiffies_to_clock_t(abs(expires));
> +		clock = min_t(unsigned long, clock, INT_MAX);
> +		ci.rta_expires = (expires > 0) ? clock : -clock;
> +	}
>  	return nla_put(skb, RTA_CACHEINFO, sizeof(ci), &ci);
>  }
>  EXPORT_SYMBOL_GPL(rtnl_put_cacheinfo);
> diff --git a/net/ipv6/route.c b/net/ipv6/route.c
> index cf02cb9..8e80fd2 100644
> --- a/net/ipv6/route.c
> +++ b/net/ipv6/route.c
> @@ -2480,12 +2480,8 @@ static int rt6_fill_node(struct net *net,
>  		goto nla_put_failure;
>  	if (nla_put_u32(skb, RTA_PRIORITY, rt->rt6i_metric))
>  		goto nla_put_failure;
> -	if (!(rt->rt6i_flags & RTF_EXPIRES))
> -		expires = 0;
> -	else if (rt->dst.expires - jiffies < INT_MAX)
> -		expires = rt->dst.expires - jiffies;
> -	else
> -		expires = INT_MAX;
> +
> +	expires = (rt->rt6i_flags & RTF_EXPIRES) ? rt->dst.expires - jiffies : 0;
>  
>  	if (rtnl_put_cacheinfo(skb, &rt->dst, 0, expires, rt->dst.error) < 0)
>  		goto nla_put_failure;
> 
> 
> 
> 
--
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