* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Theodore Ts'o @ 2016-12-16 20:43 UTC (permalink / raw)
To: George Spelvin
Cc: Jason, ak, davem, David.Laight, djb, ebiggers3, hannes,
jeanphilippe.aumasson, kernel-hardening, linux-crypto,
linux-kernel, luto, netdev, tom, torvalds, vegard.nossum
In-Reply-To: <20161216201739.24567.qmail@ns.sciencehorizons.net>
On Fri, Dec 16, 2016 at 03:17:39PM -0500, George Spelvin wrote:
> > That's a nice analysis. Might one conclude from that that hsiphash is
> > not useful for our purposes? Or does it still remain useful for
> > network facing code?
>
> I think for attacks where the threat is a DoS, it's usable. The point
> is you only have to raise the cost to equal that of a packet flood.
> (Just like in electronic warfare, the best you can possibly do is force
> the enemy to use broadband jamming.)
>
> Hash collision attacks just aren't that powerful. The original PoC
> was against an application that implemented a hard limit on hash chain
> length as a DoS defense, which the attack then exploited to turn it into
> a hard DoS.
What should we do with get_random_int() and get_random_long()? In
some cases it's being used in performance sensitive areas, and where
anti-DoS protection might be enough. In others, maybe not so much.
If we rekeyed the secret used by get_random_int() and
get_random_long() frequently (say, every minute or every 5 minutes),
would that be sufficient for current and future users of these
interfaces?
- Ted
P.S. I'll note that my performance figures when testing changes to
get_random_int() were done on a 32-bit x86; Jason, I'm guessing your
figures were using a 64-bit x86 system?. I haven't tried 32-bit ARM
or smaller CPU's (e.g., mips, et. al.) that might be more likely to be
used on IoT devices, but I'm worried about those too, of course.
^ permalink raw reply
* Re: Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Daniel Micay @ 2016-12-16 20:44 UTC (permalink / raw)
To: kernel-hardening, Jason A. Donenfeld
Cc: Jean-Philippe Aumasson, George Spelvin, Andi Kleen, David Miller,
David Laight, Eric Biggers, Hannes Frederic Sowa,
Linux Crypto Mailing List, LKML, Andy Lutomirski, Netdev,
Linus Torvalds, Theodore Ts'o, vegard.nossum,
Daniel J . Bernstein
In-Reply-To: <CALx6S351VFRZmEQphRQy6YtmZYPnOtTN7=XiNrJmhWJGv4HUBg@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 465 bytes --]
On Fri, 2016-12-16 at 11:47 -0800, Tom Herbert wrote:
>
> That's about 3x of jhash speed (7 nsecs). So that might closer
> to a more palatable replacement for jhash. Do we lose any security
> advantages with halfsiphash?
Have you tested a lower round SipHash? Probably best to stick with the
usual construction for non-DoS mitigation, but why not try SipHash 1-3,
1-2, etc. for DoS mitigation?
Rust and Swift both went with SipHash 1-3 for hash tables.
[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 866 bytes --]
^ permalink raw reply
* Re: [net-next PATCH v6 0/5] XDP for virtio_net
From: Michael S. Tsirkin @ 2016-12-16 20:48 UTC (permalink / raw)
To: David Miller
Cc: john.fastabend, daniel, netdev, alexei.starovoitov,
john.r.fastabend, brouer, tgraf
In-Reply-To: <20161216.132002.2295377318615583719.davem@davemloft.net>
On Fri, Dec 16, 2016 at 01:20:02PM -0500, David Miller wrote:
> From: "Michael S. Tsirkin" <mst@redhat.com>
> Date: Fri, 16 Dec 2016 01:17:44 +0200
>
> > OK, I think we can queue this for -next.
> >
> > It's fairly limited in the kind of hardware supported, we can and
> > probably should extend it further with time.
> >
> > Acked-by: Michael S. Tsirkin <mst@redhat.com>
>
> Michael, thanks for reviewing.
>
> Since the substance of this patch series has honestly been ready since
> before the merge window, would you mind if I send this to Linus now?
>
> That's what I was hoping I would be able to do.
>
> Thanks again.
ACK, no problem.
BTW in case people wonder, I'll be offline for a couple of weeks.
This might delay review of some patches a bit.
--
MST
^ permalink raw reply
* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Jason A. Donenfeld @ 2016-12-16 20:49 UTC (permalink / raw)
To: George Spelvin
Cc: Andi Kleen, David Miller, David Laight, Daniel J . Bernstein,
Eric Biggers, Hannes Frederic Sowa, Jean-Philippe Aumasson,
kernel-hardening, Linux Crypto Mailing List, LKML,
Andy Lutomirski, Netdev, Tom Herbert, Linus Torvalds,
Theodore Ts'o, Vegard Nossum
On Fri, Dec 16, 2016 at 9:17 PM, George Spelvin
<linux@sciencehorizons.net> wrote:
> My (speaking enerally; I should walk through every hash table you've
> converted) opinion is that:
>
> - Hash tables, even network-facing ones, can all use hsiphash as long
> as an attacker can only see collisions, i.e. ((H(x) ^ H(y)) & bits) ==
> 0, and the consequences of a successful attack is only more collisions
> (timing). While the attack is only 2x the cost (two hashes rather than
> one to test a key), the knowledge of the collision is statistical,
> especially for network attackers, which raises the cost of guessing
> beyond an even more brute-force attack.
> - When the hash value directly visible (e.g. included in a network
> packet), full SipHash should be the default.
> - Syncookies *could* use hsiphash, especially as there are
> two keys in there. Not sure if we need the performance.
> - For TCP ISNs, I'd prefer to use full SipHash. I know this is
> a very hot path, and if that's a performance bottleneck,
> we can work harder on it.
>
> In particular, TCP ISNs *used* to rotate the key periodically,
> limiting the time available to an attacker to perform an
> attack before the secret goes stale and is useless. commit
> 6e5714eaf77d79ae1c8b47e3e040ff5411b717ec upgraded to md5 and dropped
> the key rotation.
While I generally agree with this analysis for the most part, I do
think we should use SipHash and not HalfSipHash for syncookies.
Although the security risk is lower than with sequence numbers, it
previously used full MD5 for this, which means performance is not
generally a bottleneck and we'll get massive speedups no matter what,
whether using SipHash or HalfSipHash. In addition, using SipHash means
that the 128-bit key gives a larger margin and can be safe longterm.
So, I think we should err on the side of caution and stick with
SipHash in all cases in which we're upgrading from MD5.
In other words, only current jhash users should be potentially
eligible for hsiphash.
> Current code uses a 64 ns tick for the ISN, so it counts 2^24 per second.
> (32 bits wraps every 4.6 minutes.) A 4-bit counter and 28-bit hash
> (or even 3+29) would work as long as the key is regenerated no more
> than once per minute. (Just using the 4.6-minute ISN wrap time is the
> obvious simple implementation.)
>
> (Of course, I defer to DaveM's judgement on all network-related issues.)
I saw that jiffies addition in there and was wondering what it was all
about. It's currently added _after_ the siphash input, not before, to
keep with how the old algorithm worked. I'm not sure if this is
correct or if there's something wrong with that, as I haven't studied
how it works. If that jiffies should be part of the siphash input and
not added to the result, please tell me. Otherwise I'll keep things
how they are to avoid breaking something that seems to be working.
^ permalink raw reply
* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Tom Herbert @ 2016-12-16 20:57 UTC (permalink / raw)
To: George Spelvin
Cc: Jason A. Donenfeld, Andi Kleen, David S. Miller, David Laight,
Daniel J . Bernstein, Eric Biggers, Hannes Frederic Sowa,
Jean-Philippe Aumasson, kernel-hardening,
Linux Crypto Mailing List, LKML, Andy Lutomirski,
Linux Kernel Network Developers, Linus Torvalds,
Theodore Ts'o, vegard.nossum
In-Reply-To: <20161216204128.25034.qmail@ns.sciencehorizons.net>
On Fri, Dec 16, 2016 at 12:41 PM, George Spelvin
<linux@sciencehorizons.net> wrote:
> Tom Herbert wrote:
>> Tested this. Distribution and avalanche effect are still good. Speed
>> wise I see about a 33% improvement over siphash (20 nsecs/op versus 32
>> nsecs). That's about 3x of jhash speed (7 nsecs). So that might closer
>> to a more palatable replacement for jhash. Do we lose any security
>> advantages with halfsiphash?
>
> What are you testing on? And what input size? And does "33% improvement"
> mean 4/3 the rate and 3/4 the time? Or 2/3 the time and 3/2 the rate?
>
Sorry, that is over an IPv4 tuple. Intel(R) Xeon(R) CPU E5-2660 0 @
2.20GHz. Recoded the function I was using to look like more like 64
bit version and yes it is indeed slower.
> These are very odd results. On a 64-bit machine, SipHash should be the
> same speed per round, and faster because it hashes more data per round.
> (Unless you're hitting some unexpected cache/decode effect due to REX
> prefixes.)
>
> On a 32-bit machine (other than ARM, where your results might make sense,
> or maybe if you're hashing large amounts of data), the difference should
> be larger.
>
> And yes, there is a *significant* security loss. SipHash is 128 bits
> ("don't worry about it"). hsiphash is 64 bits, which is known breakable
> ("worry about it"), so we have to do a careful analysis of the cost of
> a successful attack.
>
> As mentioned in the e-mails that just flew by, hsiphash is intended
> *only* for 32-bit machines which bog down on full SipHash. On all 64-bit
> machines, it will be implemented as an alias for SipHash and the security
> concerns will Just Go Away.
>
> The place where hsiphash is expected to make a big difference is 32-bit
> x86. If you only see 33% difference with "gcc -m32", I'm going to be
> very confused.
^ permalink raw reply
* Re: Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Jason A. Donenfeld @ 2016-12-16 21:01 UTC (permalink / raw)
To: kernel-hardening, Theodore Ts'o, George Spelvin, Jason,
Andi Kleen, David Miller, David Laight, Daniel J . Bernstein,
Eric Biggers, Hannes Frederic Sowa, Jean-Philippe Aumasson,
Linux Crypto Mailing List, LKML, Andy Lutomirski, Netdev,
Tom Herbert, Linus Torvalds, Vegard Nossum
Hi Ted,
On Fri, Dec 16, 2016 at 9:43 PM, Theodore Ts'o <tytso@mit.edu> wrote:
> What should we do with get_random_int() and get_random_long()? In
> some cases it's being used in performance sensitive areas, and where
> anti-DoS protection might be enough. In others, maybe not so much.
>
> If we rekeyed the secret used by get_random_int() and
> get_random_long() frequently (say, every minute or every 5 minutes),
> would that be sufficient for current and future users of these
> interfaces?
get_random_int() and get_random_long() should quite clearly use
SipHash with its secure 128-bit key and not HalfSipHash with its
64-bit key. HalfSipHash is absolutely insufficient for this use case.
Remember, we're already an order of magnitude or more faster than
md5...
With regard to periodic rekeying... since the secret is 128-bits, I
believe this is likely sufficient for _not_ rekeying. There's also the
chaining variable, to tie together invocations of the function. If
you'd prefer, instead of the chaining variable, we could use some
siphash output to mutate the original key, but I don't think this
approach is actually better and might introduce vulnerabilities. In my
opinion chaining+128bitkey is sufficient. On the other hand, rekeying
every X minutes is 3 or 4 lines of code. If you want (just say so),
I'll add this to my next revision.
You asked about the security requirements of these functions. The
comment says they're not cryptographically secure. And right now with
MD5 they're not. So the expectations are pretty low. Moving to siphash
adds some cryptographic security, certainly. Moving to siphash plus
rekeying adds a bit more. Of course, on recent x86, RDRAND is used
instead, so the cryptographic strength then depends on the thickness
of your tinfoil hat. So probably we shouldn't change what we advertise
these functions provide, even though we're certainly improving them
performance-wise and security-wise.
> P.S. I'll note that my performance figures when testing changes to
> get_random_int() were done on a 32-bit x86; Jason, I'm guessing your
> figures were using a 64-bit x86 system?. I haven't tried 32-bit ARM
> or smaller CPU's (e.g., mips, et. al.) that might be more likely to be
> used on IoT devices, but I'm worried about those too, of course.
Yes, on x86-64. But on i386 chacha20 incurs nearly the same kind of
slowdown as siphash, so I expect the comparison to be more or less
equal. There's another thing I really didn't like about your chacha20
approach which is that it uses the /dev/urandom pool, which means
various things need to kick in in the background to refill this.
Additionally, having to refill the buffered chacha output every 32 or
so longs isn't nice. These things together make for inconsistent and
hard to understand general operating system performance, because
get_random_long is called at every process startup for ASLR. So, in
the end, I believe there's another reason for going with the siphash
approach: deterministic performance.
Jason
^ permalink raw reply
* Re: [kernel-hardening] Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Jason A. Donenfeld @ 2016-12-16 21:09 UTC (permalink / raw)
To: Daniel Micay
Cc: kernel-hardening, Jean-Philippe Aumasson, George Spelvin,
Andi Kleen, David Miller, David Laight, Eric Biggers,
Hannes Frederic Sowa, Linux Crypto Mailing List, LKML,
Andy Lutomirski, Netdev, Linus Torvalds, Theodore Ts'o,
Vegard Nossum, Daniel J . Bernstein
In-Reply-To: <1481921067.1054.6.camel@gmail.com>
Hi Daniel,
On Fri, Dec 16, 2016 at 9:44 PM, Daniel Micay <danielmicay@gmail.com> wrote:
> On Fri, 2016-12-16 at 11:47 -0800, Tom Herbert wrote:
>>
>> That's about 3x of jhash speed (7 nsecs). So that might closer
>> to a more palatable replacement for jhash. Do we lose any security
>> advantages with halfsiphash?
>
> Have you tested a lower round SipHash? Probably best to stick with the
> usual construction for non-DoS mitigation, but why not try SipHash 1-3,
> 1-2, etc. for DoS mitigation?
>
> Rust and Swift both went with SipHash 1-3 for hash tables.
Maybe not a bad idea.
SipHash2-4 for MD5 replacement, as we've done so far. This is when we
actually want things to be secure (and fast).
And then HalfSipHash1-3 for certain jhash replacements. This is for
when we're talking only about DoS or sort of just joking about
security, and want things to be very competitive with jhash. (Of
course for 64-bit we'd use SipHash1-3 instead of HalfSipHash for the
speedup.)
I need to think on this a bit more, but preliminarily, I guess this
would be maybe okay...
George, JP - what do you think?
Jason
^ permalink raw reply
* Re: Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Hannes Frederic Sowa @ 2016-12-16 21:15 UTC (permalink / raw)
To: Jason A. Donenfeld, kernel-hardening, Theodore Ts'o,
George Spelvin, Andi Kleen, David Miller, David Laight,
Daniel J . Bernstein, Eric Biggers, Jean-Philippe Aumasson,
Linux Crypto Mailing List, LKML, Andy Lutomirski, Netdev,
Tom Herbert, Linus Torvalds, Vegard Nossum
In-Reply-To: <CAHmME9pz=syTiLXUsbXFyGdbGK6pxbnU+TVLDkbYiTa-9+sckQ@mail.gmail.com>
On Fri, Dec 16, 2016, at 22:01, Jason A. Donenfeld wrote:
> Yes, on x86-64. But on i386 chacha20 incurs nearly the same kind of
> slowdown as siphash, so I expect the comparison to be more or less
> equal. There's another thing I really didn't like about your chacha20
> approach which is that it uses the /dev/urandom pool, which means
> various things need to kick in in the background to refill this.
> Additionally, having to refill the buffered chacha output every 32 or
> so longs isn't nice. These things together make for inconsistent and
> hard to understand general operating system performance, because
> get_random_long is called at every process startup for ASLR. So, in
> the end, I believe there's another reason for going with the siphash
> approach: deterministic performance.
*Hust*, so from where do you generate your key for siphash if called
early from ASLR?
Bye,
Hannes
^ permalink raw reply
* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: George Spelvin @ 2016-12-16 21:25 UTC (permalink / raw)
To: Jason, linux
Cc: ak, davem, David.Laight, djb, ebiggers3, hannes,
jeanphilippe.aumasson, kernel-hardening, linux-crypto,
linux-kernel, luto, netdev, tom, torvalds, tytso, vegard.nossum
In-Reply-To: <CAHmME9q0LaxQ3uinzWyD1mDCpyeLw_2TEAN33T6dDrTKCuHs7g@mail.gmail.com>
Jason A. Donenfeld wrote:
> I saw that jiffies addition in there and was wondering what it was all
> about. It's currently added _after_ the siphash input, not before, to
> keep with how the old algorithm worked. I'm not sure if this is
> correct or if there's something wrong with that, as I haven't studied
> how it works. If that jiffies should be part of the siphash input and
> not added to the result, please tell me. Otherwise I'll keep things
> how they are to avoid breaking something that seems to be working.
Oh, geez, I didn't realize you didn't understand this code.
Full details at
https://en.wikipedia.org/wiki/TCP_sequence_prediction_attack
But yes, the sequence number is supposed to be (random base) + (timestamp).
In the old days before Canter & Siegel when the internet was a nice place,
people just used a counter that started at boot time.
But then someone observed that I can start a connection to host X,
see the sequence number it gives back to me, and thereby learn the
seauence number it's using on its connections to host Y.
And I can use that to inject forged data into an X-to-Y connection,
without ever seeing a single byte of the traffic! (If I *can* observe
the traffic, of course, none of this makes the slightest difference.)
So the random base was made a keyed hash of the endpoint identifiers.
(Practically only the hosts matter, but generally the ports are thrown
in for good measure.) That way, the ISN that host X sends to me
tells me nothing about the ISN it's using to talk to host Y. Now the
only way to inject forged data into the X-to-Y connection is to
send 2^32 bytes, which is a little less practical.
^ permalink raw reply
* Re: [kernel-hardening] Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Jason A. Donenfeld @ 2016-12-16 21:31 UTC (permalink / raw)
To: kernel-hardening
Cc: George Spelvin, Andi Kleen, David Miller, David Laight,
Daniel J . Bernstein, Eric Biggers, Hannes Frederic Sowa,
Jean-Philippe Aumasson, Linux Crypto Mailing List, LKML,
Andy Lutomirski, Netdev, Tom Herbert, Linus Torvalds,
Theodore Ts'o, Vegard Nossum
In-Reply-To: <20161216212528.26003.qmail@ns.sciencehorizons.net>
Hi George,
On Fri, Dec 16, 2016 at 10:25 PM, George Spelvin
<linux@sciencehorizons.net> wrote:
> But yes, the sequence number is supposed to be (random base) + (timestamp).
> In the old days before Canter & Siegel when the internet was a nice place,
> people just used a counter that started at boot time.
>
> But then someone observed that I can start a connection to host X,
> see the sequence number it gives back to me, and thereby learn the
> seauence number it's using on its connections to host Y.
>
> And I can use that to inject forged data into an X-to-Y connection,
> without ever seeing a single byte of the traffic! (If I *can* observe
> the traffic, of course, none of this makes the slightest difference.)
>
> So the random base was made a keyed hash of the endpoint identifiers.
> (Practically only the hosts matter, but generally the ports are thrown
> in for good measure.) That way, the ISN that host X sends to me
> tells me nothing about the ISN it's using to talk to host Y. Now the
> only way to inject forged data into the X-to-Y connection is to
> send 2^32 bytes, which is a little less practical.
Oh, okay, that is exactly what I thought was going on. I just thought
you were implying that jiffies could be moved inside the hash, which
then confused my understanding of how things should be. In any case,
thanks for the explanation.
Jason
^ permalink raw reply
* Re: [PATCH v6 3/5] random: use SipHash in place of MD5
From: Andy Lutomirski @ 2016-12-16 21:31 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Netdev, kernel-hardening@lists.openwall.com, LKML, linux-crypto,
David Laight, Ted Tso, Hannes Frederic Sowa, Linus Torvalds,
Eric Biggers, Tom Herbert, George Spelvin, Vegard Nossum,
Andi Kleen, David S. Miller, Jean-Philippe Aumasson
In-Reply-To: <20161216030328.11602-4-Jason@zx2c4.com>
On Thu, Dec 15, 2016 at 7:03 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> -static DEFINE_PER_CPU(__u32 [MD5_DIGEST_WORDS], get_random_int_hash)
> - __aligned(sizeof(unsigned long));
> +static DEFINE_PER_CPU(u64, get_random_int_chaining);
>
[...]
> unsigned long get_random_long(void)
> {
> - __u32 *hash;
> unsigned long ret;
> + u64 *chaining;
>
> if (arch_get_random_long(&ret))
> return ret;
>
> - hash = get_cpu_var(get_random_int_hash);
> -
> - hash[0] += current->pid + jiffies + random_get_entropy();
> - md5_transform(hash, random_int_secret);
> - ret = *(unsigned long *)hash;
> - put_cpu_var(get_random_int_hash);
> -
> + chaining = &get_cpu_var(get_random_int_chaining);
> + ret = *chaining = siphash_3u64(*chaining, jiffies, random_get_entropy() +
> + current->pid, random_int_secret);
> + put_cpu_var(get_random_int_chaining);
> return ret;
> }
I think it would be nice to try to strenghen the PRNG construction.
FWIW, I'm not an expert in PRNGs, and there's fairly extensive
literature, but I can at least try. Here are some properties I'd
like:
1. A one-time leak of memory contents doesn't ruin security until
reboot. This is especially value across suspend and/or hibernation.
2. An attack with a low work factor (2^64?) shouldn't break the scheme
until reboot.
This is effectively doing:
output = H(prev_output, weak "entropy", per-boot secret);
One unfortunately downside is that, if used in a context where an
attacker can see a single output, the attacker learns the chaining
value. If the attacker can guess the entropy, then, with 2^64 work,
they learn the secret, and they can predict future outputs.
I would advocate adding two types of improvements. First, re-seed it
every now and then (every 128 calls?) by just replacing both the
chaining value and the percpu secret with fresh CSPRNG output.
Second, change the mode so that an attacker doesn't learn so much
internal state. For example:
output = H(old_chain, entropy, secret);
new_chain = old_chain + entropy + output;
This increases the effort needed to brute-force the internal state
from 2^64 to 2^128 (barring any weaknesses in the scheme).
Also, can we not call this get_random_int()? get_random_int() sounds
too much like get_random_bytes(), and the latter is intended to be a
real CSPRNG. Can we call it get_weak_random_int() or similar?
--Andy
^ permalink raw reply
* [PATCH] isdn: Constify some function parameters
From: Kees Cook @ 2016-12-16 21:40 UTC (permalink / raw)
To: Karsten Keil; +Cc: linux-kernel, Emese Revfy, netdev
From: Emese Revfy <re.emese@gmail.com>
The coming initify gcc plugin expects const pointer types, and caught
some __printf arguments that weren't const yet. This fixes those.
Signed-off-by: Emese Revfy <re.emese@gmail.com>
[kees: expanded commit message]
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/isdn/hisax/config.c | 16 ++++++++--------
drivers/isdn/hisax/hisax.h | 4 ++--
2 files changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/isdn/hisax/config.c b/drivers/isdn/hisax/config.c
index bf04d2a3cf4a..2d12c6ceeb89 100644
--- a/drivers/isdn/hisax/config.c
+++ b/drivers/isdn/hisax/config.c
@@ -659,7 +659,7 @@ int jiftime(char *s, long mark)
static u_char tmpbuf[HISAX_STATUS_BUFSIZE];
-void VHiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt,
+void VHiSax_putstatus(struct IsdnCardState *cs, char *head, const char *fmt,
va_list args)
{
/* if head == NULL the fmt contains the full info */
@@ -669,23 +669,24 @@ void VHiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt,
u_char *p;
isdn_ctrl ic;
int len;
+ const u_char *data;
if (!cs) {
printk(KERN_WARNING "HiSax: No CardStatus for message");
return;
}
spin_lock_irqsave(&cs->statlock, flags);
- p = tmpbuf;
if (head) {
+ p = tmpbuf;
p += jiftime(p, jiffies);
p += sprintf(p, " %s", head);
p += vsprintf(p, fmt, args);
*p++ = '\n';
*p = 0;
len = p - tmpbuf;
- p = tmpbuf;
+ data = tmpbuf;
} else {
- p = fmt;
+ data = fmt;
len = strlen(fmt);
}
if (len > HISAX_STATUS_BUFSIZE) {
@@ -699,13 +700,12 @@ void VHiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt,
if (i >= len)
i = len;
len -= i;
- memcpy(cs->status_write, p, i);
+ memcpy(cs->status_write, data, i);
cs->status_write += i;
if (cs->status_write > cs->status_end)
cs->status_write = cs->status_buf;
- p += i;
if (len) {
- memcpy(cs->status_write, p, len);
+ memcpy(cs->status_write, data + i, len);
cs->status_write += len;
}
#ifdef KERNELSTACK_DEBUG
@@ -729,7 +729,7 @@ void VHiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt,
}
}
-void HiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt, ...)
+void HiSax_putstatus(struct IsdnCardState *cs, char *head, const char *fmt, ...)
{
va_list args;
diff --git a/drivers/isdn/hisax/hisax.h b/drivers/isdn/hisax/hisax.h
index 6ead6314e6d2..338d0408b377 100644
--- a/drivers/isdn/hisax/hisax.h
+++ b/drivers/isdn/hisax/hisax.h
@@ -1288,9 +1288,9 @@ int jiftime(char *s, long mark);
int HiSax_command(isdn_ctrl *ic);
int HiSax_writebuf_skb(int id, int chan, int ack, struct sk_buff *skb);
__printf(3, 4)
-void HiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt, ...);
+void HiSax_putstatus(struct IsdnCardState *cs, char *head, const char *fmt, ...);
__printf(3, 0)
-void VHiSax_putstatus(struct IsdnCardState *cs, char *head, char *fmt, va_list args);
+void VHiSax_putstatus(struct IsdnCardState *cs, char *head, const char *fmt, va_list args);
void HiSax_reportcard(int cardnr, int sel);
int QuickHex(char *txt, u_char *p, int cnt);
void LogFrame(struct IsdnCardState *cs, u_char *p, int size);
--
2.7.4
--
Kees Cook
Nexus Security
^ permalink raw reply related
* Re: [PATCH v6 3/5] random: use SipHash in place of MD5
From: Jason A. Donenfeld @ 2016-12-16 21:45 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Netdev, kernel-hardening@lists.openwall.com, LKML,
Linux Crypto Mailing List, David Laight, Ted Tso,
Hannes Frederic Sowa, Linus Torvalds, Eric Biggers, Tom Herbert,
George Spelvin, Vegard Nossum, Andi Kleen, David S. Miller,
Jean-Philippe Aumasson
Hi Andy,
On Fri, Dec 16, 2016 at 10:31 PM, Andy Lutomirski <luto@amacapital.net> wrote:
> I think it would be nice to try to strenghen the PRNG construction.
> FWIW, I'm not an expert in PRNGs, and there's fairly extensive
> literature, but I can at least try.
In an effort to keep this patchset as initially as uncontroversial as
possible, I kept the same same construction as before and just swapped
out slow MD5 for fast Siphash. Additionally, the function
documentation says that it isn't cryptographically secure. But in the
end I certainly agree with you; we should most definitely improve
things, and seeing the eyeballs now on this series, I think we now
might have a mandate to do so.
> 1. A one-time leak of memory contents doesn't ruin security until
> reboot. This is especially value across suspend and/or hibernation.
Ted and I were discussing this in another thread, and it sounds like
he wants the same thing. I'll add re-generation of the secret every
once in a while. Perhaps time-based makes more sense than
counter-based for rekeying frequency?
> 2. An attack with a low work factor (2^64?) shouldn't break the scheme
> until reboot.
It won't. The key is 128-bits.
> This is effectively doing:
>
> output = H(prev_output, weak "entropy", per-boot secret);
>
> One unfortunately downside is that, if used in a context where an
> attacker can see a single output, the attacker learns the chaining
> value. If the attacker can guess the entropy, then, with 2^64 work,
> they learn the secret, and they can predict future outputs.
No, the secret is 128-bits, which isn't feasibly guessable. The secret
also isn't part of the hash, but rather is the generator of the hash
function. A keyed hash (a PRF) is a bit of a different construction
than just hashing a secret value into a hash function.
> Second, change the mode so that an attacker doesn't learn so much
> internal state. For example:
>
> output = H(old_chain, entropy, secret);
> new_chain = old_chain + entropy + output;
Happy to make this change, with making the chaining value additive
rather than a replacement.
However, I'm not sure adding entropy to the new_chain makes a
different. That entropy is basically just the cycle count plus the
jiffies count. If an attacker can already guess them, then adding them
again to the chaining value doesn't really add much.
Jason
^ permalink raw reply
* Re: [PATCH 1/2] bpf: do not use KMALLOC_SHIFT_MAX
From: Michal Hocko @ 2016-12-16 22:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: linux-mm, Cristopher Lameter, Andrew Morton, Alexei Starovoitov,
netdev, Daniel Borkmann
In-Reply-To: <20161216180209.GA77597@ast-mbp.thefacebook.com>
On Fri 16-12-16 10:02:10, Alexei Starovoitov wrote:
> On Thu, Dec 15, 2016 at 05:47:21PM +0100, Michal Hocko wrote:
> > From: Michal Hocko <mhocko@suse.com>
> >
> > 01b3f52157ff ("bpf: fix allocation warnings in bpf maps and integer
> > overflow") has added checks for the maximum allocateable size. It
> > (ab)used KMALLOC_SHIFT_MAX for that purpose. While this is not incorrect
> > it is not very clean because we already have KMALLOC_MAX_SIZE for this
> > very reason so let's change both checks to use KMALLOC_MAX_SIZE instead.
> >
> > Cc: Alexei Starovoitov <ast@kernel.org>
> > Signed-off-by: Michal Hocko <mhocko@suse.com>
>
> Nack until the patches 1 and 2 are reversed.
I do not insist on ordering. The thing is that it shouldn't matter all
that much. Or are you worried about bisectability?
--
Michal Hocko
SUSE Labs
--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org. For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
^ permalink raw reply
* Re: [TSN RFC v2 0/9] TSN driver for the kernel
From: Richard Cochran @ 2016-12-16 22:05 UTC (permalink / raw)
To: henrik; +Cc: linux-kernel, Henrik Austad, linux-media, alsa-devel, netdev
In-Reply-To: <1481911153-549-1-git-send-email-henrik@austad.us>
On Fri, Dec 16, 2016 at 06:59:04PM +0100, henrik@austad.us wrote:
> The driver is directed via ConfigFS as we need userspace to handle
> stream-reservation (MSRP), discovery and enumeration (IEEE 1722.1) and
> whatever other management is needed.
I complained about configfs before, but you didn't listen.
> 2 new fields in netdev_ops have been introduced, and the Intel
> igb-driver has been updated (as this an AVB-capable NIC which is
> available as a PCI-e card).
The igb hacks show that you are on the wrong track. We can and should
be able to support TSN without resorting to driver specific hacks and
module parameters.
> Before reading on - this is not even beta, but I'd really appreciate if
> people would comment on the overall architecture and perhaps provide
> some pointers to where I should improve/fix/update
As I said before about V1, this architecture stinks. You appear to
have continued hacking along and posted the same design again. Did
you even address any of the points I raised back then?
You are trying to put tons of code into the kernel that really belongs
in user space, and at the same time, you omit critical functions that
only the kernel can provide.
> There are at least one AVB-driver (the AV-part of TSN) in the kernel
> already.
And which driver is that?
Thanks,
Richard
^ permalink raw reply
* Re: Soft lockup in inet_put_port on 4.6
From: Josef Bacik @ 2016-12-16 22:08 UTC (permalink / raw)
To: Hannes Frederic Sowa
Cc: Tom Herbert, Craig Gallek, Eric Dumazet,
Linux Kernel Network Developers
In-Reply-To: <1481901684.24490.7@smtp.office365.com>
On Fri, Dec 16, 2016 at 10:21 AM, Josef Bacik <jbacik@fb.com> wrote:
> On Fri, Dec 16, 2016 at 9:54 AM, Josef Bacik <jbacik@fb.com> wrote:
>> On Thu, Dec 15, 2016 at 7:07 PM, Hannes Frederic Sowa
>> <hannes@stressinduktion.org> wrote:
>>> Hi Josef,
>>>
>>> On 15.12.2016 19:53, Josef Bacik wrote:
>>>> On Tue, Dec 13, 2016 at 6:32 PM, Tom Herbert
>>>> <tom@herbertland.com> wrote:
>>>>> On Tue, Dec 13, 2016 at 3:03 PM, Craig Gallek
>>>>> <kraigatgoog@gmail.com>
>>>>> wrote:
>>>>>> On Tue, Dec 13, 2016 at 3:51 PM, Tom Herbert
>>>>>> <tom@herbertland.com>
>>>>>> wrote:
>>>>>>> I think there may be some suspicious code in
>>>>>>> inet_csk_get_port. At
>>>>>>> tb_found there is:
>>>>>>>
>>>>>>> if (((tb->fastreuse > 0 && reuse) ||
>>>>>>> (tb->fastreuseport > 0 &&
>>>>>>>
>>>>>>> !rcu_access_pointer(sk->sk_reuseport_cb) &&
>>>>>>> sk->sk_reuseport && uid_eq(tb->fastuid,
>>>>>>> uid))) &&
>>>>>>> smallest_size == -1)
>>>>>>> goto success;
>>>>>>> if
>>>>>>> (inet_csk(sk)->icsk_af_ops->bind_conflict(sk,
>>>>>>> tb, true)) {
>>>>>>> if ((reuse ||
>>>>>>> (tb->fastreuseport > 0 &&
>>>>>>> sk->sk_reuseport &&
>>>>>>>
>>>>>>> !rcu_access_pointer(sk->sk_reuseport_cb) &&
>>>>>>> uid_eq(tb->fastuid, uid))) &&
>>>>>>> smallest_size != -1 && --attempts
>>>>>>> >= 0) {
>>>>>>> spin_unlock_bh(&head->lock);
>>>>>>> goto again;
>>>>>>> }
>>>>>>> goto fail_unlock;
>>>>>>> }
>>>>>>>
>>>>>>> AFAICT there is redundancy in these two conditionals. The
>>>>>>> same clause
>>>>>>> is being checked in both: (tb->fastreuseport > 0 &&
>>>>>>> !rcu_access_pointer(sk->sk_reuseport_cb) && sk->sk_reuseport
>>>>>>> &&
>>>>>>> uid_eq(tb->fastuid, uid))) && smallest_size == -1. If this is
>>>>>>> true the
>>>>>>> first conditional should be hit, goto done, and the second
>>>>>>> will never
>>>>>>> evaluate that part to true-- unless the sk is changed (do we
>>>>>>> need
>>>>>>> READ_ONCE for sk->sk_reuseport_cb?).
>>>>>> That's an interesting point... It looks like this function also
>>>>>> changed in 4.6 from using a single local_bh_disable() at the
>>>>>> beginning
>>>>>> with several spin_lock(&head->lock) to exclusively
>>>>>> spin_lock_bh(&head->lock) at each locking point. Perhaps the
>>>>>> full bh
>>>>>> disable variant was preventing the timers in your stack trace
>>>>>> from
>>>>>> running interleaved with this function before?
>>>>>
>>>>> Could be, although dropping the lock shouldn't be able to affect
>>>>> the
>>>>> search state. TBH, I'm a little lost in reading function, the
>>>>> SO_REUSEPORT handling is pretty complicated. For instance,
>>>>> rcu_access_pointer(sk->sk_reuseport_cb) is checked three times
>>>>> in that
>>>>> function and also in every call to inet_csk_bind_conflict. I
>>>>> wonder if
>>>>> we can simply this under the assumption that SO_REUSEPORT is only
>>>>> allowed if the port number (snum) is explicitly specified.
>>>>
>>>> Ok first I have data for you Hannes, here's the time distributions
>>>> before during and after the lockup (with all the debugging in
>>>> place the
>>>> box eventually recovers). I've attached it as a text file since
>>>> it is
>>>> long.
>>>
>>> Thanks a lot!
>>>
>>>> Second is I was thinking about why we would spend so much time
>>>> doing the
>>>> ->owners list, and obviously it's because of the massive amount of
>>>> timewait sockets on the owners list. I wrote the following dumb
>>>> patch
>>>> and tested it and the problem has disappeared completely. Now I
>>>> don't
>>>> know if this is right at all, but I thought it was weird we
>>>> weren't
>>>> copying the soreuseport option from the original socket onto the
>>>> twsk.
>>>> Is there are reason we aren't doing this currently? Does this
>>>> help
>>>> explain what is happening? Thanks,
>>>
>>> The patch is interesting and a good clue, but I am immediately a bit
>>> concerned that we don't copy/tag the socket with the uid also to
>>> keep
>>> the security properties for SO_REUSEPORT. I have to think a bit more
>>> about this.
>>>
>>> We have seen hangs during connect. I am afraid this patch wouldn't
>>> help
>>> there while also guaranteeing uniqueness.
>>
>>
>> Yeah so I looked at the code some more and actually my patch is
>> really bad. If sk2->sk_reuseport is set we'll look at
>> sk2->sk_reuseport_cb, which is outside of the timewait sock, so
>> that's definitely bad.
>>
>> But we should at least be setting it to 0 so that we don't do this
>> normally. Unfortunately simply setting it to 0 doesn't fix the
>> problem. So for some reason having ->sk_reuseport set to 1 on a
>> timewait socket makes this problem non-existent, which is strange.
>>
>> So back to the drawing board I guess. I wonder if doing what craig
>> suggested and batching the timewait timer expires so it hurts less
>> would accomplish the same results. Thanks,
>
> Wait no I lied, we access the sk->sk_reuseport_cb, not sk2's. This
> is the code
>
> if ((!reuse || !sk2->sk_reuse ||
> sk2->sk_state == TCP_LISTEN) &&
> (!reuseport || !sk2->sk_reuseport ||
> rcu_access_pointer(sk->sk_reuseport_cb) ||
> (sk2->sk_state != TCP_TIME_WAIT &&
> !uid_eq(uid, sock_i_uid(sk2))))) {
>
> if (!sk2->sk_rcv_saddr ||
> !sk->sk_rcv_saddr ||
> sk2->sk_rcv_saddr ==
> sk->sk_rcv_saddr)
> break;
> }
>
> so in my patches case we now have reuseport == 1, sk2->sk_reuseport
> == 1. But now we are using reuseport, so sk->sk_reuseport_cb should
> be non-NULL right? So really setting the timewait sock's
> sk_reuseport should have no bearing on how this loop plays out right?
> Thanks,
So more messing around and I noticed that we basically don't do the
tb->fastreuseport logic at all if we've ended up with a non
SO_REUSEPORT socket on that tb. So before I fully understood what I
was doing I fixed it so that after we go through ->bind_conflict() once
with a SO_REUSEPORT socket, we reset tb->fastreuseport to 1 and set the
uid to match the uid of the socket. This made the problem go away.
Tom pointed out that if we bind to the same port on a different address
and we have a non SO_REUSEPORT socket with the same address on this tb
then we'd be screwed with my code.
Which brings me to his proposed solution. We need another hash table
that is indexed based on the binding address. Then each node
corresponds to one address/port binding, with non-SO_REUSEPORT entries
having only one entry, and normal SO_REUSEPORT entries having many.
This cleans up the need to search all the possible sockets on any given
tb, we just go and look at the one we care about. Does this make
sense? Thanks,
Josef
^ permalink raw reply
* Re: [TSN RFC v2 5/9] Add TSN header for the driver
From: Richard Cochran @ 2016-12-16 22:09 UTC (permalink / raw)
To: henrik
Cc: linux-kernel, Henrik Austad, linux-media, alsa-devel, netdev,
David S. Miller
In-Reply-To: <1481911153-549-6-git-send-email-henrik@austad.us>
On Fri, Dec 16, 2016 at 06:59:09PM +0100, henrik@austad.us wrote:
> +/*
> + * List of current subtype fields in the common header of AVTPDU
> + *
> + * Note: AVTPDU is a remnant of the standards from when it was AVB.
> + *
> + * The list has been updated with the recent values from IEEE 1722, draft 16.
> + */
> +enum avtp_subtype {
> + TSN_61883_IIDC = 0, /* IEC 61883/IIDC Format */
> + TSN_MMA_STREAM, /* MMA Streams */
> + TSN_AAF, /* AVTP Audio Format */
> + TSN_CVF, /* Compressed Video Format */
> + TSN_CRF, /* Clock Reference Format */
> + TSN_TSCF, /* Time-Synchronous Control Format */
> + TSN_SVF, /* SDI Video Format */
> + TSN_RVF, /* Raw Video Format */
> + /* 0x08 - 0x6D reserved */
> + TSN_AEF_CONTINOUS = 0x6e, /* AES Encrypted Format Continous */
> + TSN_VSF_STREAM, /* Vendor Specific Format Stream */
> + /* 0x70 - 0x7e reserved */
> + TSN_EF_STREAM = 0x7f, /* Experimental Format Stream */
> + /* 0x80 - 0x81 reserved */
> + TSN_NTSCF = 0x82, /* Non Time-Synchronous Control Format */
> + /* 0x83 - 0xed reserved */
> + TSN_ESCF = 0xec, /* ECC Signed Control Format */
> + TSN_EECF, /* ECC Encrypted Control Format */
> + TSN_AEF_DISCRETE, /* AES Encrypted Format Discrete */
> + /* 0xef - 0xf9 reserved */
> + TSN_ADP = 0xfa, /* AVDECC Discovery Protocol */
> + TSN_AECP, /* AVDECC Enumeration and Control Protocol */
> + TSN_ACMP, /* AVDECC Connection Management Protocol */
> + /* 0xfd reserved */
> + TSN_MAAP = 0xfe, /* MAAP Protocol */
> + TSN_EF_CONTROL, /* Experimental Format Control */
> +};
The kernel shouldn't be in the business of assembling media packets.
Thanks,
Richard
^ permalink raw reply
* Re: [PATCH v6 3/5] random: use SipHash in place of MD5
From: Jason A. Donenfeld @ 2016-12-16 22:12 UTC (permalink / raw)
To: Andy Lutomirski, Ted Tso
Cc: Netdev, kernel-hardening@lists.openwall.com, LKML,
Linux Crypto Mailing List, David Laight, Hannes Frederic Sowa,
Linus Torvalds, Eric Biggers, Tom Herbert, George Spelvin,
Vegard Nossum, Andi Kleen, David S. Miller,
Jean-Philippe Aumasson
In-Reply-To: <CAHmME9rbKi3O1SS89LRMEUeMdKyrdutXAfjb9QmW3KNoCuE-wg@mail.gmail.com>
Hi Andy, Ted,
I've made the requested changes. Keys now rotate and are per-CPU
based. The chaining value is now additive instead of replacing.
DavidL suggested I lower the velocity of `git-send-email` triggers, so
if you'd like to take a look at this before I post v7, you can follow
along at my git tree here:
https://git.zx2c4.com/linux-dev/log/?h=siphash
Choose the commit entitled "random: use SipHash in place of MD5"
Thanks,
Jason
^ permalink raw reply
* Re: [PATCH v6 3/5] random: use SipHash in place of MD5
From: Andy Lutomirski @ 2016-12-16 22:13 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: Netdev, kernel-hardening@lists.openwall.com, LKML,
Linux Crypto Mailing List, David Laight, Ted Tso,
Hannes Frederic Sowa, Linus Torvalds, Eric Biggers, Tom Herbert,
George Spelvin, Vegard Nossum, Andi Kleen, David S. Miller,
Jean-Philippe Aumasson
In-Reply-To: <CAHmME9rbKi3O1SS89LRMEUeMdKyrdutXAfjb9QmW3KNoCuE-wg@mail.gmail.com>
On Fri, Dec 16, 2016 at 1:45 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Hi Andy,
>
> On Fri, Dec 16, 2016 at 10:31 PM, Andy Lutomirski <luto@amacapital.net> wrote:
>> I think it would be nice to try to strenghen the PRNG construction.
>> FWIW, I'm not an expert in PRNGs, and there's fairly extensive
>> literature, but I can at least try.
>
> In an effort to keep this patchset as initially as uncontroversial as
> possible, I kept the same same construction as before and just swapped
> out slow MD5 for fast Siphash. Additionally, the function
> documentation says that it isn't cryptographically secure. But in the
> end I certainly agree with you; we should most definitely improve
> things, and seeing the eyeballs now on this series, I think we now
> might have a mandate to do so.
>
>> 1. A one-time leak of memory contents doesn't ruin security until
>> reboot. This is especially value across suspend and/or hibernation.
>
> Ted and I were discussing this in another thread, and it sounds like
> he wants the same thing. I'll add re-generation of the secret every
> once in a while. Perhaps time-based makes more sense than
> counter-based for rekeying frequency?
Counter may be faster -- you don't need to read a timer. Lots of CPUs
are surprisingly slow at timing. OTOH jiffies are fast.
>
>> 2. An attack with a low work factor (2^64?) shouldn't break the scheme
>> until reboot.
>
> It won't. The key is 128-bits.
Whoops, I thought the key was 64-bit...
>
>> This is effectively doing:
>>
>> output = H(prev_output, weak "entropy", per-boot secret);
>>
>> One unfortunately downside is that, if used in a context where an
>> attacker can see a single output, the attacker learns the chaining
>> value. If the attacker can guess the entropy, then, with 2^64 work,
>> they learn the secret, and they can predict future outputs.
>
> No, the secret is 128-bits, which isn't feasibly guessable. The secret
> also isn't part of the hash, but rather is the generator of the hash
> function. A keyed hash (a PRF) is a bit of a different construction
> than just hashing a secret value into a hash function.
I was thinking in the random oracle model, in which case the whole
function is just a PRF that takes a few input parameters, one of which
is a key.
>
>> Second, change the mode so that an attacker doesn't learn so much
>> internal state. For example:
>>
>> output = H(old_chain, entropy, secret);
>> new_chain = old_chain + entropy + output;
>
> Happy to make this change, with making the chaining value additive
> rather than a replacement.
>
> However, I'm not sure adding entropy to the new_chain makes a
> different. That entropy is basically just the cycle count plus the
> jiffies count. If an attacker can already guess them, then adding them
> again to the chaining value doesn't really add much.
Agreed. A simpler contruction would be:
chaining++;
output = H(chaining, secret);
And this looks a whole lot like Ted's ChaCha20 construction.
The benefit of my construction is that (in the random oracle model,
assuming my intuition is right), if an attacker misses ~128 samples
and entropy has at least one bit of independent min-entropy per
sample, then the attacker needs ~2^128 work to brute-force the
chaining value even fi the attacker knew both the original chaining
value and the secret.
--Andy
^ permalink raw reply
* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: George Spelvin @ 2016-12-16 22:13 UTC (permalink / raw)
To: linux, tytso
Cc: ak, davem, David.Laight, djb, ebiggers3, hannes, Jason,
jeanphilippe.aumasson, kernel-hardening, linux-crypto,
linux-kernel, luto, netdev, tom, torvalds, vegard.nossum
In-Reply-To: <20161216204358.nlwifgcqnu6pitxs@thunk.org>
> What should we do with get_random_int() and get_random_long()? In
> some cases it's being used in performance sensitive areas, and where
> anti-DoS protection might be enough. In others, maybe not so much.
This is tricky. The entire get_random_int() structure is an abuse of
the hash function and will need to be thoroughly rethought to convert
it to SipHash. Remember, SipHash's security goals are very different
from MD5, so there's no obvious way to do the conversion.
(It's *documented* as "not cryptographically secure", but we know
where that goes.)
> If we rekeyed the secret used by get_random_int() and
> get_random_long() frequently (say, every minute or every 5 minutes),
> would that be sufficient for current and future users of these
> interfaces?
Remembering that on "real" machines it's full SipHash, then I'd say that
64-bit security + rekeying seems reasonable.
The question is, the idea has recently been floated to make hsiphash =
SipHash-1-3 on 64-bit machines. Is *that* okay?
The annoying thing about the currently proposed patch is that the *only*
chaining is the returned value. What I'd *like* to do is the same
pattern as we do with md5, and remember v[0..3] between invocations.
But there's no partial SipHash primitive; we only get one word back.
Even
*chaining += ret = siphash_3u64(...)
would be an improvement.
Although we could do something like
c0 = chaining[0];
chaining[0] = c1 = chaining[1];
ret = hsiphash(c0, c1, ...)
chaining[1] = c0 + ret;
^ permalink raw reply
* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Andy Lutomirski @ 2016-12-16 22:15 UTC (permalink / raw)
To: George Spelvin
Cc: Ted Ts'o, Andi Kleen, David S. Miller, David Laight,
D. J. Bernstein, Eric Biggers, Hannes Frederic Sowa,
Jason A. Donenfeld, Jean-Philippe Aumasson,
kernel-hardening@lists.openwall.com, Linux Crypto Mailing List,
linux-kernel@vger.kernel.org, Network Development, Tom Herbert,
Linus Torvalds, Vegard Nossum
In-Reply-To: <20161216221352.26899.qmail@ns.sciencehorizons.net>
On Fri, Dec 16, 2016 at 2:13 PM, George Spelvin
<linux@sciencehorizons.net> wrote:
>> What should we do with get_random_int() and get_random_long()? In
>> some cases it's being used in performance sensitive areas, and where
>> anti-DoS protection might be enough. In others, maybe not so much.
>
> This is tricky. The entire get_random_int() structure is an abuse of
> the hash function and will need to be thoroughly rethought to convert
> it to SipHash. Remember, SipHash's security goals are very different
> from MD5, so there's no obvious way to do the conversion.
>
> (It's *documented* as "not cryptographically secure", but we know
> where that goes.)
>
>> If we rekeyed the secret used by get_random_int() and
>> get_random_long() frequently (say, every minute or every 5 minutes),
>> would that be sufficient for current and future users of these
>> interfaces?
>
> Remembering that on "real" machines it's full SipHash, then I'd say that
> 64-bit security + rekeying seems reasonable.
>
> The question is, the idea has recently been floated to make hsiphash =
> SipHash-1-3 on 64-bit machines. Is *that* okay?
>
>
> The annoying thing about the currently proposed patch is that the *only*
> chaining is the returned value. What I'd *like* to do is the same
> pattern as we do with md5, and remember v[0..3] between invocations.
> But there's no partial SipHash primitive; we only get one word back.
>
> Even
> *chaining += ret = siphash_3u64(...)
>
> would be an improvement.
This is almost exactly what I suggested in my email on the other
thread from a few seconds ago :)
--Andy
^ permalink raw reply
* Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: Jason A. Donenfeld @ 2016-12-16 22:18 UTC (permalink / raw)
To: George Spelvin
Cc: Theodore Ts'o, Andi Kleen, David Miller, David Laight,
Daniel J . Bernstein, Eric Biggers, Hannes Frederic Sowa,
Jean-Philippe Aumasson, kernel-hardening,
Linux Crypto Mailing List, LKML, Andy Lutomirski, Netdev,
Tom Herbert, Linus Torvalds, Vegard Nossum
In-Reply-To: <20161216221352.26899.qmail@ns.sciencehorizons.net>
On Fri, Dec 16, 2016 at 11:13 PM, George Spelvin
<linux@sciencehorizons.net> wrote:
> Remembering that on "real" machines it's full SipHash, then I'd say that
> 64-bit security + rekeying seems reasonable.
64-bit security for an RNG is not reasonable even with rekeying. No no
no. Considering we already have a massive speed-up here with the
secure version, there's zero reason to start weakening the security
because we're trigger happy with our benchmarks. No no no.
^ permalink raw reply
* Re: Soft lockup in inet_put_port on 4.6
From: Tom Herbert @ 2016-12-16 22:18 UTC (permalink / raw)
To: Josef Bacik
Cc: Hannes Frederic Sowa, Craig Gallek, Eric Dumazet,
Linux Kernel Network Developers
In-Reply-To: <1481926135.24490.8@smtp.office365.com>
On Fri, Dec 16, 2016 at 2:08 PM, Josef Bacik <jbacik@fb.com> wrote:
> On Fri, Dec 16, 2016 at 10:21 AM, Josef Bacik <jbacik@fb.com> wrote:
>>
>> On Fri, Dec 16, 2016 at 9:54 AM, Josef Bacik <jbacik@fb.com> wrote:
>>>
>>> On Thu, Dec 15, 2016 at 7:07 PM, Hannes Frederic Sowa
>>> <hannes@stressinduktion.org> wrote:
>>>>
>>>> Hi Josef,
>>>>
>>>> On 15.12.2016 19:53, Josef Bacik wrote:
>>>>>
>>>>> On Tue, Dec 13, 2016 at 6:32 PM, Tom Herbert <tom@herbertland.com>
>>>>> wrote:
>>>>>>
>>>>>> On Tue, Dec 13, 2016 at 3:03 PM, Craig Gallek <kraigatgoog@gmail.com>
>>>>>> wrote:
>>>>>>>
>>>>>>> On Tue, Dec 13, 2016 at 3:51 PM, Tom Herbert <tom@herbertland.com>
>>>>>>> wrote:
>>>>>>>>
>>>>>>>> I think there may be some suspicious code in inet_csk_get_port. At
>>>>>>>> tb_found there is:
>>>>>>>>
>>>>>>>> if (((tb->fastreuse > 0 && reuse) ||
>>>>>>>> (tb->fastreuseport > 0 &&
>>>>>>>> !rcu_access_pointer(sk->sk_reuseport_cb) &&
>>>>>>>> sk->sk_reuseport && uid_eq(tb->fastuid,
>>>>>>>> uid))) &&
>>>>>>>> smallest_size == -1)
>>>>>>>> goto success;
>>>>>>>> if (inet_csk(sk)->icsk_af_ops->bind_conflict(sk,
>>>>>>>> tb, true)) {
>>>>>>>> if ((reuse ||
>>>>>>>> (tb->fastreuseport > 0 &&
>>>>>>>> sk->sk_reuseport &&
>>>>>>>>
>>>>>>>> !rcu_access_pointer(sk->sk_reuseport_cb) &&
>>>>>>>> uid_eq(tb->fastuid, uid))) &&
>>>>>>>> smallest_size != -1 && --attempts >=
>>>>>>>> 0) {
>>>>>>>> spin_unlock_bh(&head->lock);
>>>>>>>> goto again;
>>>>>>>> }
>>>>>>>> goto fail_unlock;
>>>>>>>> }
>>>>>>>>
>>>>>>>> AFAICT there is redundancy in these two conditionals. The same
>>>>>>>> clause
>>>>>>>> is being checked in both: (tb->fastreuseport > 0 &&
>>>>>>>> !rcu_access_pointer(sk->sk_reuseport_cb) && sk->sk_reuseport &&
>>>>>>>> uid_eq(tb->fastuid, uid))) && smallest_size == -1. If this is true
>>>>>>>> the
>>>>>>>> first conditional should be hit, goto done, and the second will
>>>>>>>> never
>>>>>>>> evaluate that part to true-- unless the sk is changed (do we need
>>>>>>>> READ_ONCE for sk->sk_reuseport_cb?).
>>>>>>>
>>>>>>> That's an interesting point... It looks like this function also
>>>>>>> changed in 4.6 from using a single local_bh_disable() at the
>>>>>>> beginning
>>>>>>> with several spin_lock(&head->lock) to exclusively
>>>>>>> spin_lock_bh(&head->lock) at each locking point. Perhaps the full
>>>>>>> bh
>>>>>>> disable variant was preventing the timers in your stack trace from
>>>>>>> running interleaved with this function before?
>>>>>>
>>>>>>
>>>>>> Could be, although dropping the lock shouldn't be able to affect the
>>>>>> search state. TBH, I'm a little lost in reading function, the
>>>>>> SO_REUSEPORT handling is pretty complicated. For instance,
>>>>>> rcu_access_pointer(sk->sk_reuseport_cb) is checked three times in
>>>>>> that
>>>>>> function and also in every call to inet_csk_bind_conflict. I wonder
>>>>>> if
>>>>>> we can simply this under the assumption that SO_REUSEPORT is only
>>>>>> allowed if the port number (snum) is explicitly specified.
>>>>>
>>>>>
>>>>> Ok first I have data for you Hannes, here's the time distributions
>>>>> before during and after the lockup (with all the debugging in place
>>>>> the
>>>>> box eventually recovers). I've attached it as a text file since it is
>>>>> long.
>>>>
>>>>
>>>> Thanks a lot!
>>>>
>>>>> Second is I was thinking about why we would spend so much time doing
>>>>> the
>>>>> ->owners list, and obviously it's because of the massive amount of
>>>>> timewait sockets on the owners list. I wrote the following dumb patch
>>>>> and tested it and the problem has disappeared completely. Now I don't
>>>>> know if this is right at all, but I thought it was weird we weren't
>>>>> copying the soreuseport option from the original socket onto the twsk.
>>>>> Is there are reason we aren't doing this currently? Does this help
>>>>> explain what is happening? Thanks,
>>>>
>>>>
>>>> The patch is interesting and a good clue, but I am immediately a bit
>>>> concerned that we don't copy/tag the socket with the uid also to keep
>>>> the security properties for SO_REUSEPORT. I have to think a bit more
>>>> about this.
>>>>
>>>> We have seen hangs during connect. I am afraid this patch wouldn't help
>>>> there while also guaranteeing uniqueness.
>>>
>>>
>>>
>>> Yeah so I looked at the code some more and actually my patch is really
>>> bad. If sk2->sk_reuseport is set we'll look at sk2->sk_reuseport_cb, which
>>> is outside of the timewait sock, so that's definitely bad.
>>>
>>> But we should at least be setting it to 0 so that we don't do this
>>> normally. Unfortunately simply setting it to 0 doesn't fix the problem. So
>>> for some reason having ->sk_reuseport set to 1 on a timewait socket makes
>>> this problem non-existent, which is strange.
>>>
>>> So back to the drawing board I guess. I wonder if doing what craig
>>> suggested and batching the timewait timer expires so it hurts less would
>>> accomplish the same results. Thanks,
>>
>>
>> Wait no I lied, we access the sk->sk_reuseport_cb, not sk2's. This is the
>> code
>>
>> if ((!reuse || !sk2->sk_reuse ||
>> sk2->sk_state == TCP_LISTEN) &&
>> (!reuseport || !sk2->sk_reuseport ||
>> rcu_access_pointer(sk->sk_reuseport_cb) ||
>> (sk2->sk_state != TCP_TIME_WAIT &&
>> !uid_eq(uid, sock_i_uid(sk2))))) {
>>
>> if (!sk2->sk_rcv_saddr || !sk->sk_rcv_saddr
>> ||
>> sk2->sk_rcv_saddr == sk->sk_rcv_saddr)
>> break;
>> }
>>
>> so in my patches case we now have reuseport == 1, sk2->sk_reuseport == 1.
>> But now we are using reuseport, so sk->sk_reuseport_cb should be non-NULL
>> right? So really setting the timewait sock's sk_reuseport should have no
>> bearing on how this loop plays out right? Thanks,
>
>
>
> So more messing around and I noticed that we basically don't do the
> tb->fastreuseport logic at all if we've ended up with a non SO_REUSEPORT
> socket on that tb. So before I fully understood what I was doing I fixed it
> so that after we go through ->bind_conflict() once with a SO_REUSEPORT
> socket, we reset tb->fastreuseport to 1 and set the uid to match the uid of
> the socket. This made the problem go away. Tom pointed out that if we bind
> to the same port on a different address and we have a non SO_REUSEPORT
> socket with the same address on this tb then we'd be screwed with my code.
>
> Which brings me to his proposed solution. We need another hash table that
> is indexed based on the binding address. Then each node corresponds to one
> address/port binding, with non-SO_REUSEPORT entries having only one entry,
> and normal SO_REUSEPORT entries having many. This cleans up the need to
> search all the possible sockets on any given tb, we just go and look at the
> one we care about. Does this make sense? Thanks,
>
Hi Josef,
Thinking about it some more the hash table won't work because of the
rules of binding different addresses to the same port. What I think we
can do is to change inet_bind_bucket to be structure that contains all
the information used to detect conflicts (reuse*, if, address, uid,
etc.) and a list of sockets that share that exact same information--
for instance all socket in timewait state create through some listener
socket should wind up on single bucket. When we do the bind_conflict
function we only should have to walk this buckets, not the full list
of sockets.
Thoughts on this?
Thanks,
Tom
> Josef
>
^ permalink raw reply
* Re: [PATCH v6 3/5] random: use SipHash in place of MD5
From: Jason A. Donenfeld @ 2016-12-16 22:23 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Netdev, kernel-hardening@lists.openwall.com, LKML,
Linux Crypto Mailing List, David Laight, Ted Tso,
Hannes Frederic Sowa, Linus Torvalds, Eric Biggers, Tom Herbert,
George Spelvin, Vegard Nossum, Andi Kleen, David S. Miller,
Jean-Philippe Aumasson
In-Reply-To: <CALCETrX9v=Uwd1zZub=QpD73Lq0LM67NEi1qwqRUjtD5U1bHYw@mail.gmail.com>
Hi Andy,
> Agreed. A simpler contruction would be:
>
> chaining++;
> output = H(chaining, secret);
>
> And this looks a whole lot like Ted's ChaCha20 construction.
In that simpler construction with counter-based secret rekeying and in
Ted's ChaCha20 construction, the issue is that every X hits, there's a
call to get_random_bytes, which has variable performance and entropy
issues. Doing it my way with it being time based, in the event that
somebody runs ` :(){ :|:& };:`, system performance doesn't suffer
because ASLR is making repeated calls to get_random_bytes every 128 or
so process creations. In the time based way, the system performance
will not suffer.
Jason
^ permalink raw reply
* Re: [kernel-hardening] Re: [PATCH v5 1/4] siphash: add cryptographically secure PRF
From: George Spelvin @ 2016-12-16 22:41 UTC (permalink / raw)
To: Jason, kernel-hardening
Cc: ak, davem, David.Laight, djb, ebiggers3, hannes,
jeanphilippe.aumasson, linux-crypto, linux-kernel, linux, luto,
netdev, tom, torvalds, tytso, vegard.nossum
In-Reply-To: <CAHmME9qPx3WUHF3__3wNOXr-AUti4WPO1qDiFus3Zr133FyV1g@mail.gmail.com>
An idea I had which mght be useful:
You could perhaps save two rounds in siphash_*u64.
The final word with the length (called "b" in your implementation)
only needs to be there if the input is variable-sized.
If every use of a given key is of a fixed-size input, you don't need
a length suffix. When the input is an even number of words, that can
save you two rounds.
This requires an audit of callers (e.g. you have to use different
keys for IPv4 and IPv6 ISNs), but can save time.
(This is crypto 101; search "MD-strengthening" or see the remark on
p. 101 on Damgaard's 1989 paper "A design principle for hash functions" at
http://saluc.engr.uconn.edu/refs/algorithms/hashalg/damgard89adesign.pdf
but I'm sure that Ted, Jean-Philippe, and/or DJB will confirm if you'd
like.)
Jason A. Donenfeld wrote:
> Oh, okay, that is exactly what I thought was going on. I just thought
> you were implying that jiffies could be moved inside the hash, which
> then confused my understanding of how things should be. In any case,
> thanks for the explanation.
No, the rekeying procedure is cleverer.
The thing is, all that matters is that the ISN increments fast enough,
but not wrap too soon.
It *is* permitted to change the random base, as long as it only
increases, and slower than the timestamp does.
So what you do is every few minutes, you increment the high 4 bits of the
random base and change the key used to generate the low 28 bits.
The base used for any particular host might change from 0x10000000
to 0x2fffffff, or from 0x1fffffff to 0x20000000, but either way, it's
increasing, and not too fast.
This has the downside that an attacker can see 4 bits of the base,
so only needs to send send 2^28 = 256 MB to flood the connection,
but the upside that the key used to generate the low bits changes
faster than it can be broken.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox