linux-kernel - Re: [PATCH 05/17] Blackfin: convert to generic checksum code

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: <200906191105.37732.arnd@arndb.de>
Date:	Fri, 19 Jun 2009 11:05:37 +0200
From:	Arnd Bergmann <arnd@...db.de>
To:	Mike Frysinger <vapier.adi@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	uclinux-dist-devel@...ckfin.uclinux.org,
	Michal Simek <monstr@...str.eu>,
	Paul Mundt <lethal@...ux-sh.org>
Subject: Re: [PATCH 05/17] Blackfin: convert to generic checksum code

On Friday 19 June 2009, Mike Frysinger wrote:
> On Mon, Jun 15, 2009 at 07:04, Arnd Bergmann wrote:
> > On Sunday 14 June 2009, Mike Frysinger wrote:
> >> The Blackfin port only implemented an optimized version of the
> >> csum_tcpudp_nofold function, so convert everything else to the new
> >> generic code.
> >>
> >> Signed-off-by: Mike Frysinger <vapier@...too.org>
> >
> > Have you tested this one well? I was as careful as possible with the
> > version I added, but it was basically only tested on microblaze, which
> > has a different endianess from blackfin. Some areas of the code may be
> > sensitive to this.
> 
> i did some tests and it looks like do_csum() is broken :(

Thanks for testing. Indeed it turns out to be an endian-problem with
the handling of the last byte:

> here's the input:
> do_csum({ 0xff 0xfb 0x01 }, 3)
> 
> and here's the output:
> Blackfin: 0xfc00
> generic: 0xfcff
>
> if i run the two funcs on my x86, i see similar behavior.  the
> attached csum-test.c contains the csum code from lib/checksum.c and
> arch/blackfin/lib/checksum.c and shows the problem.

x86 and blackfin are both little-endian, so your variant is correct
there. They add the 0x01 to the low byte of the 16-bit word, while
on big-endian machines, you have to add it to the high byte.

> -mike
> csum-test.c
>   #include <stdio.h>
> 
> static inline unsigned short gen_from32to16(unsigned long x)
> {
>         /* add up 16-bit and 16-bit for 16+c bit */
>         x = (x & 0xffff) + (x >> 16);
>         /* add up carry.. */
>         x = (x & 0xffff) + (x >> 16);
>         return x;
> }
> 
> static unsigned int gen_do_csum(const unsigned char *buff, int len)
> {
> ...
>         if (len & 1)
>                 result += (*buff << 8);

On little-endian, this needs to be

	if (len & 1)
		result += *buff;

> static unsigned short bf_do_csum(const unsigned char *buff, int len)
> {
> ...
>         if (len > 0)
>                 sum += *buff;

Same problem, on big-endian this would need to be
	if (len > 0)
		sum += (*buff << 8);

The bug potentially also exists in arch/sh/lib64/c-checksum.c, which only
works on little-endian machines, while the sh architecture code appears
dual-endian. Paul, is your lib64/c-checksum.c implementation potentially
run on big-endian machines, or is sh64 guaranteed to be little-endian?

I've committed the patch below now.

	Arnd <><

---
>From e01fed86629737809c46dcb8b807347e84640b70 Mon Sep 17 00:00:00 2001
From: Arnd Bergmann <arnd@...db.de>
Date: Fri, 19 Jun 2009 10:41:19 +0200
Subject: [PATCH] lib/checksum.c: fix endianess bug

The new generic checksum code has a small dependency on endianess and
worked only on big-endian systems. I could not find a nice efficient
way to express this, so I added an #ifdef. Using
'result += le16_to_cpu(*buff);' would have worked as well, but
would be slightly less efficient on big-endian systems and IMHO
would not be clearer.

Also fix a bug that prevents this from working on 64-bit machines.
If you have a 64-bit CPU and want to use the generic checksum
code, you should probably do some more optimizations anyway, but
at least the code should not break.

Reported-by: Mike Frysinger <vapier@...too.org>
Signed-off-by: Arnd Bergmann <arnd@...db.de>
---
 lib/checksum.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/lib/checksum.c b/lib/checksum.c
index 12e5a1c..4a1c84a 100644
--- a/lib/checksum.c
+++ b/lib/checksum.c
@@ -71,7 +71,7 @@ static unsigned int do_csum(const unsigned char *buff, int len)
 		if (count) {
 			unsigned long carry = 0;
 			do {
-				unsigned long w = *(unsigned long *) buff;
+				unsigned long w = *(unsigned int *) buff;
 				count--;
 				buff += 4;
 				result += carry;
@@ -87,7 +87,11 @@ static unsigned int do_csum(const unsigned char *buff, int len)
 		}
 	}
 	if (len & 1)
+#ifdef __LITTLE_ENDIAN
+		result += *buff;
+#else
 		result += (*buff << 8);
+#endif
 	result = from32to16(result);
 	if (odd)
 		result = ((result >> 8) & 0xff) | ((result & 0xff) << 8);
-- 
1.6.3.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/