linux-kernel - Re: [PATCH 1/2] staging: slicoss: rewrite eeprom 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:	Mon, 19 May 2014 13:49:41 -0700
From:	David Matlack <matlackdavid@...il.com>
To:	Dan Carpenter <dan.carpenter@...cle.com>
Cc:	gregkh@...uxfoundation.org, devel@...verdev.osuosl.org,
	charrer@...critech.com, linux-kernel@...r.kernel.org,
	Lior Dotan <liodot@...il.com>
Subject: Re: [PATCH 1/2] staging: slicoss: rewrite eeprom checksum code

On Mon, May 19, 2014 at 2:16 AM, Dan Carpenter <dan.carpenter@...cle.com> wrote:
> This patch is great, thanks, don't resend.  But I wish the subject said
> "fix" instead of "rewrite".  I was expecting it to just be a cleanup.
>
> It helps a lot if `git log --oneline` says which patches should be
> backported.
>
> regards,
> dan carpenter
>

Thanks for the review Dan. I know you said not resend but I'd like to clean up
the patch a bit if you don't mind:

* Add a comment making it clear this is RFC 1071

* Remove the byte swapping code. Specifically,

+       if ((unsigned long) eeprom & 1) {
+               leading_byte = bp;
+               bp++;
        }

and

+       if (leading_byte) {
+               checksum = __reduce(checksum);
+               checksum <<= 8;
+
+               final_word = *leading_byte;
+               if (trailing_byte)
+                       final_word |= *trailing_byte << 8;
+       }

  After reading the RFC [1], I don't think the it's correct in either
  implementation (the bytes in the final result are in the wrong order). And
  since this function is only used to checksum the eeprom, and not checksum a
  fragment of a IP packet, it's not needed. So we can just remove it.

* s/rewrite/fix/ in the subject

[1] http://tools.ietf.org/html/rfc1071 (see Section 2.(A) and 2.(B))
--
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/