netdev - Re: [PATCH net-next 2/3] ipv6: use newly introduced __ipv6_addr_needs_scope_id and ipv6_iface_scope

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: <20130213103302.GC1096@order.stressinduktion.org>
Date:	Wed, 13 Feb 2013 11:33:02 +0100
From:	Hannes Frederic Sowa <hannes@...essinduktion.org>
To:	Brian Haley <brian.haley@...com>
Cc:	netdev@...r.kernel.org, yoshfuji@...ux-ipv6.org
Subject: Re: [PATCH net-next 2/3] ipv6: use newly introduced __ipv6_addr_needs_scope_id and ipv6_iface_scope_id

On Tue, Feb 12, 2013 at 09:51:06PM -0500, Brian Haley wrote:
> On 02/12/2013 07:13 PM, Hannes Frederic Sowa wrote:
> >> --- a/net/ipv6/af_inet6.c
> >> +++ b/net/ipv6/af_inet6.c
> >> @@ -323,7 +323,7 @@ int inet6_bind(struct socket *sock, struct sockaddr *uaddr, int addr_len)
> >>  			struct net_device *dev = NULL;
> >>  
> >>  			rcu_read_lock();
> >> -			if (addr_type & IPV6_ADDR_LINKLOCAL) {
> >> +			if (__ipv6_addr_needs_scope_id(addr_type)) {
> >>  				if (addr_len >= sizeof(struct sockaddr_in6) &&
> >>  				    addr->sin6_scope_id) {
> >>  					/* Override any existing binding, if another one
> > 
> > By trying to setup the multicast interface scoped routes by default I
> > just found a bug in this patch essentially breaking ipv6 multicast. I
> > overlooked that ipv6_addr_type strips off the scopes, thus my check if
> > a multicast address needs a scope_id always returns true. I'll check
> > if I can convert the ipv6_addr_type calls to __ipv6_addr_type and will
> > reroll the patch.  Sorry, my tests were too focused on interface/local
> > multicast. :(
> 
> I'd always thought of adding helper inlines like these in net/ipv6.h:
> 
> static inline bool ipv6_addr_linklocal(const struct in6_addr *a)
> {
>         return ((a->s6_addr32[0] & htonl(0xFFC00000)) == htonl(0xFE800000));
> }
> 
> static inline bool ipv6_addr_mc_linklocal(const struct in6_addr *a)
> {
>         return (((a->s6_addr32[0] & htonl(0xFF000000)) == htonl(0xFF000000)) &&
>                 ((a->s6_addr32[1] & 0x0F) == IPV6_ADDR_SCOPE_LINKLOCAL));
> }
> 
> Maybe something like that would help here?
> 
> When I saw this in patch 3/3 it just seemed like the long way to determine if
> the address was a link-local multicast:
> 
> 	!__ipv6_addr_needs_scope_id(__ipv6_addr_type(&hdr->daddr))
> 
> The helper isn't as generic as your patch, but more direct.

Yup, that would have prevented the bug. My idea was to introduce an
opaque type to have compiler warnings on misuse of addr_type. I'll have a look
later today on how to proceed with this patch. Thanks!

--
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