[U-Boot] [PATCH] net/dns.c: Fix broken endian handling in dns command

[U-Boot] [PATCH] net/dns.c: Fix broken endian handling in dns command

[Bernhard Kaindl]

From: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>

The U-Boot dns command only worked in little-endian CPUs so far because it
was based on an antique version of the TADNS source which was using a broken
macro to read the shorts found in DNS reply messages by shifting the LSB from
the message into the CPU's MSB of a short int and the MSB from the stream into
the LSB part of the CPU's short int. So far, so twisted.

To correct the twisted bytes, the code used ntohs() as a byte-swapping function
to swap the MSB back where it belongs and vice versa.

This works fine, except that ntohs() naturally does nothing on big-endian CPUs.

So on big-endian CPUs, the MSB from the message stayed in the LSB of the CPU
and vice versa.

Ditch this brain-deadness by just shifting the MSB from the network byte
stream of the reply message into the right (MSB) location of a short int
and putting the LSB from the network byte stream as the lower byte of it,
and we are done with reading the short from the network stream for both
endianesses, no ntohs() or such!

The current TADNS source uses the same macro meanwhile.

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>
Cc: Pieter Voorthuijsen <pieter.voorthuijsen@prodrive.nl>
Cc: Robin Getz <rgetz@blackfin.uclinux.org>
---
 net/dns.c |   22 ++++++++++------------
 1 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/net/dns.c b/net/dns.c
index b51d1bd..7034183 100644
--- a/net/dns.c
+++ b/net/dns.c
@@ -28,6 +28,9 @@
 
 #include "dns.h"
 
+/* p is a char *, we shift the MSB up in our CPU endianness -> no ntohs used! */
+#define netbytes2hs(p) ((p)[0] << 8 | (p)[1])
+
 char *NetDNSResolve;	/* The host to resolve  */
 char *NetDNSenvvar;	/* The envvar to store the answer in */
 
@@ -109,7 +112,6 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 	int found, stop, dlen;
 	char IPStr[22];
 	IPaddr_t IPAddress;
-	short tmp;
 
 
 	debug("%s\n", __func__);
@@ -120,14 +122,14 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		debug("0x%p - 0x%.2x  0x%.2x  0x%.2x  0x%.2x\n",
 			pkt+i, pkt[i], pkt[i+1], pkt[i+2], pkt[i+3]);
 
-	/* We sent 1 query. We want to see more that 1 answer. */
+	/* We sent one query. We want to have a single answer: */
 	header = (struct header *) pkt;
 	if (ntohs(header->nqueries) != 1)
 		return;
 
 	/* Received 0 answers */
 	if (header->nanswers == 0) {
-		puts("DNS server returned no answers\n");
+		puts("DNS: host not found\n");
 		NetState = NETLOOP_SUCCESS;
 		return;
 	}
@@ -139,9 +141,8 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		continue;
 
 	/* We sent query class 1, query type 1 */
-	tmp = p[1] | (p[2] << 8);
-	if (&p[5] > e || ntohs(tmp) != DNS_A_RECORD) {
-		puts("DNS response was not A record\n");
+	if (&p[5] > e || netbytes2hs(p+1) != DNS_A_RECORD) {
+		puts("DNS: response not an A record\n");
 		NetState = NETLOOP_SUCCESS;
 		return;
 	}
@@ -160,14 +161,12 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		}
 		debug("Name (Offset in header): %d\n", p[1]);
 
