Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1540675

Re: [PATCH v2] siphash: add cryptographically secure hashtable function

Show all headers | View raw

Hey Eric,

Lots of good points; thanks for the review. Responses are inline below.

On Mon, Dec 12, 2016 at 6:42 AM, Eric Biggers <ebiggers3@gmail.com> wrote:
> Maybe add to the help text for CONFIG_TEST_HASH that it now tests siphash too?

Good call. Will do.

> This assumes the key and message buffers are aligned to __alignof__(u64).
> Unless that's going to be a clearly documented requirement for callers, you
> should use get_unaligned_le64() instead.  And you can pass a 'u8 *' directly to
> get_unaligned_le64(), no need for a helper function.

I had thought about that briefly, but just sort of figured most people
were passing in aligned variables... but that's a pretty bad
assumption to make especially for 64-bit alignment. I'll switch to
using the get_unaligned functions.

[As a side note, I wonder if crypto/chacha20_generic.c should be using
the unaligned functions instead too, at least for the iv reading...]

> It makes sense for this to return a u64, but that means the cpu_to_le64() is
> wrong, since u64 indicates CPU endianness.  It should just return 'b'.

At first I was very opposed to making this change, since by returning
a value with an explicit byte order, you can cast to u8 and have
uniform indexed byte access across platforms. But of course this
doesn't make any sense, since it's returning a u64, and it makes all
other bitwise operations non-uniform anyway.  I checked with JP
(co-creator of siphash, CC'd) and he confirmed your suspicion that it
was just to make the test vector comparison easier and for some
byte-wise uniformity, but that it's not strictly necessary. So, I've
removed that last cpu_to_le64, and I've also refactored those test
vectors to be written as ULL literals, so that a simple == integer
comparison will work across platforms.

> Can you mention in a comment where the test vectors came from?

Sure, will do.


> If you make the output really be CPU-endian like I'm suggesting then this will
> need to be something like:
>
>         if (out != get_unaligned_le64(test_vectors[i])) {
>
> Or else make the test vectors be an array of u64.

Yep, I wound up doing that.

Thanks Eric! Will submit a v3 soon if nobody else has comments.

Jason

Back to linux.kernel | Previous | Next — Previous in thread | Find similar | Unroll thread

Thread

Re: [kernel-hardening] [PATCH] siphash: add cryptographically secure  hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-11 16:40 +0100
  Re: [kernel-hardening] [PATCH] siphash: add cryptographically secure  hashtable function Greg KH <gregkh@linuxfoundation.org> - 2016-12-11 21:50 +0100
    [PATCH v2] siphash: add cryptographically secure hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-12 04:50 +0100
      Re: [PATCH v2] siphash: add cryptographically secure hashtable function Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-12 05:10 +0100
        Re: [PATCH v2] siphash: add cryptographically secure hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-12 06:50 +0100
          Re: [PATCH v2] siphash: add cryptographically secure hashtable function Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-12 22:40 +0100
            [PATCH v3] siphash: add cryptographically secure hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-12 23:30 +0100
              Re: [PATCH v3] siphash: add cryptographically secure hashtable  function Andi Kleen <ak@linux.intel.com> - 2016-12-13 00:10 +0100
              Re: [PATCH v3] siphash: add cryptographically secure hashtable  function Eric Biggers <ebiggers3@gmail.com> - 2016-12-13 09:50 +0100
                Re: [PATCH v3] siphash: add cryptographically secure hashtable function Linus Torvalds <torvalds@linux-foundation.org> - 2016-12-13 20:30 +0100
                Re: [PATCH v3] siphash: add cryptographically secure hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-13 23:50 +0100
                [PATCH v4] siphash: add cryptographically secure hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-13 23:50 +0100
      Re: [PATCH v2] siphash: add cryptographically secure hashtable  function Eric Biggers <ebiggers3@gmail.com> - 2016-12-12 06:50 +0100
        Re: [PATCH v2] siphash: add cryptographically secure hashtable function "Jason A. Donenfeld" <Jason@zx2c4.com> - 2016-12-12 22:20 +0100

csiph-web