netdev - Re: [PATCH 02/05] ipv6: RFC4214 Support

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: <200711071658.59478.netdev@axxeo.de>
Date:	Wed, 7 Nov 2007 16:58:59 +0100
From:	Ingo Oeser <netdev@...eo.de>
To:	"Templin, Fred L" <Fred.L.Templin@...ing.com>
Cc:	netdev@...r.kernel.org
Subject: Re: [PATCH 02/05] ipv6: RFC4214 Support

Hi Fred,

some comments.

Templin, Fred L schrieb:
> From: Fred L. Templin <fred.l.templin@...ing.com>
> 
> This is experimental support for the Intra-Site Automatic
> Tunnel Addressing Protocol (ISATAP) per RFC4214. It uses
> the SIT module, and is configured using the unmodified
> "ip" utility with device names beginning with: "isatap".
> 
> The following diffs are specific to the Linux 2.6.23
> kernel distribution.
> 
> Signed-off-by: Fred L. Templin <fred.l.templin@...ing.com>
> 
> ---
> 
> --- linux-2.6.23/include/net/addrconf.h.orig	2007-10-09
> 13:31:38.000000000 -0700
> +++ linux-2.6.23/include/net/addrconf.h	2007-10-26 10:49:40.000000000
> -0700
> @@ -241,6 +241,34 @@ static inline int ipv6_addr_is_ll_all_ro
>  		addr->s6_addr32[3] == htonl(0x00000002));
>  }
>  
> +#if defined(CONFIG_IPV6_ISATAP)
> +static inline int ipv6_isatap_eui64(u8 *eui, __be32 *addr)
"addr" is only used for reading, not writing. No need to pass it as a pointer.

> +{
> +	__be32 ipv4 = ntohl(*addr);

ntohl(be32_value) != be32_value, so the _be32 attribution of ipv4 
is wrong here and sparse will scream.

> +
> +	eui[0] = 0;
> +
> +	/* Check for RFC3330 global address ranges */
> +	if (((ipv4 >= 0x01000000) && (ipv4 < 0x0a000000)) ||
> +	    ((ipv4 >= 0x0b000000) && (ipv4 < 0x7f000000)) ||
> +	    ((ipv4 >= 0x80000000) && (ipv4 < 0xa9fe0000)) ||
> +	    ((ipv4 >= 0xa9ff0000) && (ipv4 < 0xac100000)) ||
> +	    ((ipv4 >= 0xac200000) && (ipv4 < 0xc0a80000)) ||
> +	    ((ipv4 >= 0xc0a90000) && (ipv4 < 0xc6120000)) ||
> +	    ((ipv4 >= 0xc6140000) && (ipv4 < 0xe0000000))) eui[0] |=
> 0x2;
> +

Instead of converting network to host byte order at runtime 
and comparing the results to constants, let the compiler convert
the constants to network byte order and compare in network order.

so use:

 if (((*addr >= htonl(0x01000000)) && (*addr < htonl(0x0a000000))) || ....

instead. The compiler will notice that "0x01000000" is a constant and will
use "_constant_htonl()" automatically.


> +	eui[1] = 0; eui[2] = 0x5E; eui[3] = 0xFE;
> +	memcpy (eui+4, addr, 4);
> +	return (0);
> +}

Nitpick: 
	"return" is not a function. Please write "return 0;" instead.

> +
> +static inline int ipv6_addr_is_isatap(const struct in6_addr *addr)
> +{
> +       return (addr->s6_addr32[2] == __constant_htonl(0x02005EFE) ||
> +               addr->s6_addr32[2] == __constant_htonl(0x00005EFE));
> +}
> +#endif

The compiler will notice that "0x01000000" is a constant and will
use "_constant_htonl()" automatically. Please use simply htonl().


Best Regards

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