[natty, natty/ti-omap4 CVE 1/1] ipv6: make fragment identifications less predictable

[Stefan Bader]

On 24.08.2011 12:30, Andy Whitcroft wrote:
> On Wed, Aug 24, 2011 at 11:53:30AM +0200, Stefan Bader wrote:
> 
>>> -static __inline__ void ipv6_select_ident(struct frag_hdr *fhdr)
>>> -{
>>> -	static u32 ipv6_fragmentation_id = 1;
>>> -	static DEFINE_SPINLOCK(ip6_id_lock);
>>> -
>>> -	spin_lock_bh(&ip6_id_lock);
>>> -	fhdr->identification = htonl(ipv6_fragmentation_id);
>>> -	if (++ipv6_fragmentation_id == 0)
>>> -		ipv6_fragmentation_id = 1;
>>> -	spin_unlock_bh(&ip6_id_lock);
>>> -}
> 
> This _old_ code ensures that fragmentation_id is never == 0, and that
> value is used directly without modification.  An id of 0 is invalid.
> 
>>> +static u32 hashidentrnd __read_mostly;
>>> +#define FID_HASH_SZ 16
>>> +static u32 ipv6_fragmentation_id[FID_HASH_SZ];
>>> +
>>> +void __init initialize_hashidentrnd(void)
>>> +{
>>> +	get_random_bytes(&hashidentrnd, sizeof(hashidentrnd));
>>> +}
>>> +
>>> +static u32 __ipv6_select_ident(const struct in6_addr *addr)
>>> +{
>>> +	u32 newid, oldid, hash = jhash2((u32 *)addr, 4, hashidentrnd);
>>> +	u32 *pid = &ipv6_fragmentation_id[hash % FID_HASH_SZ];
>>> +
>>> +	do {
>>> +		oldid = *pid;
>>> +		newid = oldid + 1;
>>> +		if (!(hash + newid))
>>> +			newid++;
>>
>> I am trying to understand what hash + newid is supposed to tell. Looking at the
>> old code there was a test to prevent the id from being 0 when it wrapped. If I
>> understand this patch correctly there is now multiple ids selected based on the
>> addresses hash... So why is the above not just
>>
>> if (!newid)
>>   newid = 1;
>>
>> ?
>>
>>> +	} while (cmpxchg(pid, oldid, newid) != oldid);
>>> +
>>> +	return hash + newid;
>>> +}
> 
> The key here is that the value which will be used is this last one at
> the end.  It will return hash + newid.  It is that value which must not
> be 0 as it is that which is used as the id.  Therefore the check is
> hash + newid not being 0 to match the return.
> 
> -apw

I see. Somehow missed that the whole hash+newid is returned for assignment and
only the newid part being stored in a bucket...