linux-crypto.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Hannes Frederic Sowa <hannes@stressinduktion.org>
To: "Jason A. Donenfeld" <Jason@zx2c4.com>
Cc: David Laight <David.Laight@aculab.com>,
	Netdev <netdev@vger.kernel.org>,
	"kernel-hardening@lists.openwall.com"
	<kernel-hardening@lists.openwall.com>,
	Jean-Philippe Aumasson <jeanphilippe.aumasson@gmail.com>,
	LKML <linux-kernel@vger.kernel.org>,
	Linux Crypto Mailing List <linux-crypto@vger.kernel.org>,
	"Daniel J . Bernstein" <djb@cr.yp.to>,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Eric Biggers <ebiggers3@gmail.com>
Subject: Re: [PATCH v2 1/4] siphash: add cryptographically secure hashtable function
Date: Thu, 15 Dec 2016 22:17:44 +0100	[thread overview]
Message-ID: <7861e8ca-69fe-30d1-3d79-27ee33a510d0@stressinduktion.org> (raw)
In-Reply-To: <CAHmME9rDCb=2rojJba13Uew9V9qAbxv1qcJGHwEAKoahxyE9QA@mail.gmail.com>

On 15.12.2016 21:43, Jason A. Donenfeld wrote:
> On Thu, Dec 15, 2016 at 9:31 PM, Hannes Frederic Sowa
> <hannes@stressinduktion.org> wrote:
>> ARM64 and x86-64 have memory operations that are not vector operations
>> that operate on 128 bit memory.
> 
> Fair enough. imull I guess.
> 
>> How do you know that the compiler for some architecture will not chose a
>> more optimized instruction to load a 64 bit memory value into two 32 bit
>> registers if you tell the compiler it is 8 byte aligned but it actually
>> isn't? I don't know the answer but telling the compiler some data is 8
>> byte aligned while it isn't really pretty much seems like a call for
>> trouble.
> 
> If a compiler is in the business of using special 64-bit instructions
> on 64-bit aligned data, then it is also the job of the compiler to
> align structs to 64-bits when passed __aligned(8), which is what we've
> done in this code. If the compiler were to hypothetically choose to
> ignore that and internally convert it to a __aligned(4), then it would
> only be able to do so with the knowledge that it will never use 64-bit
> aligned data instructions. But so far as I can tell, gcc always
> respects __aligned(8), which is why I use it in this patchset.
> 
> I think there might have been confusion here, because perhaps someone
> was hoping that since in6_addr is 128-bits, that the __aligned
> attribute would not be required and that the struct would just
> automatically be aligned to at least 8 bytes. But in fact, as I
> mentioned, in6_addr is actually composed of u32[4] and not u64[2], so
> it will only be aligned to 4 bytes, making the __aligned(8) necessary.
> 
> I think for the purposes of this patchset, this is a solved problem.
> There's the unaligned version of the function if you don't know about
> the data, and there's the aligned version if you're using
> __aligned(SIPHASH_ALIGNMENT) on your data. Plain and simple.

And I was exactly questioning this.

static unsigned int inet6_hash_frag(__be32 id, const struct in6_addr *saddr,
				    const struct in6_addr *daddr)
{
	net_get_random_once(&ip6_frags.rnd, sizeof(ip6_frags.rnd));
	return jhash_3words(ipv6_addr_hash(saddr), ipv6_addr_hash(daddr),
			    (__force u32)id, ip6_frags.rnd);
}

This function had a hash DoS (and kind of still has), but it has been
mitigated by explicit checks, I hope.

So you start looking for all the pointers where ipv6 addresses could
come from and find some globally defined struct where I would need to
put the aligned(SIPHASH_ALIGNMENT) into to make this work on 32 bit
code? Otherwise just the unaligned version is safe on 32 bit code.

Who knows this? It isn't even obvious by looking at the header!

I would be interested if the compiler can actually constant-fold the
address of the stack allocation with an simple if () or some
__builtin_constant_p fiddeling, so we don't have this constant review
overhead to which function we pass which data. This would also make
this whole discussion mood.

Bye,
Hannes

  parent reply	other threads:[~2016-12-15 21:17 UTC|newest]