-		tmp = p[2] | (p[3] << 8);
-		type = ntohs(tmp);
+		type = netbytes2hs(p+2);
 		debug("type = %d\n", type);
 		if (type == DNS_CNAME_RECORD) {
 			/* CNAME answer. shift to the next section */
 			debug("Found canonical name\n");
-			tmp = p[10] | (p[11] << 8);
-			dlen = ntohs(tmp);
+			dlen = netbytes2hs(p+10);
 			debug("dlen = %d\n", dlen);
 			p += 12 + dlen;
 		} else if (type == DNS_A_RECORD) {
@@ -181,8 +180,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 
 	if (found && &p[12] < e) {
 
-		tmp = p[10] | (p[11] << 8);
-		dlen = ntohs(tmp);
+		dlen = netbytes2hs(p+10);
 		p += 12;
 		memcpy(&IPAddress, p, 4);
 
-- 
1.7.3.4

[U-Boot] [PATCH] net/dns.c: Fix broken endian handling in dns command

[Mike Frysinger]

On Wednesday 12 October 2011 17:23:20 Bernhard Kaindl wrote:
> The U-Boot dns command only worked in little-endian CPUs so far because it
> was based on an antique version of the TADNS source which was using a
> broken macro to read the shorts found in DNS reply messages by shifting
> the LSB from the message into the CPU's MSB of a short int and the MSB
> from the stream into the LSB part of the CPU's short int. So far, so
> twisted.
> 
> To correct the twisted bytes, the code used ntohs() as a byte-swapping
> function to swap the MSB back where it belongs and vice versa.
> 
> This works fine, except that ntohs() naturally does nothing on big-endian
> CPUs.
> 
> So on big-endian CPUs, the MSB from the message stayed in the LSB of the
> CPU and vice versa.
> 
> Ditch this brain-deadness by just shifting the MSB from the network byte
> stream of the reply message into the right (MSB) location of a short int
> and putting the LSB from the network byte stream as the lower byte of it,
> and we are done with reading the short from the network stream for both
> endianesses, no ntohs() or such!

please use a standard macro instead of inventing yet another.  we've got the 
rich Linux api which should cover every case you could possibly need.
	cpu_to_{l,b}e{16,32,64}(...)
	{l,b}e_to_cpu{16,32,63}(...)

see include/linux/byteorder/
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111012/701fc3e3/attachment.pgp 

[U-Boot] [PATCH] net/dns.c: Fix broken endian handling in dns command

[Bernhard Kaindl]

Am 12.10.2011 23:48, schrieb Mike Frysinger:
>>
>> Ditch this brain-deadness by just shifting the MSB from the network byte
>> stream of the reply message into the right (MSB) location of a short int
>> and putting the LSB from the network byte stream as the lower byte of it,
>> and we are done with reading the short from the network stream for both
>> endianesses, no ntohs() or such!
>
> please use a standard macro instead of inventing yet another.  we've got the
> rich Linux api which should cover every case you could possibly need.
> 	cpu_to_{l,b}e{16,32,64}(...)
> 	{l,b}e_to_cpu{16,32,63}(...)
>
> see include/linux/byteorder/
> -mike

No, of course I looked, but didn't find what is needed.

These are just conditional byte swaps in 16,32 or 64 bit, not more.

The code needs is a conversion from an uneven "const char *" to u16!

It ___is___ possible to use be_to_cpu16, but it needs ugly casting:

         if (&p[5] > e || be_to_cpu16(*(u16 *)(p+1)) != DNS_A_RECORD) {

To not be distracted by reading the ugly casting, one would have to put 
this into a macro again, but if you really like to see be_to_cpu16 so 
much, I can change the macro to:

#define netstring_to_cpu_short(p)    be16_to_cpu(*(u16 *)(p))

This result doesn't make it more readable than what I already posted.

I'd prefer the already sent patch.

Best Regards,
Bernhard

[U-Boot] [PATCH] net/dns.c: Fix broken endian handling in dns command

[Mike Frysinger]

On Friday 14 October 2011 03:44:13 Bernhard Kaindl wrote:
> Am 12.10.2011 23:48, schrieb Mike Frysinger:
> >> Ditch this brain-deadness by just shifting the MSB from the network byte
> >> stream of the reply message into the right (MSB) location of a short int
> >> and putting the LSB from the network byte stream as the lower byte of
> >> it, and we are done with reading the short from the network stream for
> >> both endianesses, no ntohs() or such!
> > 
> > please use a standard macro instead of inventing yet another.  we've got
> > the rich Linux api which should cover every case you could possibly
> > need.
> > 
> > 	cpu_to_{l,b}e{16,32,64}(...)
> > 	{l,b}e_to_cpu{16,32,63}(...)
> > 
> > see include/linux/byteorder/
> 
> No, of course I looked, but didn't find what is needed.
> 
> These are just conditional byte swaps in 16,32 or 64 bit, not more.
> 
> The code needs is a conversion from an uneven "const char *" to u16!
> 
> It ___is___ possible to use be_to_cpu16, but it needs ugly casting:
> 
>          if (&p[5] > e || be_to_cpu16(*(u16 *)(p+1)) != DNS_A_RECORD) {
> 
> To not be distracted by reading the ugly casting, one would have to put
> this into a macro again, but if you really like to see be_to_cpu16 so
> much, I can change the macro to:
> 
> #define netstring_to_cpu_short(p)    be16_to_cpu(*(u16 *)(p))
> 
> This result doesn't make it more readable than what I already posted.

sounds like you want get_unaligned_be16()
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111014/a6f62b68/attachment.pgp 

[U-Boot] [PATCH] net/dns.c: Fix endian conversion for big-endian in dns command

[Bernhard Kaindl]

From: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>

net/dns.c used endian conversion macros wrongly (shorts in reply
were put swapped into CPU, and then ntohs() was used to swap it
back, which broke on big-endian).

Fix this by using the correct linux conversion macro for reading
a unaligned short in network byte order: get_unaligned_be16()
Thanks to Mike Frysinger pointing at the best macro to use.

Tested on big and little endian qemu boards (mips and versatile)

Signed-off-by: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>
Cc: Pieter Voorthuijsen <pieter.voorthuijsen@prodrive.nl>
Cc: Robin Getz <rgetz@blackfin.uclinux.org>
---
 net/dns.c |   20 ++++++++------------
 1 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/net/dns.c b/net/dns.c
index b51d1bd..7a3f1f9 100644
--- a/net/dns.c
+++ b/net/dns.c
@@ -25,6 +25,7 @@
 #include <common.h>
 #include <command.h>
 #include <net.h>
+#include <asm/unaligned.h>
 
 #include "dns.h"
 
@@ -109,7 +110,6 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 	int found, stop, dlen;
 	char IPStr[22];
 	IPaddr_t IPAddress;
-	short tmp;
 
 
 	debug("%s\n", __func__);
@@ -120,14 +120,14 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		debug("0x%p - 0x%.2x  0x%.2x  0x%.2x  0x%.2x\n",
 			pkt+i, pkt[i], pkt[i+1], pkt[i+2], pkt[i+3]);
 
-	/* We sent 1 query. We want to see more that 1 answer. */
+	/* We sent one query. We want to have a single answer: */
 	header = (struct header *) pkt;
 	if (ntohs(header->nqueries) != 1)
 		return;
 
 	/* Received 0 answers */
 	if (header->nanswers == 0) {
-		puts("DNS server returned no answers\n");
+		puts("DNS: host not found\n");
 		NetState = NETLOOP_SUCCESS;
 		return;
 	}
@@ -139,9 +139,8 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		continue;
 
 	/* We sent query class 1, query type 1 */
-	tmp = p[1] | (p[2] << 8);
-	if (&p[5] > e || ntohs(tmp) != DNS_A_RECORD) {
-		puts("DNS response was not A record\n");
+	if (&p[5] > e || get_unaligned_be16(p+1) != DNS_A_RECORD) {
+		puts("DNS: response was not an A record\n");
 		NetState = NETLOOP_SUCCESS;
 		return;
 	}
@@ -160,14 +159,12 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 		}
 		debug("Name (Offset in header): %d\n", p[1]);
 
-		tmp = p[2] | (p[3] << 8);
-		type = ntohs(tmp);
+		type = get_unaligned_be16(p+2);
 		debug("type = %d\n", type);
 		if (type == DNS_CNAME_RECORD) {
 			/* CNAME answer. shift to the next section */
 			debug("Found canonical name\n");
-			tmp = p[10] | (p[11] << 8);
-			dlen = ntohs(tmp);
+			dlen = get_unaligned_be16(p+10);
 			debug("dlen = %d\n", dlen);
 			p += 12 + dlen;
 		} else if (type == DNS_A_RECORD) {
@@ -181,8 +178,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
 
 	if (found && &p[12] < e) {
 
-		tmp = p[10] | (p[11] << 8);
-		dlen = ntohs(tmp);
+		dlen = get_unaligned_be16(p+10);
 		p += 12;
 		memcpy(&IPAddress, p, 4);
 
-- 
1.7.3.4

[U-Boot] [PATCH] net/dns.c: Fix endian conversion for big-endian in dns command

[Robin Getz]

On Sun 16 Oct 2011 05:59, Bernhard Kaindl pondered:
> From: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>
> 
> net/dns.c used endian conversion macros wrongly (shorts in reply
> were put swapped into CPU, and then ntohs() was used to swap it
> back, which broke on big-endian).
> 
> Fix this by using the correct linux conversion macro for reading
> a unaligned short in network byte order: get_unaligned_be16()
> Thanks to Mike Frysinger pointing at the best macro to use.
> 
> Tested on big and little endian qemu boards (mips and versatile)

This also tweaks some error messages and comments, which isn't captured in the change log?

Otherwise, Ack from me.

> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>
> Cc: Pieter Voorthuijsen <pieter.voorthuijsen@prodrive.nl>
> Cc: Robin Getz <rgetz@blackfin.uclinux.org>
> ---
>  net/dns.c |   20 ++++++++------------
>  1 files changed, 8 insertions(+), 12 deletions(-)
> 
> diff --git a/net/dns.c b/net/dns.c
> index b51d1bd..7a3f1f9 100644
> --- a/net/dns.c
> +++ b/net/dns.c
> @@ -25,6 +25,7 @@
>  #include <common.h>
>  #include <command.h>
>  #include <net.h>
> +#include <asm/unaligned.h>
>  
>  #include "dns.h"
>  
> @@ -109,7 +110,6 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>  	int found, stop, dlen;
>  	char IPStr[22];
>  	IPaddr_t IPAddress;
> -	short tmp;
>  
>  
>  	debug("%s\n", __func__);
> @@ -120,14 +120,14 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>  		debug("0x%p - 0x%.2x  0x%.2x  0x%.2x  0x%.2x\n",
>  			pkt+i, pkt[i], pkt[i+1], pkt[i+2], pkt[i+3]);
>  
> -	/* We sent 1 query. We want to see more that 1 answer. */
> +	/* We sent one query. We want to have a single answer: */
>  	header = (struct header *) pkt;
>  	if (ntohs(header->nqueries) != 1)
>  		return;
>  
>  	/* Received 0 answers */
>  	if (header->nanswers == 0) {
> -		puts("DNS server returned no answers\n");
> +		puts("DNS: host not found\n");
>  		NetState = NETLOOP_SUCCESS;
>  		return;
>  	}
> @@ -139,9 +139,8 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>  		continue;
>  
>  	/* We sent query class 1, query type 1 */
> -	tmp = p[1] | (p[2] << 8);
> -	if (&p[5] > e || ntohs(tmp) != DNS_A_RECORD) {
> -		puts("DNS response was not A record\n");
> +	if (&p[5] > e || get_unaligned_be16(p+1) != DNS_A_RECORD) {
> +		puts("DNS: response was not an A record\n");
>  		NetState = NETLOOP_SUCCESS;
>  		return;
>  	}
> @@ -160,14 +159,12 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>  		}
>  		debug("Name (Offset in header): %d\n", p[1]);
>  
> -		tmp = p[2] | (p[3] << 8);
> -		type = ntohs(tmp);
> +		type = get_unaligned_be16(p+2);
>  		debug("type = %d\n", type);
>  		if (type == DNS_CNAME_RECORD) {
>  			/* CNAME answer. shift to the next section */
>  			debug("Found canonical name\n");
> -			tmp = p[10] | (p[11] << 8);
> -			dlen = ntohs(tmp);
> +			dlen = get_unaligned_be16(p+10);
>  			debug("dlen = %d\n", dlen);
>  			p += 12 + dlen;
>  		} else if (type == DNS_A_RECORD) {
> @@ -181,8 +178,7 @@ DnsHandler(uchar *pkt, unsigned dest, IPaddr_t sip, unsigned src, unsigned len)
>  
>  	if (found && &p[12] < e) {
>  
> -		tmp = p[10] | (p[11] << 8);
> -		dlen = ntohs(tmp);
> +		dlen = get_unaligned_be16(p+10);
>  		p += 12;
>  		memcpy(&IPAddress, p, 4);
>  

[U-Boot] [PATCH] net/dns.c: Fix endian conversion for big-endian in dns command

[Mike Frysinger]

Acked-by: Mike Frysinger <vapier@gentoo.org>
-mike
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
Url : http://lists.denx.de/pipermail/u-boot/attachments/20111016/21c5c1c5/attachment.pgp 

[U-Boot] [PATCH] net/dns.c: Fix endian conversion for big-endian in dns command

[Wolfgang Denk]

Dear Bernhard Kaindl,

In message <1318759162-10523-1-git-send-email-bernhard.kaindl@gmx.net> you wrote:
> From: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>
> 
> net/dns.c used endian conversion macros wrongly (shorts in reply
> were put swapped into CPU, and then ntohs() was used to swap it
> back, which broke on big-endian).
> 
> Fix this by using the correct linux conversion macro for reading
> a unaligned short in network byte order: get_unaligned_be16()
> Thanks to Mike Frysinger pointing at the best macro to use.
> 
> Tested on big and little endian qemu boards (mips and versatile)
> 
> Signed-off-by: Bernhard Kaindl <bernhard.kaindl@thalesgroup.com>
> Cc: Pieter Voorthuijsen <pieter.voorthuijsen@prodrive.nl>
> Cc: Robin Getz <rgetz@blackfin.uclinux.org>
> ---
>  net/dns.c |   20 ++++++++------------
>  1 files changed, 8 insertions(+), 12 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
The Wright Bothers weren't the first to fly. They were just the first
not to crash.