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]

Date:	Tue, 23 Jun 2009 23:14:17 +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

I missed your mail last week and only now stumbled over it after Linus
has pulled my tree.

On Friday 19 June 2009, Mike Frysinger wrote:
> >
> > sounds like a good idea.
> 
> how about the attached

Mostly good, but needs some improvements. At least it helped me
track down the last problem.

> > lib/checksum.c: Fix another endianess bug
> 
> hrm, still not quite :/
> 
> the attached test code shows failures in every case

When I tried running it on x86-64, it only showed failures for
numbers 1, 2 and 4. I fixed them with this patch:
---
lib/checksum: fix one more thinko

When do_csum gets unaligned data, we really need to treat
the first byte as an even byte, not an odd byte, because
we swap the two halves later.

Found by Mike's checksum-selftest module.

Reported-by: Mike Frysinger <vapier.adi@...il.com>
Signed-off-by: Arnd Bergmann <arnd@...db.de>

diff --git a/lib/checksum.c b/lib/checksum.c
index b08c2d0..0975087 100644
--- a/lib/checksum.c
+++ b/lib/checksum.c
@@ -57,9 +57,9 @@ static unsigned int do_csum(const unsigned char *buff, int len)
 	odd = 1 & (unsigned long) buff;
 	if (odd) {
 #ifdef __LITTLE_ENDIAN
-		result = *buff;
-#else
 		result += (*buff << 8);
+#else
+		result = *buff;
 #endif
 		len--;
 		buff++;
---

>extern unsigned short do_csum(const unsigned char *buff, int len);
>

do_csum is really an internal function. IMHO we should better check
csum_partial(), ip_fast_csum(), csum_fold(), csum_tcpudp_magic()
and ip_compute_csum(), or at least a subset of them.

>static unsigned char __initdata do_csum_data2[] = {
>        0x0d, 0x0a,
>};
>static unsigned char __initdata do_csum_data3[] = {
>        0xff, 0xfb, 0x01,
>};
> ...
>static struct do_csum_data __initdata do_csum_data[] = {
>        DO_CSUM_DATA(1, 0x0020),
>        DO_CSUM_DATA(2, 0xfc00),
>        DO_CSUM_DATA(3, 0x0a0d),
>        DO_CSUM_DATA(5, 0x7fc4),
>        DO_CSUM_DATA(7, 0x7597),
>        DO_CSUM_DATA(255, 0x4f96),
>};

You mixed up do_csum_data2 and do_csum_data3, so they will always
show up as incorrect. Also, the expected checksum is endian-dependent.
The test module should either be modified to expect 0xffff to be
returned in every case, or should use le16_to_cpu(0x0020) etc.

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