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

[Andy Whitcroft]

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