Thread overview: 75+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-12-14  3:59 [PATCH v2 1/4] siphash: add cryptographically secure hashtable function Jason A. Donenfeld
2016-12-14  3:59 ` [PATCH v2 2/4] siphash: add convenience functions for jhash converts Jason A. Donenfeld
2016-12-14  3:59 ` [PATCH v2 3/4] secure_seq: use siphash24 instead of md5_transform Jason A. Donenfeld
2016-12-14 12:53   ` Jason A. Donenfeld
2016-12-14 13:16     ` Hannes Frederic Sowa
2016-12-14 13:44       ` Jason A. Donenfeld
2016-12-14 14:47         ` David Laight
2016-12-14 17:49           ` Jason A. Donenfeld
2016-12-14 17:56     ` David Miller
2016-12-14 18:06       ` Jason A. Donenfeld
2016-12-14 19:22         ` Hannes Frederic Sowa
2016-12-14 19:38           ` Jason A. Donenfeld
2016-12-14 20:27             ` Hannes Frederic Sowa
2016-12-14 20:12     ` Tom Herbert
2016-12-14 21:01       ` Jason A. Donenfeld
2016-12-14  3:59 ` [PATCH v2 4/4] random: use siphash24 instead of md5 for get_random_int/long Jason A. Donenfeld
2016-12-14 11:21 ` [PATCH v2 1/4] siphash: add cryptographically secure hashtable function Hannes Frederic Sowa
2016-12-14 13:10   ` Jason A. Donenfeld
2016-12-14 15:09     ` Hannes Frederic Sowa
2016-12-14 19:47       ` Jason A. Donenfeld
2016-12-15  7:57     ` Herbert Xu
2016-12-15  8:15       ` [kernel-hardening] " Daniel Micay
2016-12-14 12:46 ` Jason A. Donenfeld
2016-12-14 22:03   ` Hannes Frederic Sowa
2016-12-14 23:29     ` Jason A. Donenfeld
2016-12-15  8:31       ` Hannes Frederic Sowa
2016-12-15 11:04     ` David Laight
2016-12-15 12:23       ` Hannes Frederic Sowa
2016-12-15 12:28         ` David Laight
2016-12-15 12:50           ` Hannes Frederic Sowa
2016-12-15 13:56             ` David Laight
2016-12-15 14:56               ` Hannes Frederic Sowa
2016-12-15 15:41                 ` David Laight
2016-12-15 15:53                   ` Hannes Frederic Sowa
2016-12-15 18:50                     ` Jason A. Donenfeld
2016-12-15 20:31                       ` Hannes Frederic Sowa
2016-12-15 20:43                         ` Jason A. Donenfeld
2016-12-15 21:04                           ` Peter Zijlstra
2016-12-15 21:09                             ` Hannes Frederic Sowa
2016-12-15 21:17                           ` Hannes Frederic Sowa [this message]
2016-12-15 21:09                       ` Peter Zijlstra
2016-12-15 21:11                         ` [kernel-hardening] " Jason A. Donenfeld
2016-12-15 21:14                           ` Linus Torvalds
2016-12-14 18:46 ` [PATCH v3 1/3] " Jason A. Donenfeld
2016-12-14 18:46   ` [PATCH v3 2/3] secure_seq: use siphash24 instead of md5_transform Jason A. Donenfeld
2016-12-14 21:44     ` kbuild test robot
2016-12-14 18:46   ` [PATCH v3 3/3] random: use siphash24 instead of md5 for get_random_int/long Jason A. Donenfeld
2016-12-14 21:56     ` kbuild test robot
2016-12-14 21:57     ` kbuild test robot
2016-12-15 10:14     ` David Laight
2016-12-15 18:51       ` Jason A. Donenfeld
2016-12-14 19:18   ` [PATCH v3 1/3] siphash: add cryptographically secure hashtable function Tom Herbert
2016-12-14 19:35     ` Jason A. Donenfeld
2016-12-14 20:55       ` Jason A. Donenfeld
2016-12-14 21:35         ` Tom Herbert
2016-12-14 22:56           ` Jason A. Donenfeld
2016-12-14 23:14             ` Tom Herbert
2016-12-14 23:17               ` Jason A. Donenfeld
2016-12-18  0:06                 ` Christian Kujau
2016-12-14 23:30             ` Linus Torvalds
2016-12-14 23:34               ` Jason A. Donenfeld
2016-12-15  0:10                 ` Linus Torvalds
2016-12-15 10:22                   ` David Laight
2016-12-14 21:15   ` kbuild test robot
2016-12-14 21:21     ` Jason A. Donenfeld
2016-12-15  1:46   ` [PATCH v4 1/4] " Jason A. Donenfeld
2016-12-15  1:46     ` [PATCH v4 2/4] siphash: add N[qd]word helpers Jason A. Donenfeld
2016-12-15  1:46     ` [PATCH v4 3/4] secure_seq: use siphash instead of md5_transform Jason A. Donenfeld
2016-12-15  1:46     ` [PATCH v4 4/4] random: use siphash instead of MD5 for get_random_int/long Jason A. Donenfeld
2016-12-15  4:23     ` [PATCH v4 1/4] siphash: add cryptographically secure hashtable function kbuild test robot
2016-12-15 21:25 [PATCH v2 " Jason A. Donenfeld
2016-12-15 21:45 ` Hannes Frederic Sowa
2016-12-15 23:43   ` Jason A. Donenfeld
2016-12-16  0:03     ` Hannes Frederic Sowa
2016-12-15 23:47   ` Jason A. Donenfeld

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=7861e8ca-69fe-30d1-3d79-27ee33a510d0@stressinduktion.org \
    --to=hannes@stressinduktion.org \
    --cc=David.Laight@aculab.com \
    --cc=Jason@zx2c4.com \
    --cc=djb@cr.yp.to \
    --cc=ebiggers3@gmail.com \
    --cc=jeanphilippe.aumasson@gmail.com \
    --cc=kernel-hardening@lists.openwall.com \
    --cc=linux-crypto@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=netdev@vger.kernel.org \
    --cc=torvalds@linux-foundation.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).