| Message ID | 1314111512-24177-6-git-send-email-apw@canonical.com |
|---|---|
| State | New |
| Headers | show |
On 23.08.2011 16:58, Andy Whitcroft wrote: > [ Backport of upstream commit 87c48fa3b4630905f98268dde838ee43626a060c ] > > Fernando Gont reported current IPv6 fragment identification generation > was not secure, because using a very predictable system-wide generator, > allowing various attacks. > > IPv4 uses inetpeer cache to address this problem and to get good > performance. We'll use this mechanism when IPv6 inetpeer is stable > enough in linux-3.1 > > For the time being, we use jhash on destination address to provide less > predictable identifications. Also remove a spinlock and use cmpxchg() to > get better SMP performance. > > Reported-by: Fernando Gont <fernando@gont.com.ar> > Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com> > Signed-off-by: David S. Miller <davem@davemloft.net> > Signed-off-by: Greg Kroah-Hartman <gregkh@suse.de> > > CVE-2011-2699 > BugLink: http://bugs.launchpad.net/bugs/827685 > Signed-off-by: Andy Whitcroft <apw@canonical.com> > --- > include/net/ipv6.h | 12 +----------- > include/net/transp_v6.h | 2 ++ > net/ipv6/af_inet6.c | 2 ++ > net/ipv6/ip6_output.c | 40 +++++++++++++++++++++++++++++++++++----- > net/ipv6/udp.c | 2 +- > 5 files changed, 41 insertions(+), 17 deletions(-) > > diff --git a/include/net/ipv6.h b/include/net/ipv6.h > index 96e50e0..24175cf 100644 > --- a/include/net/ipv6.h > +++ b/include/net/ipv6.h > @@ -460,17 +460,7 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add > return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr)); > } > > -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); > -} > +extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt); > > /* > * Prototypes exported by ipv6 > diff --git a/include/net/transp_v6.h b/include/net/transp_v6.h > index 42a0eb6..70eef79 100644 > --- a/include/net/transp_v6.h > +++ b/include/net/transp_v6.h > @@ -16,6 +16,8 @@ extern struct proto tcpv6_prot; > > struct flowi; > > +extern void initialize_hashidentrnd(void); > + > /* extention headers */ > extern int ipv6_exthdrs_init(void); > extern void ipv6_exthdrs_exit(void); > diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c > index 978e80e..ca171b7 100644 > --- a/net/ipv6/af_inet6.c > +++ b/net/ipv6/af_inet6.c > @@ -1081,6 +1081,8 @@ static int __init inet6_init(void) > goto out; > } > > + initialize_hashidentrnd(); > + > err = proto_register(&tcpv6_prot, 1); > if (err) > goto out; > diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c > index 5f8d242..ed0c1a7 100644 > --- a/net/ipv6/ip6_output.c > +++ b/net/ipv6/ip6_output.c > @@ -596,6 +596,35 @@ int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr) > return offset; > } > > +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; > +} > + > +void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt) > +{ > + fhdr->identification = htonl(__ipv6_select_ident(&rt->rt6i_dst.addr)); > +} > + > int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *)) > { > struct sk_buff *frag; > @@ -680,7 +709,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *)) > skb_reset_network_header(skb); > memcpy(skb_network_header(skb), tmp_hdr, hlen); > > - ipv6_select_ident(fh); > + ipv6_select_ident(fh, rt); > fh->nexthdr = nexthdr; > fh->reserved = 0; > fh->frag_off = htons(IP6_MF); > @@ -826,7 +855,7 @@ slow_path: > fh->nexthdr = nexthdr; > fh->reserved = 0; > if (!frag_id) { > - ipv6_select_ident(fh); > + ipv6_select_ident(fh, rt); > frag_id = fh->identification; > } else > fh->identification = frag_id; > @@ -1030,7 +1059,8 @@ static inline int ip6_ufo_append_data(struct sock *sk, > int getfrag(void *from, char *to, int offset, int len, > int odd, struct sk_buff *skb), > void *from, int length, int hh_len, int fragheaderlen, > - int transhdrlen, int mtu,unsigned int flags) > + int transhdrlen, int mtu,unsigned int flags, > + struct rt6_info *rt) > > { > struct sk_buff *skb; > @@ -1075,7 +1105,7 @@ static inline int ip6_ufo_append_data(struct sock *sk, > skb_shinfo(skb)->gso_size = (mtu - fragheaderlen - > sizeof(struct frag_hdr)) & ~7; > skb_shinfo(skb)->gso_type = SKB_GSO_UDP; > - ipv6_select_ident(&fhdr); > + ipv6_select_ident(&fhdr, rt); > skb_shinfo(skb)->ip6_frag_id = fhdr.identification; > __skb_queue_tail(&sk->sk_write_queue, skb); > > @@ -1231,7 +1261,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to, > > err = ip6_ufo_append_data(sk, getfrag, from, length, > hh_len, fragheaderlen, > - transhdrlen, mtu, flags); > + transhdrlen, mtu, flags, rt); > if (err) > goto error; > return 0; > diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c > index 119edb6..b6de206 100644 > --- a/net/ipv6/udp.c > +++ b/net/ipv6/udp.c > @@ -1360,7 +1360,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, int features) > fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen); > fptr->nexthdr = nexthdr; > fptr->reserved = 0; > - ipv6_select_ident(fptr); > + ipv6_select_ident(fptr, (struct rt6_info *)skb_dst(skb)); > > /* Fragment the skb. ipv6 header and the remaining fields of the > * fragment header are updated in ipv6_gso_segment()
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
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...
diff --git a/include/net/ipv6.h b/include/net/ipv6.h index 96e50e0..24175cf 100644 --- a/include/net/ipv6.h +++ b/include/net/ipv6.h @@ -460,17 +460,7 @@ static inline int ipv6_addr_diff(const struct in6_addr *a1, const struct in6_add return __ipv6_addr_diff(a1, a2, sizeof(struct in6_addr)); } -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); -} +extern void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt); /* * Prototypes exported by ipv6 diff --git a/include/net/transp_v6.h b/include/net/transp_v6.h index 42a0eb6..70eef79 100644 --- a/include/net/transp_v6.h +++ b/include/net/transp_v6.h @@ -16,6 +16,8 @@ extern struct proto tcpv6_prot; struct flowi; +extern void initialize_hashidentrnd(void); + /* extention headers */ extern int ipv6_exthdrs_init(void); extern void ipv6_exthdrs_exit(void); diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c index 978e80e..ca171b7 100644 --- a/net/ipv6/af_inet6.c +++ b/net/ipv6/af_inet6.c @@ -1081,6 +1081,8 @@ static int __init inet6_init(void) goto out; } + initialize_hashidentrnd(); + err = proto_register(&tcpv6_prot, 1); if (err) goto out; diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c index 5f8d242..ed0c1a7 100644 --- a/net/ipv6/ip6_output.c +++ b/net/ipv6/ip6_output.c @@ -596,6 +596,35 @@ int ip6_find_1stfragopt(struct sk_buff *skb, u8 **nexthdr) return offset; } +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++; + } while (cmpxchg(pid, oldid, newid) != oldid); + + return hash + newid; +} + +void ipv6_select_ident(struct frag_hdr *fhdr, struct rt6_info *rt) +{ + fhdr->identification = htonl(__ipv6_select_ident(&rt->rt6i_dst.addr)); +} + int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *)) { struct sk_buff *frag; @@ -680,7 +709,7 @@ int ip6_fragment(struct sk_buff *skb, int (*output)(struct sk_buff *)) skb_reset_network_header(skb); memcpy(skb_network_header(skb), tmp_hdr, hlen); - ipv6_select_ident(fh); + ipv6_select_ident(fh, rt); fh->nexthdr = nexthdr; fh->reserved = 0; fh->frag_off = htons(IP6_MF); @@ -826,7 +855,7 @@ slow_path: fh->nexthdr = nexthdr; fh->reserved = 0; if (!frag_id) { - ipv6_select_ident(fh); + ipv6_select_ident(fh, rt); frag_id = fh->identification; } else fh->identification = frag_id; @@ -1030,7 +1059,8 @@ static inline int ip6_ufo_append_data(struct sock *sk, int getfrag(void *from, char *to, int offset, int len, int odd, struct sk_buff *skb), void *from, int length, int hh_len, int fragheaderlen, - int transhdrlen, int mtu,unsigned int flags) + int transhdrlen, int mtu,unsigned int flags, + struct rt6_info *rt) { struct sk_buff *skb; @@ -1075,7 +1105,7 @@ static inline int ip6_ufo_append_data(struct sock *sk, skb_shinfo(skb)->gso_size = (mtu - fragheaderlen - sizeof(struct frag_hdr)) & ~7; skb_shinfo(skb)->gso_type = SKB_GSO_UDP; - ipv6_select_ident(&fhdr); + ipv6_select_ident(&fhdr, rt); skb_shinfo(skb)->ip6_frag_id = fhdr.identification; __skb_queue_tail(&sk->sk_write_queue, skb); @@ -1231,7 +1261,7 @@ int ip6_append_data(struct sock *sk, int getfrag(void *from, char *to, err = ip6_ufo_append_data(sk, getfrag, from, length, hh_len, fragheaderlen, - transhdrlen, mtu, flags); + transhdrlen, mtu, flags, rt); if (err) goto error; return 0; diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c index 119edb6..b6de206 100644 --- a/net/ipv6/udp.c +++ b/net/ipv6/udp.c @@ -1360,7 +1360,7 @@ static struct sk_buff *udp6_ufo_fragment(struct sk_buff *skb, int features) fptr = (struct frag_hdr *)(skb_network_header(skb) + unfrag_ip6hlen); fptr->nexthdr = nexthdr; fptr->reserved = 0; - ipv6_select_ident(fptr); + ipv6_select_ident(fptr, (struct rt6_info *)skb_dst(skb)); /* Fragment the skb. ipv6 header and the remaining fields of the * fragment header are updated in ipv6_gso_segment()