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

login
register
mail settings
Submitter Bernhard Kaindl
Date Oct. 12, 2011, 9:23 p.m.
Message ID <1318454600-18349-1-git-send-email-bernhard.kaindl@gmx.net>
Download mbox | patch
Permalink /patch/119304/
State Changes Requested
Headers show

Comments

Bernhard Kaindl - Oct. 12, 2011, 9:23 p.m.
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(-)
Mike Frysinger - Oct. 12, 2011, 9:48 p.m.
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
Bernhard Kaindl - Oct. 14, 2011, 7:44 a.m.
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
Mike Frysinger - Oct. 14, 2011, 3:41 p.m.
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

Patch

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