6242 sha512 is broken in grub

Review Request #100 — Created Oct. 7, 2015 and submitted

tsoome
illumos-gate
6242
9267603...
general

6242 sha512 is broken in grub

the root and immediate causes for checksum mismatch are missing type casts and wrong byte order in resulting checksum returned to caller.

However, this update also is addressing the case where last block is not full block - perhaps its not actually issue in zfs; also this code is using explicit 128bit message length setup.

build and boot from previously unaccessible pool with grub; accessing files from loader.

tsoome
tsoome
hans
  1. Ship It!
  2. 
      
mahrens
  1. It isn't clear to me why we need to byteswap the checksum here, but if it works then I'm happy. It would be great if you can also get Saso to review this since he wrote it originally.

    1. I havent checked the actual standard doc for exact explanation - I just assume our kernel module [sha2.c] (and libgcrypt for that matter) have implemented it according to standards;) and yes, without this byteswap indeed we get resulting bytes in resulting octlets in reverse order and therefore the checksum match will obviously fail - I actually did double-check this while I was developing the initial fix in loader project and while implementing this bugfix for grub.

    2. Matt: this is because the GRUB implementation of SHA2 is somewhat simplified and didn't use to do stuff 100% by the book (before this fix). So while calculating, SHA-512 keeps state in eight 64-bit registers labeled H[0] through H[7]. SHA-2 allows little-endian platforms to perform the actual transform arithmetic in native byte order to preserve speed, but for the final product to be consistent with the standard, little-endian hardware then needs to byte-swap output. This wasn't done in GRUB and instead I incorrectly used the little-endian state. If you have a look at Encode64 in usr/src/common/crypto/sha2/sha2.c (which is used during the final output phase in the regular in-kernel SHA2Final()), it byteswaps on little endian hardware.

  2. 
      
skiselkov
  1. Just one minor documentation issue, other than that looks great!

  2. You might want to add a note to this function to make it clear that it's always encoding in big-endian.

  3. 
      
tsoome
skiselkov
  1. Ship It!
  2. 
      
tsoome
Review request changed

Status: Closed (submitted)

Loading...