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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Tue, 23 Jun 2009 17:53:44 -0400
From:	Mike Frysinger <vapier.adi@...il.com>
To:	Arnd Bergmann <arnd@...db.de>
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 Tue, Jun 23, 2009 at 17:14, Arnd Bergmann wrote:
> --- 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++;
> ---

thanks, my tests seem to work now ... i was doing telnet/rsh stuff as
well as icmp/udp/tcp flooding.  i'll convert Blackfin to generic
checksum post 2.6.31 so we can test it some more in the meantime.

>>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.

and this is an internal tester ... if we're going to let arches
override it, then its function signature are pretty much set in stone.
 i agree we should test every checksum function, but i think only
testing do_csum indirectly would be detrimental to the people who
would want to use this -- arch maintainers looking to implement
do_csum() themselves.  otherwise they need to step through the
surrounding functions a bit to find the exact values given to
do_csum() and the exact value expected back such that the calling
function still works.  and every arch guy is going to do this same
thing.

>>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.

i was going through different ways of formatting the data and looks
like i messed that up, thanks

> 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

i'm a bit of a nub here wrt checksuming.  the test module i came up
with by simply flooding my board and copying & pasting mismatched
buffers ;).  are you saying the following should be OK ?
-    if (tret != do_csum_data[i].ret) {
+    if (tret != le16_to_cpu(do_csum_data[i].ret)) {

unless there is a macro i could use that'd do the expansion at CPP
time so i'd write the test data as:
static struct do_csum_data __initdata do_csum_data[] = {
        DO_CSUM_DATA(1, le16_to_cpu(0x0020)),
...
-mike
--
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/