* [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
@ 2021-07-06 13:27 Mathias Krause
2021-07-06 13:27 ` [PATCH 1/2] compat: better grsecurity compatibility Mathias Krause
` (2 more replies)
0 siblings, 3 replies; 35+ messages in thread
From: Mathias Krause @ 2021-07-06 13:27 UTC (permalink / raw)
To: Jason A . Donenfeld; +Cc: wireguard, Mathias Krause
Hi Jason,
this mini series enhances compatibility of the wireguard-linux-compat
repo with grsecurity. I noticed some build breakage on our 4.14 kernel
which the following patches take care of.
Please apply!
Thanks,
Mathias
Mathias Krause (2):
compat: better grsecurity compatibility
curve25519-x86_64: solve register constraints with reserved registers
src/compat/compat-asm.h | 4 ++--
src/compat/compat.h | 4 ++--
src/crypto/zinc/curve25519/curve25519-x86_64.c | 4 ++--
3 files changed, 6 insertions(+), 6 deletions(-)
--
2.20.1
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 1/2] compat: better grsecurity compatibility
2021-07-06 13:27 [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Mathias Krause
@ 2021-07-06 13:27 ` Mathias Krause
2021-07-06 13:27 ` [PATCH 2/2] curve25519-x86_64: solve register constraints with reserved registers Mathias Krause
2021-08-08 20:53 ` [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Jason A. Donenfeld
2 siblings, 0 replies; 35+ messages in thread
From: Mathias Krause @ 2021-07-06 13:27 UTC (permalink / raw)
To: Jason A . Donenfeld; +Cc: wireguard, Mathias Krause
grsecurity kernels tend to carry additional backports and changes, like
commit b60b87fc2996 ("netlink: add ethernet address policy types") or
the SYM_FUNC_* changes. RAP nowadays hooks the latter, therefore no
diversion to RAP_ENTRY is needed any more.
Instead of relying on the kernel version test, also test for the macros
we're about to define to not already be defined to account for these
additional changes in the grsecurity patch without breaking
compatibility to the older public ones.
Also test for CONFIG_PAX instead of RAP_PLUGIN for the timer API related
changes as these don't depend on the RAP plugin to be enabled but just a
PaX/grsecurity patch to be applied. While there is no preprocessor knob
for the latter, use CONFIG_PAX as this will likely be enabled in every
kernel that uses the patch.
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
src/compat/compat-asm.h | 4 ++--
src/compat/compat.h | 4 ++--
2 files changed, 4 insertions(+), 4 deletions(-)
diff --git a/src/compat/compat-asm.h b/src/compat/compat-asm.h
index fde21dabba4f..5bfdb9410933 100644
--- a/src/compat/compat-asm.h
+++ b/src/compat/compat-asm.h
@@ -22,7 +22,7 @@
#endif
/* PaX compatibility */
-#if defined(RAP_PLUGIN)
+#if defined(RAP_PLUGIN) && defined(RAP_ENTRY)
#undef ENTRY
#define ENTRY RAP_ENTRY
#endif
@@ -51,7 +51,7 @@
#undef pull
#endif
-#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 4, 76) && !defined(ISCENTOS8S)
+#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 4, 76) && !defined(ISCENTOS8S) && !defined(SYM_FUNC_START)
#define SYM_FUNC_START ENTRY
#define SYM_FUNC_END ENDPROC
#endif
diff --git a/src/compat/compat.h b/src/compat/compat.h
index b2041327d85c..da6912d871fa 100644
--- a/src/compat/compat.h
+++ b/src/compat/compat.h
@@ -830,7 +830,7 @@ static inline void skb_mark_not_on_list(struct sk_buff *skb)
}
#endif
-#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 20, 0) && !defined(ISRHEL8)
+#if LINUX_VERSION_CODE < KERNEL_VERSION(4, 20, 0) && !defined(ISRHEL8) && !defined(NLA_POLICY_EXACT_LEN)
#define NLA_POLICY_EXACT_LEN(_len) { .type = NLA_UNSPEC, .len = _len }
#endif
#if LINUX_VERSION_CODE < KERNEL_VERSION(5, 2, 0) && !defined(ISRHEL8)
@@ -1127,7 +1127,7 @@ static const struct header_ops ip_tunnel_header_ops = { .parse_protocol = ip_tun
#undef __read_mostly
#define __read_mostly
#endif
-#if (defined(RAP_PLUGIN) || defined(CONFIG_CFI_CLANG)) && LINUX_VERSION_CODE < KERNEL_VERSION(4, 15, 0)
+#if (defined(CONFIG_PAX) || defined(CONFIG_CFI_CLANG)) && LINUX_VERSION_CODE < KERNEL_VERSION(4, 15, 0)
#include <linux/timer.h>
#define wg_expired_retransmit_handshake(a) wg_expired_retransmit_handshake(unsigned long timer)
#define wg_expired_send_keepalive(a) wg_expired_send_keepalive(unsigned long timer)
--
2.20.1
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH 2/2] curve25519-x86_64: solve register constraints with reserved registers
2021-07-06 13:27 [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Mathias Krause
2021-07-06 13:27 ` [PATCH 1/2] compat: better grsecurity compatibility Mathias Krause
@ 2021-07-06 13:27 ` Mathias Krause
2021-08-08 20:53 ` [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Jason A. Donenfeld
2 siblings, 0 replies; 35+ messages in thread
From: Mathias Krause @ 2021-07-06 13:27 UTC (permalink / raw)
To: Jason A . Donenfeld; +Cc: wireguard, Mathias Krause
The register constraints for the inline assembly in fsqr() and fsqr2()
are pretty tight on what the compiler may assign to the remaining three
register variables. The clobber list only allows the following to be
used: RDI, RSI, RBP and R12. With RAP reserving R12 and a kernel having
CONFIG_FRAME_POINTER=y, claiming RBP, there are only two registers left
so the compiler rightfully complains about impossible constraints.
Provide alternatives that'll allow a memory reference for 'out' to solve
the allocation constraint dilemma for this configuration.
Signed-off-by: Mathias Krause <minipli@grsecurity.net>
---
Yes, the '+' constraint prefix doesn't need to be repeated for the
alternatives. In fact, it's invalid syntax to do so (see [1]). Also
"+rm" won't do the trick either, as in this case gcc still insists to
have a free register -- even if it would choose a memory operand in the
end.
[1] https://gcc.gnu.org/onlinedocs/gcc/Multi-Alternative.html#Multi-Alternative
---
src/crypto/zinc/curve25519/curve25519-x86_64.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/src/crypto/zinc/curve25519/curve25519-x86_64.c b/src/crypto/zinc/curve25519/curve25519-x86_64.c
index 79716c425b0c..67f55affcf88 100644
--- a/src/crypto/zinc/curve25519/curve25519-x86_64.c
+++ b/src/crypto/zinc/curve25519/curve25519-x86_64.c
@@ -581,7 +581,7 @@ static inline void fsqr(u64 *out, const u64 *f, u64 *tmp)
" cmovc %%rdx, %%rax;"
" add %%rax, %%r8;"
" movq %%r8, 0(%0);"
- : "+&r" (tmp), "+&r" (f), "+&r" (out)
+ : "+&r,&r" (tmp), "+&r,&r" (f), "+&r,m" (out)
:
: "%rax", "%rcx", "%rdx", "%r8", "%r9", "%r10", "%r11", "%rbx", "%r13", "%r14", "%r15", "memory", "cc"
);
@@ -743,7 +743,7 @@ static inline void fsqr2(u64 *out, const u64 *f, u64 *tmp)
" cmovc %%rdx, %%rax;"
" add %%rax, %%r8;"
" movq %%r8, 32(%0);"
- : "+&r" (tmp), "+&r" (f), "+&r" (out)
+ : "+&r,&r" (tmp), "+&r,&r" (f), "+&r,m" (out)
:
: "%rax", "%rcx", "%rdx", "%r8", "%r9", "%r10", "%r11", "%rbx", "%r13", "%r14", "%r15", "memory", "cc"
);
--
2.20.1
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-07-06 13:27 [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Mathias Krause
2021-07-06 13:27 ` [PATCH 1/2] compat: better grsecurity compatibility Mathias Krause
2021-07-06 13:27 ` [PATCH 2/2] curve25519-x86_64: solve register constraints with reserved registers Mathias Krause
@ 2021-08-08 20:53 ` Jason A. Donenfeld
2021-08-09 10:13 ` Mathias Krause
2 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-08-08 20:53 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
Hi Mathias,
Sorry for the delay in reviewing these. Thanks for that. I've merged
them with a trivial change. The constraint one is interesting; it
looks like with your change and those extra constraints it winds up
spilling rdi to the stack?
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-08-08 20:53 ` [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Jason A. Donenfeld
@ 2021-08-09 10:13 ` Mathias Krause
2021-12-03 22:20 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-08-09 10:13 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Hi Jason,
Am 08.08.21 um 22:53 schrieb Jason A. Donenfeld:
> Hi Mathias,
>
> Sorry for the delay in reviewing these. Thanks for that. I've merged
> them with a trivial change.
thanks!
> The constraint one is interesting; it
> looks like with your change and those extra constraints it winds up
> spilling rdi to the stack?
Indeed, looking, for example, at fsquare_times() (!RAP, FRAMEPOINTER=y),
it looks like gcc prefers to choose the constraint with a memory output
argument even if it could fulfill the register allocation request, i.e.
it could allocate (all remaining) 3 registers instead of only 2 but
chooses to do the latter nonetheless. However, for me it's %rsi that's
put on the stack. Also it's not spilled, but used as a variable -- the
stack frame increases by 8 bytes and the stack slot where %rsi is
written to is used as-is for operations in the end. But, again, this
differs from the original code that used to allocate three registers for
the inline assembly. The old code has two memory loads at the end and
one compare while the new one has only one -- directly encode into the
compare instruction. So, memory-wise, a net win?
Below is the diff of the disassembly of fsquare_times(), spare of
addresses to reduce clutter with comments below each hunk:
--- old.dis 2021-08-09 11:46:53.050680456 +0200
+++ new.dis 2021-08-09 11:48:18.816851059 +0200
@@ -6,10 +6,10 @@
push %r13
push %r12
push %rbx
- sub $0x18,%rsp
- mov %rdi,-0x38(%rbp)
- mov %rdx,-0x40(%rbp)
- mov %ecx,-0x2c(%rbp)
+ sub $0x20,%rsp
+ mov %rdi,-0x40(%rbp)
+ mov %rdx,-0x48(%rbp)
+ mov %ecx,-0x30(%rbp)
mov %rdx,%r12
mov (%rsi),%rdx
mulx 0x8(%rsi),%r8,%r14
This first hunk is really only the increased stack frame and resulting
offset changes.
@@ -92,15 +92,14 @@
cmovb %rdx,%rax
add %rax,%r8
mov %r8,(%r12)
- mov -0x2c(%rbp),%eax
+ mov -0x30(%rbp),%eax
sub $0x1,%eax
- mov %eax,-0x30(%rbp)
- je 1f42 <fsquare_times+0x3a2>
+ mov %eax,-0x34(%rbp)
+ je 1f3c <fsquare_times+0x39c>
xor %r12d,%r12d
- mov %r12d,-0x2c(%rbp) <-- no longer needed in new code
- mov -0x38(%rbp),%rsi
- mov -0x40(%rbp),%rdi
- mov %rsi,%r12 <-- see below
+ mov -0x40(%rbp),%rsi
+ mov -0x48(%rbp),%rdi
+ mov %rsi,-0x30(%rbp) <-- reg -> mem operand constraint
mov (%rsi),%rdx
mulx 0x8(%rsi),%r8,%r14
xor %r15,%r15
Offset changes again. Beside the one dropped instruction there's only
one real change ("mov %rsi,%r12" -> "mov %rsi,-0x30(%rbp)"), induced by
the memory constraint of the inline assembly, switching a register
operand to a memory location.
@@ -154,7 +153,7 @@
adcx %rcx,%r14
mov %r14,0x38(%rdi)
mov %rdi,%rsi
- mov %r12,%rdi
+ mov -0x30(%rbp),%rdi
mov $0x26,%rdx
mulx 0x20(%rsi),%r8,%r13
xor %rcx,%rcx
Load of the stack slot which used to be a register operation before.
@@ -182,12 +181,10 @@
cmovb %rdx,%rax
add %rax,%r8
mov %r8,(%rdi)
- addl $0x1,-0x2c(%rbp) <-- mem operation
- mov -0x30(%rbp),%ebx <-- mem load
- mov -0x2c(%rbp),%eax <-- mem load
- cmp %ebx,%eax <-- reg compare
- jne 1d83 <fsquare_times+0x1e3>
- add $0x18,%rsp
+ add $0x1,%r12d <-- reg operation
+ cmp -0x34(%rbp),%r12d <-- mem compare
+ jne 1d7f <fsquare_times+0x1df>
+ add $0x20,%rsp
pop %rbx
pop %r12
pop %r13
We switch one memory operation (addl) to a register one and spare the
memory loads for the compare by directly encoding one of the memory
operands into the compare instruction. IMHO a net win, as gcc now can
use a register for the loop condition variable, making the conditional
jump depend only on one memory operand, not two. But only a benchmark
can tell if it's really faster or even slower.
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-08-09 10:13 ` Mathias Krause
@ 2021-12-03 22:20 ` Jason A. Donenfeld
2021-12-03 22:25 ` Jason A. Donenfeld
2021-12-06 14:04 ` Mathias Krause
0 siblings, 2 replies; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-03 22:20 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
Hey Mathias,
This resulted in kind of an interesting regression with old compilers
on old kernel versions when I backported this to
wireguard-linux-compat:
https://git.zx2c4.com/wireguard-linux-compat/commit/?id=8118c247a75ae95169f0a9a539dfc661ffda8bc5
The 25519 tests fail for 4.8.17, 4.7.10, 4.6.7, 4.5.7 with gcc 6:
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.8.17-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.7.10-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.6.7-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.5.7-x86_64.log
But then they crash for 4.0.9, 3.19.8, 3.17.8 with gcc 5:
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.0.9-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.19.8-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.17.8-x86_64.log
And also crash with 3.16.85, 3.15.10, 3.14.79, 3.12.74, 3.11.10 with gcc 4:
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.16.85-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.15.10-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.14.79-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.13.11-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.12.74-x86_64.log
https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.11.10-x86_64.log
Any intuition about what might have happened?
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-03 22:20 ` Jason A. Donenfeld
@ 2021-12-03 22:25 ` Jason A. Donenfeld
2021-12-06 14:04 ` Mathias Krause
1 sibling, 0 replies; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-03 22:25 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
On Fri, Dec 3, 2021 at 11:20 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> on old kernel versions when I backported this to
Er, nix the "when I backported this" part: you sent it originally for
compat and never for mainline.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-03 22:20 ` Jason A. Donenfeld
2021-12-03 22:25 ` Jason A. Donenfeld
@ 2021-12-06 14:04 ` Mathias Krause
2021-12-06 14:48 ` Jason A. Donenfeld
1 sibling, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-06 14:04 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Hi Jason,
Am 03.12.21 um 23:20 schrieb Jason A. Donenfeld:
> This resulted in kind of an interesting regression with old compilers
> on old kernel versions when I backported this to
> wireguard-linux-compat:
> https://git.zx2c4.com/wireguard-linux-compat/commit/?id=8118c247a75ae95169f0a9a539dfc661ffda8bc5
>
> The 25519 tests fail for 4.8.17, 4.7.10, 4.6.7, 4.5.7 with gcc 6:
>
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.8.17-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.7.10-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.6.7-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.5.7-x86_64.log
>
> But then they crash for 4.0.9, 3.19.8, 3.17.8 with gcc 5:
>
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/4.0.9-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.19.8-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.17.8-x86_64.log
>
> And also crash with 3.16.85, 3.15.10, 3.14.79, 3.12.74, 3.11.10 with gcc 4:
>
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.16.85-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.15.10-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.14.79-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.13.11-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.12.74-x86_64.log
> https://build.wireguard.com/wireguard-linux-compat/e8db181d62467da6c476cf4ac21e13dd477612c8/3.11.10-x86_64.log
>
> Any intuition about what might have happened?
Sorry to hear that. I didn't ran into such issues when doing the
backport and, in fact, trying to reproduce the selftest errors / crashes
failed so far on v4.8.17 with gcc 6.3 and 4.6.3:
[ 0.137871] wireguard: chacha20 self-tests: pass
[ 0.141106] wireguard: poly1305 self-tests: pass
[ 0.141604] wireguard: chacha20poly1305 self-tests: pass
[ 0.142309] wireguard: blake2s self-tests: pass
[ 0.157012] wireguard: curve25519 self-tests: pass
[ 0.157430] wireguard: allowedips self-tests: pass
[ 0.158354] wireguard: nonce counter self-tests: pass
[ 0.388426] wireguard: ratelimiter self-tests: pass
[ 0.389045] wireguard: WireGuard 1.0.20210606 loaded. See
www.wireguard.com for information.
[ 0.389874] wireguard: Copyright (C) 2015-2019 Jason A. Donenfeld
<Jason@zx2c4.com>. All Rights Reserved.
I'll try older kernels and see if they trigger. In case not, can you
send me the object files of a failing kernel?
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 14:04 ` Mathias Krause
@ 2021-12-06 14:48 ` Jason A. Donenfeld
2021-12-06 16:24 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-06 14:48 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
Hey Mathias,
I couldn't repro with a new kernel either. Only these old ones.
Here are some object files for the logs in the earlier message:
https://data.zx2c4.com/curve25519-miscompile-a87440f8-7d1e-4179-be83-c82b4636a448.tar.zst
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 14:48 ` Jason A. Donenfeld
@ 2021-12-06 16:24 ` Mathias Krause
2021-12-06 16:27 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-06 16:24 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Am 06.12.21 um 15:48 schrieb Jason A. Donenfeld:
> I couldn't repro with a new kernel either. Only these old ones.
>
> Here are some object files for the logs in the earlier message:
> https://data.zx2c4.com/curve25519-miscompile-a87440f8-7d1e-4179-be83-c82b4636a448.tar.zst
Just a heads-up, some of these seem to have been compiled with a recent
gcc which might not be what was intended?:
$ strings -fa curve25519-*.o | grep -i gcc
curve25519-3.11.10.o: GCC: (GNU) 4.9.4
curve25519-3.12.74.o: GCC: (Gentoo 11.2.0 p1) 11.2.0
curve25519-3.14.79.o: GCC: (Gentoo 11.2.0 p1) 11.2.0
curve25519-3.15.10.o: GCC: (GNU) 4.9.4
curve25519-3.16.85.o: GCC: (Gentoo 11.2.0 p1) 11.2.0
curve25519-3.17.8.o: GCC: (GNU) 5.5.0
curve25519-3.19.8.o: GCC: (GNU) 5.5.0
curve25519-4.0.9.o: GCC: (GNU) 5.5.0
curve25519-4.5.7.o: GCC: (GNU) 6.5.0
curve25519-4.6.7.o: GCC: (GNU) 6.5.0
curve25519-4.7.10.o: GCC: (GNU) 6.5.0
curve25519-4.8.17.o: GCC: (GNU) 6.5.0
Anyhow, now I'm able to reproduce it myself. Was a PEBCAK config failure.
I'll look into it and work on a fix!
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 16:24 ` Mathias Krause
@ 2021-12-06 16:27 ` Jason A. Donenfeld
2021-12-06 18:18 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-06 16:27 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
Hi Mathias,
Oh, you're right about recent gcc. That actually _is_ intended, yet
they still fail. It would seem, then, that the problem is not so much
gcc version as it is some kernel patch that never made it to these
ancient kernels.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 16:27 ` Jason A. Donenfeld
@ 2021-12-06 18:18 ` Mathias Krause
2021-12-06 18:55 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-06 18:18 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Hi Jason,
Am 06.12.21 um 17:27 schrieb Jason A. Donenfeld:
> Oh, you're right about recent gcc. That actually _is_ intended, yet
> they still fail. It would seem, then, that the problem is not so much
> gcc version as it is some kernel patch that never made it to these
> ancient kernels.
actually, it's the i/o constraints, they're wrong. 'out' is an input
operand but we specify it as an output one. Now this works when gcc
respects the "+" constraint, as in marking this operand as being read
and written, thereby implicitly requiring it to be initialized. But
looks like older gcc ignore that (at least when using alternatives) and
make the asm work on a stale 'out' operand, resulting in the selftest
failures and crashes you've seen.
The following change fixes it by putting 'out' to the input operand
list, where it really belongs to:
diff --git a/src/crypto/zinc/curve25519/curve25519-x86_64.c
b/src/crypto/zinc/curve25519/curve25519-x86_64.c
index 67f55affcf88..f26ed5d897ac 100644
--- a/src/crypto/zinc/curve25519/curve25519-x86_64.c
+++ b/src/crypto/zinc/curve25519/curve25519-x86_64.c
@@ -581,8 +581,8 @@ static inline void fsqr(u64 *out, const u64 *f, u64
*tmp)
" cmovc %%rdx, %%rax;"
" add %%rax, %%r8;"
" movq %%r8, 0(%0);"
- : "+&r,&r" (tmp), "+&r,&r" (f), "+&r,m" (out)
- :
+ : "+&r,&r" (tmp), "+&r,&r" (f)
+ : "r,m" (out)
: "%rax", "%rcx", "%rdx", "%r8", "%r9", "%r10", "%r11", "%rbx",
"%r13", "%r14", "%r15", "memory", "cc"
);
}
@@ -743,8 +743,8 @@ static inline void fsqr2(u64 *out, const u64 *f, u64
*tmp)
" cmovc %%rdx, %%rax;"
" add %%rax, %%r8;"
" movq %%r8, 32(%0);"
- : "+&r,&r" (tmp), "+&r,&r" (f), "+&r,m" (out)
- :
+ : "+&r,&r" (tmp), "+&r,&r" (f)
+ : "r,m" (out)
: "%rax", "%rcx", "%rdx", "%r8", "%r9", "%r10", "%r11", "%rbx",
"%r13", "%r14", "%r15", "memory", "cc"
);
}
We still need the early clobber constraint ("&") for 'tmp' and 'f' as
they are, in fact, written to early. But 'out' is only ever read, so can
be a normal input operand.
I'll create a proper patch and send it out tomorrow, if you don't beat
me to.
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 18:18 ` Mathias Krause
@ 2021-12-06 18:55 ` Jason A. Donenfeld
2021-12-06 19:28 ` Jason A. Donenfeld
2021-12-06 21:00 ` Mathias Krause
0 siblings, 2 replies; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-06 18:55 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
Hi Mathias,
Nice detective work! I just loaded this up on the CI, so we'll see if
this does work across the board.
It sounds like the original code also had this bug -- the "r"(out)
should be in the output constraints, not in the input constraints,
right? Sounds like something I should report to the EverCrypt authors
and also fix upstream too then.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 18:55 ` Jason A. Donenfeld
@ 2021-12-06 19:28 ` Jason A. Donenfeld
2021-12-06 20:54 ` Mathias Krause
2021-12-06 21:00 ` Mathias Krause
1 sibling, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-06 19:28 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
On Mon, Dec 6, 2021 at 7:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> Nice detective work! I just loaded this up on the CI, so we'll see if
> this does work across the board.
Looks like https://git.zx2c4.com/wireguard-linux-compat/commit/?id=42c931dbccf9570f10a84e282daf79f385d51623
is all green on https://www.wireguard.com/build-status/ in the
wireguard-linux-compat category. Let me know if that commit looks okay
to you or if you want to adjust something about it. After that, I'll
cut a new compat snapshot release.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 19:28 ` Jason A. Donenfeld
@ 2021-12-06 20:54 ` Mathias Krause
2021-12-08 14:56 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-06 20:54 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Hi Jason,
Am 06.12.21 um 20:28 schrieb Jason A. Donenfeld:
> On Mon, Dec 6, 2021 at 7:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>> Nice detective work! I just loaded this up on the CI, so we'll see if
>> this does work across the board.
>
> Looks like https://git.zx2c4.com/wireguard-linux-compat/commit/?id=42c931dbccf9570f10a84e282daf79f385d51623
> is all green on https://www.wireguard.com/build-status/ in the
> wireguard-linux-compat category. Let me know if that commit looks okay
> to you or if you want to adjust something about it. After that, I'll
> cut a new compat snapshot release.
ah, you modified the original commit of mine. Yeah, that works too.
However, I'd add the following to the commit log to account for the
output to input operand move:
"""
Also make 'out' an input-only operand as it is only used as such. This
not only allows gcc to optimize its usage further, but also works around
older gcc versions, apparently failing to handle multiple alternatives
correctly, as in failing to initialize the 'out' operand with its input
value.
"""
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 18:55 ` Jason A. Donenfeld
2021-12-06 19:28 ` Jason A. Donenfeld
@ 2021-12-06 21:00 ` Mathias Krause
2021-12-08 14:56 ` Jason A. Donenfeld
1 sibling, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-06 21:00 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Am 06.12.21 um 19:55 schrieb Jason A. Donenfeld:
> Nice detective work! I just loaded this up on the CI, so we'll see if
> this does work across the board.
Thanks.
> It sounds like the original code also had this bug -- the "r"(out)
> should be in the output constraints, not in the input constraints,
> right? Sounds like something I should report to the EverCrypt authors
> and also fix upstream too then.
Yes, probably, but you're mixing up the two. "r"(out) should be an input
operand as it's only read in the inline asm. The other two ('tmp' and
'f') are read and written, so need to be output operands (as they
already are!) but also marked as earlyclobber ("&") as they're modified
before all input operands are read ('out', in this case). It's just a
hint to the compiler to not make the register allocation overlap with
any (other) input operand register.
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 20:54 ` Mathias Krause
@ 2021-12-08 14:56 ` Jason A. Donenfeld
0 siblings, 0 replies; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-08 14:56 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
On Mon, Dec 6, 2021 at 9:54 PM Mathias Krause <minipli@grsecurity.net> wrote:
>
> Hi Jason,
>
> Am 06.12.21 um 20:28 schrieb Jason A. Donenfeld:
> > On Mon, Dec 6, 2021 at 7:55 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> >> Nice detective work! I just loaded this up on the CI, so we'll see if
> >> this does work across the board.
> >
> > Looks like https://git.zx2c4.com/wireguard-linux-compat/commit/?id=42c931dbccf9570f10a84e282daf79f385d51623
> > is all green on https://www.wireguard.com/build-status/ in the
> > wireguard-linux-compat category. Let me know if that commit looks okay
> > to you or if you want to adjust something about it. After that, I'll
> > cut a new compat snapshot release.
>
> ah, you modified the original commit of mine. Yeah, that works too.
> However, I'd add the following to the commit log to account for the
> output to input operand move:
>
> """
> Also make 'out' an input-only operand as it is only used as such. This
> not only allows gcc to optimize its usage further, but also works around
> older gcc versions, apparently failing to handle multiple alternatives
> correctly, as in failing to initialize the 'out' operand with its input
> value.
> """
>
Sure, will do.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-06 21:00 ` Mathias Krause
@ 2021-12-08 14:56 ` Jason A. Donenfeld
2021-12-09 7:59 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-08 14:56 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list
On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
> Yes, probably, but you're mixing up the two.
Oh, thanks, right.
I'll talk to EverCrypt upstream and see.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-08 14:56 ` Jason A. Donenfeld
@ 2021-12-09 7:59 ` Mathias Krause
2021-12-10 22:36 ` Jason A. Donenfeld
2021-12-10 22:58 ` Jason A. Donenfeld
0 siblings, 2 replies; 35+ messages in thread
From: Mathias Krause @ 2021-12-09 7:59 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list
Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
> On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
>> Yes, probably, but you're mixing up the two.
>
> Oh, thanks, right.
>
> I'll talk to EverCrypt upstream and see.
FWIW, 'out' is also wrongly flagged as output operand in fmul() and
fmul2(). But making it an input operand needs more surgery, as the
operand order changes and this requires some code churn.
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-09 7:59 ` Mathias Krause
@ 2021-12-10 22:36 ` Jason A. Donenfeld
2021-12-10 22:58 ` Jason A. Donenfeld
1 sibling, 0 replies; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-10 22:36 UTC (permalink / raw)
To: Mathias Krause, Aymeric Fromherz; +Cc: WireGuard mailing list
CC'ing in Aymeric, who's working on Vale's codegen.
On Thu, Dec 9, 2021 at 8:59 AM Mathias Krause <minipli@grsecurity.net> wrote:
>
> Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
> > On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
> >> Yes, probably, but you're mixing up the two.
> >
> > Oh, thanks, right.
> >
> > I'll talk to EverCrypt upstream and see.
>
> FWIW, 'out' is also wrongly flagged as output operand in fmul() and
> fmul2(). But making it an input operand needs more surgery, as the
> operand order changes and this requires some code churn.
>
> Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-09 7:59 ` Mathias Krause
2021-12-10 22:36 ` Jason A. Donenfeld
@ 2021-12-10 22:58 ` Jason A. Donenfeld
2021-12-11 16:35 ` Aymeric Fromherz
1 sibling, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-10 22:58 UTC (permalink / raw)
To: Mathias Krause, aymeric.fromherz; +Cc: WireGuard mailing list
CC'ing in Aymeric, who's working on Vale's codegen.
On Thu, Dec 9, 2021 at 8:59 AM Mathias Krause <minipli@grsecurity.net> wrote:
>
> Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
> > On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
> >> Yes, probably, but you're mixing up the two.
> >
> > Oh, thanks, right.
> >
> > I'll talk to EverCrypt upstream and see.
>
> FWIW, 'out' is also wrongly flagged as output operand in fmul() and
> fmul2(). But making it an input operand needs more surgery, as the
> operand order changes and this requires some code churn.
>
> Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-10 22:58 ` Jason A. Donenfeld
@ 2021-12-11 16:35 ` Aymeric Fromherz
2021-12-12 21:43 ` Jason A. Donenfeld
2021-12-13 7:44 ` Mathias Krause
0 siblings, 2 replies; 35+ messages in thread
From: Aymeric Fromherz @ 2021-12-11 16:35 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: Mathias Krause, WireGuard mailing list
Thanks for the heads-up. We were being overly conservative during verification of inline assembly code in Vale, and marked several registers as possibly modified while they were only read.
This is now fixed for fmul, fmul2, fsqr and fsqr2, and will be merged into the master branch of EverCrypt shortly.
In the meantime, the diff for the resulting inline assembly after Vale codegen is available here: https://github.com/project-everest/hacl-star/pull/501/commits/1a71adb40c3f78da16e16975dbb1d4de5adeab8c#diff-5aabe9f6aa87508c9d81d4c9e89eff0b06b1e2aeaf5b04eba51da71c5bea6940
Cheers,
Aymeric
----- Mail original -----
> De: "Jason A. Donenfeld" <Jason@zx2c4.com>
> À: "Mathias Krause" <minipli@grsecurity.net>, "aymeric fromherz" <aymeric.fromherz@inria.fr>
> Cc: "WireGuard mailing list" <wireguard@lists.zx2c4.com>
> Envoyé: Vendredi 10 Décembre 2021 23:58:01
> Objet: Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
> CC'ing in Aymeric, who's working on Vale's codegen.
>
> On Thu, Dec 9, 2021 at 8:59 AM Mathias Krause <minipli@grsecurity.net> wrote:
>>
>> Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
>> > On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
>> >> Yes, probably, but you're mixing up the two.
>> >
>> > Oh, thanks, right.
>> >
>> > I'll talk to EverCrypt upstream and see.
>>
>> FWIW, 'out' is also wrongly flagged as output operand in fmul() and
>> fmul2(). But making it an input operand needs more surgery, as the
>> operand order changes and this requires some code churn.
>>
> > Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-11 16:35 ` Aymeric Fromherz
@ 2021-12-12 21:43 ` Jason A. Donenfeld
2021-12-13 7:54 ` Mathias Krause
2021-12-13 7:44 ` Mathias Krause
1 sibling, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-12 21:43 UTC (permalink / raw)
To: Aymeric Fromherz; +Cc: Mathias Krause, WireGuard mailing list
Hi Aymeric, Mathias,
On Sat, Dec 11, 2021 at 5:35 PM Aymeric Fromherz
<aymeric.fromherz@inria.fr> wrote:
>
> Thanks for the heads-up. We were being overly conservative during verification of inline assembly code in Vale, and marked several registers as possibly modified while they were only read.
>
> This is now fixed for fmul, fmul2, fsqr and fsqr2, and will be merged into the master branch of EverCrypt shortly.
> In the meantime, the diff for the resulting inline assembly after Vale codegen is available here: https://github.com/project-everest/hacl-star/pull/501/commits/1a71adb40c3f78da16e16975dbb1d4de5adeab8c#diff-5aabe9f6aa87508c9d81d4c9e89eff0b06b1e2aeaf5b04eba51da71c5bea6940
Thanks for that! I've imported this into my staging tree here:
https://w-g.pw/l/rmQK
I'll wait a few days -- and until the Vale PR has been merged -- to
submit that to the linux-crypto mailing list, but there's a preview.
Mathias - I wonder if you still need alternatives with the new codegen
there.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-11 16:35 ` Aymeric Fromherz
2021-12-12 21:43 ` Jason A. Donenfeld
@ 2021-12-13 7:44 ` Mathias Krause
2021-12-13 14:20 ` Aymeric Fromherz
1 sibling, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-13 7:44 UTC (permalink / raw)
To: Aymeric Fromherz, Jason A. Donenfeld; +Cc: WireGuard mailing list
Hi Aymeric,
the changes look good to me -- quite what we already had in grsec. Just
one more nit. The constraints for the 'out' parameter in fmul(),
fmul2(), fsqr() and fsqr2() can be further lifted to "rm" as 'out' is
only referenced once. This allows gcc to choose either a register or a
memory operand, as it sees fit. In our experiments the latter lead to
much better code gen.
Thanks,
Mathias
Am 11.12.21 um 17:35 schrieb Aymeric Fromherz:
> Thanks for the heads-up. We were being overly conservative during verification of inline assembly code in Vale, and marked several registers as possibly modified while they were only read.
>
> This is now fixed for fmul, fmul2, fsqr and fsqr2, and will be merged into the master branch of EverCrypt shortly.
> In the meantime, the diff for the resulting inline assembly after Vale codegen is available here: https://github.com/project-everest/hacl-star/pull/501/commits/1a71adb40c3f78da16e16975dbb1d4de5adeab8c#diff-5aabe9f6aa87508c9d81d4c9e89eff0b06b1e2aeaf5b04eba51da71c5bea6940
>
> Cheers,
> Aymeric
>
> ----- Mail original -----
>> De: "Jason A. Donenfeld" <Jason@zx2c4.com>
>> À: "Mathias Krause" <minipli@grsecurity.net>, "aymeric fromherz" <aymeric.fromherz@inria.fr>
>> Cc: "WireGuard mailing list" <wireguard@lists.zx2c4.com>
>> Envoyé: Vendredi 10 Décembre 2021 23:58:01
>> Objet: Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
>
>> CC'ing in Aymeric, who's working on Vale's codegen.
>>
>> On Thu, Dec 9, 2021 at 8:59 AM Mathias Krause <minipli@grsecurity.net> wrote:
>>>
>>> Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
>>>> On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
>>>>> Yes, probably, but you're mixing up the two.
>>>>
>>>> Oh, thanks, right.
>>>>
>>>> I'll talk to EverCrypt upstream and see.
>>>
>>> FWIW, 'out' is also wrongly flagged as output operand in fmul() and
>>> fmul2(). But making it an input operand needs more surgery, as the
>>> operand order changes and this requires some code churn.
>>>
>>> Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-12 21:43 ` Jason A. Donenfeld
@ 2021-12-13 7:54 ` Mathias Krause
2021-12-13 11:36 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-13 7:54 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list, Aymeric Fromherz
Hi Jason,
Am 12.12.21 um 22:43 schrieb Jason A. Donenfeld:
> [snip]
>
> Thanks for that! I've imported this into my staging tree here:
> https://w-g.pw/l/rmQK
>
> I'll wait a few days -- and until the Vale PR has been merged -- to
> submit that to the linux-crypto mailing list, but there's a preview.
> Mathias - I wonder if you still need alternatives with the new codegen
> there.
yes, the alternative is still needed, as Aymeric's change doesn't lower
the number of required registers -- neither would a "rm" operand
constraint do, as gcc still insists in all constraints to be possible at
least, even if it would choose "m" instead of "r" in the end.
So, after importing Aymeric's changes into wireguard-linux-compat, the
alternatives change is still needed. I can help you create one, if you
want me to (we have that change in grsec anyway, just used named asm
operands here, as the %0/%1/%2/... was rather confusing and hard to follow).
Thanks,
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 7:54 ` Mathias Krause
@ 2021-12-13 11:36 ` Jason A. Donenfeld
2021-12-13 16:29 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-13 11:36 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list, Aymeric Fromherz
Hi Mathias,
On Mon, Dec 13, 2021 at 8:54 AM Mathias Krause <minipli@grsecurity.net> wrote:
> So, after importing Aymeric's changes into wireguard-linux-compat, the
> alternatives change is still needed. I can help you create one, if you
> want me to (we have that change in grsec anyway, just used named asm
> operands here, as the %0/%1/%2/... was rather confusing and hard to follow).
Sure, I'll take a patch for that. Let's wait until Aymeric
incorporates your suggestion on the out param, merges the PR, and then
I'll reimport that to wireguard-linux-compat, and you can send a
patch.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 7:44 ` Mathias Krause
@ 2021-12-13 14:20 ` Aymeric Fromherz
2021-12-13 14:33 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Aymeric Fromherz @ 2021-12-13 14:20 UTC (permalink / raw)
To: Mathias Krause; +Cc: Jason A. Donenfeld, WireGuard mailing list
Hi Mathias,
Thanks for the comments. Unfortunately, changing "r" to "rm" for the out parameter in a systematic way is quite trick.
Allowing arguments to be passed independently in a register or in memory is currently out of scope of the Vale model we use for verification.
We must decide early on in the verification pipeline if the argument is passed in a register, or if it stored in memory.
Doing this in a sound way would require a significant change to our (trusted) model.
Cheers,
Aymeric
----- Mail original -----
> De: "Mathias Krause" <minipli@grsecurity.net>
> À: "Aymeric Fromherz" <aymeric.fromherz@inria.fr>, "Jason A. Donenfeld" <Jason@zx2c4.com>
> Cc: "WireGuard mailing list" <wireguard@lists.zx2c4.com>
> Envoyé: Lundi 13 Décembre 2021 08:44:51
> Objet: Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
> Hi Aymeric,
>
> the changes look good to me -- quite what we already had in grsec. Just
> one more nit. The constraints for the 'out' parameter in fmul(),
> fmul2(), fsqr() and fsqr2() can be further lifted to "rm" as 'out' is
> only referenced once. This allows gcc to choose either a register or a
> memory operand, as it sees fit. In our experiments the latter lead to
> much better code gen.
>
> Thanks,
> Mathias
>
> Am 11.12.21 um 17:35 schrieb Aymeric Fromherz:
>> Thanks for the heads-up. We were being overly conservative during verification
>> of inline assembly code in Vale, and marked several registers as possibly
>> modified while they were only read.
>>
>> This is now fixed for fmul, fmul2, fsqr and fsqr2, and will be merged into the
>> master branch of EverCrypt shortly.
>> In the meantime, the diff for the resulting inline assembly after Vale codegen
>> is available here:
>> https://github.com/project-everest/hacl-star/pull/501/commits/1a71adb40c3f78da16e16975dbb1d4de5adeab8c#diff-5aabe9f6aa87508c9d81d4c9e89eff0b06b1e2aeaf5b04eba51da71c5bea6940
>>
>> Cheers,
>> Aymeric
>>
>> ----- Mail original -----
>>> De: "Jason A. Donenfeld" <Jason@zx2c4.com>
>>> À: "Mathias Krause" <minipli@grsecurity.net>, "aymeric fromherz"
>>> <aymeric.fromherz@inria.fr>
>>> Cc: "WireGuard mailing list" <wireguard@lists.zx2c4.com>
>>> Envoyé: Vendredi 10 Décembre 2021 23:58:01
>>> Objet: Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
>>
>>> CC'ing in Aymeric, who's working on Vale's codegen.
>>>
>>> On Thu, Dec 9, 2021 at 8:59 AM Mathias Krause <minipli@grsecurity.net> wrote:
>>>>
>>>> Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
>>>>> On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
>>>>>> Yes, probably, but you're mixing up the two.
>>>>>
>>>>> Oh, thanks, right.
>>>>>
>>>>> I'll talk to EverCrypt upstream and see.
>>>>
>>>> FWIW, 'out' is also wrongly flagged as output operand in fmul() and
>>>> fmul2(). But making it an input operand needs more surgery, as the
>>>> operand order changes and this requires some code churn.
>>>>
> >>> Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 14:20 ` Aymeric Fromherz
@ 2021-12-13 14:33 ` Mathias Krause
2021-12-13 14:37 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-13 14:33 UTC (permalink / raw)
To: Aymeric Fromherz; +Cc: Jason A. Donenfeld, WireGuard mailing list
Hi Aymeric,
yeah, don't worry. We can keep this change downstream in grsec. Dunno if
Jason wants to take it as well, his call. It was just a side observation
which came out of our need to read and understand the code to provide a
workaround for the gcc bug we were hitting.
Thanks,
Mathias
Am 13.12.21 um 15:20 schrieb Aymeric Fromherz:
> Hi Mathias,
>
> Thanks for the comments. Unfortunately, changing "r" to "rm" for the out parameter in a systematic way is quite trick.
> Allowing arguments to be passed independently in a register or in memory is currently out of scope of the Vale model we use for verification.
> We must decide early on in the verification pipeline if the argument is passed in a register, or if it stored in memory.
> Doing this in a sound way would require a significant change to our (trusted) model.
>
> Cheers,
> Aymeric
>
> ----- Mail original -----
>> De: "Mathias Krause" <minipli@grsecurity.net>
>> À: "Aymeric Fromherz" <aymeric.fromherz@inria.fr>, "Jason A. Donenfeld" <Jason@zx2c4.com>
>> Cc: "WireGuard mailing list" <wireguard@lists.zx2c4.com>
>> Envoyé: Lundi 13 Décembre 2021 08:44:51
>> Objet: Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
>
>> Hi Aymeric,
>>
>> the changes look good to me -- quite what we already had in grsec. Just
>> one more nit. The constraints for the 'out' parameter in fmul(),
>> fmul2(), fsqr() and fsqr2() can be further lifted to "rm" as 'out' is
>> only referenced once. This allows gcc to choose either a register or a
>> memory operand, as it sees fit. In our experiments the latter lead to
>> much better code gen.
>>
>> Thanks,
>> Mathias
>>
>> Am 11.12.21 um 17:35 schrieb Aymeric Fromherz:
>>> Thanks for the heads-up. We were being overly conservative during verification
>>> of inline assembly code in Vale, and marked several registers as possibly
>>> modified while they were only read.
>>>
>>> This is now fixed for fmul, fmul2, fsqr and fsqr2, and will be merged into the
>>> master branch of EverCrypt shortly.
>>> In the meantime, the diff for the resulting inline assembly after Vale codegen
>>> is available here:
>>> https://github.com/project-everest/hacl-star/pull/501/commits/1a71adb40c3f78da16e16975dbb1d4de5adeab8c#diff-5aabe9f6aa87508c9d81d4c9e89eff0b06b1e2aeaf5b04eba51da71c5bea6940
>>>
>>> Cheers,
>>> Aymeric
>>>
>>> ----- Mail original -----
>>>> De: "Jason A. Donenfeld" <Jason@zx2c4.com>
>>>> À: "Mathias Krause" <minipli@grsecurity.net>, "aymeric fromherz"
>>>> <aymeric.fromherz@inria.fr>
>>>> Cc: "WireGuard mailing list" <wireguard@lists.zx2c4.com>
>>>> Envoyé: Vendredi 10 Décembre 2021 23:58:01
>>>> Objet: Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
>>>
>>>> CC'ing in Aymeric, who's working on Vale's codegen.
>>>>
>>>> On Thu, Dec 9, 2021 at 8:59 AM Mathias Krause <minipli@grsecurity.net> wrote:
>>>>>
>>>>> Am 08.12.21 um 15:56 schrieb Jason A. Donenfeld:
>>>>>> On Mon, Dec 6, 2021 at 10:00 PM Mathias Krause <minipli@grsecurity.net> wrote:
>>>>>>> Yes, probably, but you're mixing up the two.
>>>>>>
>>>>>> Oh, thanks, right.
>>>>>>
>>>>>> I'll talk to EverCrypt upstream and see.
>>>>>
>>>>> FWIW, 'out' is also wrongly flagged as output operand in fmul() and
>>>>> fmul2(). But making it an input operand needs more surgery, as the
>>>>> operand order changes and this requires some code churn.
>>>>>
>>>>> Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 14:33 ` Mathias Krause
@ 2021-12-13 14:37 ` Jason A. Donenfeld
2021-12-13 16:32 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-13 14:37 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list, Aymeric Fromherz
On Mon, Dec 13, 2021 at 3:33 PM Mathias Krause <minipli@grsecurity.net> wrote:
> yeah, don't worry. We can keep this change downstream in grsec. Dunno if
> Jason wants to take it as well, his call. It was just a side observation
> which came out of our need to read and understand the code to provide a
> workaround for the gcc bug we were hitting.
I suppose I can load it up in kbench9000 to see if it makes a
difference. If it doesn't matter much, I'd prefer sticking with the
formally verified stuff. But if there is a nice speedup, then I guess
we can revisit more handwavy "obviously this is the same thing"
arguments.
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 11:36 ` Jason A. Donenfeld
@ 2021-12-13 16:29 ` Jason A. Donenfeld
2021-12-13 16:46 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-13 16:29 UTC (permalink / raw)
To: Mathias Krause; +Cc: WireGuard mailing list, Aymeric Fromherz
On Mon, Dec 13, 2021 at 12:36 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>
> Hi Mathias,
>
> On Mon, Dec 13, 2021 at 8:54 AM Mathias Krause <minipli@grsecurity.net> wrote:
> > So, after importing Aymeric's changes into wireguard-linux-compat, the
> > alternatives change is still needed. I can help you create one, if you
> > want me to (we have that change in grsec anyway, just used named asm
> > operands here, as the %0/%1/%2/... was rather confusing and hard to follow).
>
> Sure, I'll take a patch for that. Let's wait until Aymeric
> incorporates your suggestion on the out param, merges the PR, and then
> I'll reimport that to wireguard-linux-compat, and you can send a
> patch.
Actually, wasn't so bad, did it myself: https://w-g.pw/l/LjOY
Hopefully nothing to butchered in the process.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 14:37 ` Jason A. Donenfeld
@ 2021-12-13 16:32 ` Mathias Krause
2021-12-13 16:33 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-13 16:32 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list, Aymeric Fromherz
Am 13.12.21 um 15:37 schrieb Jason A. Donenfeld:
> On Mon, Dec 13, 2021 at 3:33 PM Mathias Krause <minipli@grsecurity.net> wrote:
>> yeah, don't worry. We can keep this change downstream in grsec. Dunno if
>> Jason wants to take it as well, his call. It was just a side observation
>> which came out of our need to read and understand the code to provide a
>> workaround for the gcc bug we were hitting.
>
> I suppose I can load it up in kbench9000 to see if it makes a
> difference. If it doesn't matter much, I'd prefer sticking with the
> formally verified stuff. But if there is a nice speedup, then I guess
> we can revisit more handwavy "obviously this is the same thing"
> arguments.
Had to hack main.c and run.sh a little, but here are some numbers:
root@box:~# ./run.sh
[+] Setting no-turbo to status 1
[+] Setting non-boot CPUs to status 0
[+] Inserting module to run tests
insmod: ERROR: could not insert module kbench9000.ko: Unknown symbol in
module
[+] Gathering results
ever64: 115100 cycles per call
ever64_out_r: 115080 cycles per call
ever64_out_rm: 113957 cycles per call
[+] Setting non-boot CPUs to status 1
[+] Setting no-turbo to status 0
Slightly faster.
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 16:32 ` Mathias Krause
@ 2021-12-13 16:33 ` Jason A. Donenfeld
2021-12-13 16:39 ` Mathias Krause
0 siblings, 1 reply; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-13 16:33 UTC (permalink / raw)
To: Mathias Krause
Cc: WireGuard mailing list, Aymeric Fromherz, Karthik Bhargavan
Hi Mathias,
On Mon, Dec 13, 2021 at 5:32 PM Mathias Krause <minipli@grsecurity.net> wrote:
>
> Am 13.12.21 um 15:37 schrieb Jason A. Donenfeld:
> > On Mon, Dec 13, 2021 at 3:33 PM Mathias Krause <minipli@grsecurity.net> wrote:
> >> yeah, don't worry. We can keep this change downstream in grsec. Dunno if
> >> Jason wants to take it as well, his call. It was just a side observation
> >> which came out of our need to read and understand the code to provide a
> >> workaround for the gcc bug we were hitting.
> >
> > I suppose I can load it up in kbench9000 to see if it makes a
> > difference. If it doesn't matter much, I'd prefer sticking with the
> > formally verified stuff. But if there is a nice speedup, then I guess
> > we can revisit more handwavy "obviously this is the same thing"
> > arguments.
>
> Had to hack main.c and run.sh a little, but here are some numbers:
>
> root@box:~# ./run.sh
> [+] Setting no-turbo to status 1
> [+] Setting non-boot CPUs to status 0
> [+] Inserting module to run tests
> insmod: ERROR: could not insert module kbench9000.ko: Unknown symbol in
> module
> [+] Gathering results
> ever64: 115100 cycles per call
> ever64_out_r: 115080 cycles per call
> ever64_out_rm: 113957 cycles per call
> [+] Setting non-boot CPUs to status 1
> [+] Setting no-turbo to status 0
>
> Slightly faster.
Huh, that's actually a pretty nice speedup for just changing register
allocation... What CPU is this?
Jason
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 16:33 ` Jason A. Donenfeld
@ 2021-12-13 16:39 ` Mathias Krause
2021-12-13 16:53 ` Jason A. Donenfeld
0 siblings, 1 reply; 35+ messages in thread
From: Mathias Krause @ 2021-12-13 16:39 UTC (permalink / raw)
To: Jason A. Donenfeld
Cc: WireGuard mailing list, Aymeric Fromherz, Karthik Bhargavan
Am 13.12.21 um 17:33 schrieb Jason A. Donenfeld:
>> root@box:~# ./run.sh
>> [+] Setting no-turbo to status 1
>> [+] Setting non-boot CPUs to status 0
>> [+] Inserting module to run tests
>> insmod: ERROR: could not insert module kbench9000.ko: Unknown symbol in
>> module
>> [+] Gathering results
>> ever64: 115100 cycles per call
>> ever64_out_r: 115080 cycles per call
>> ever64_out_rm: 113957 cycles per call
>> [+] Setting non-boot CPUs to status 1
>> [+] Setting no-turbo to status 0
>>
>> Slightly faster.
>
> Huh, that's actually a pretty nice speedup for just changing register
> allocation... What CPU is this?
It's an "Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz" But shouldn't matter
all that much, as the inline ASM is integer ops only.
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 16:29 ` Jason A. Donenfeld
@ 2021-12-13 16:46 ` Mathias Krause
0 siblings, 0 replies; 35+ messages in thread
From: Mathias Krause @ 2021-12-13 16:46 UTC (permalink / raw)
To: Jason A. Donenfeld; +Cc: WireGuard mailing list, Aymeric Fromherz
Am 13.12.21 um 17:29 schrieb Jason A. Donenfeld:
> On Mon, Dec 13, 2021 at 12:36 PM Jason A. Donenfeld <Jason@zx2c4.com> wrote:
>>
>> Hi Mathias,
>>
>> On Mon, Dec 13, 2021 at 8:54 AM Mathias Krause <minipli@grsecurity.net> wrote:
>>> So, after importing Aymeric's changes into wireguard-linux-compat, the
>>> alternatives change is still needed. I can help you create one, if you
>>> want me to (we have that change in grsec anyway, just used named asm
>>> operands here, as the %0/%1/%2/... was rather confusing and hard to follow).
>>
>> Sure, I'll take a patch for that. Let's wait until Aymeric
>> incorporates your suggestion on the out param, merges the PR, and then
>> I'll reimport that to wireguard-linux-compat, and you can send a
>> patch.
>
> Actually, wasn't so bad, did it myself: https://w-g.pw/l/LjOY
> Hopefully nothing to butchered in the process.
Looks good to me. Actually had to fetch the repo and use `git show
--color-words -w` to get a sensible signal-to-noise ratio ;)
Mathias
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches
2021-12-13 16:39 ` Mathias Krause
@ 2021-12-13 16:53 ` Jason A. Donenfeld
0 siblings, 0 replies; 35+ messages in thread
From: Jason A. Donenfeld @ 2021-12-13 16:53 UTC (permalink / raw)
To: Mathias Krause
Cc: WireGuard mailing list, Aymeric Fromherz, Karthik Bhargavan
On Mon, Dec 13, 2021 at 5:39 PM Mathias Krause <minipli@grsecurity.net> wrote:
>
> Am 13.12.21 um 17:33 schrieb Jason A. Donenfeld:
> >> root@box:~# ./run.sh
> >> [+] Setting no-turbo to status 1
> >> [+] Setting non-boot CPUs to status 0
> >> [+] Inserting module to run tests
> >> insmod: ERROR: could not insert module kbench9000.ko: Unknown symbol in
> >> module
> >> [+] Gathering results
> >> ever64: 115100 cycles per call
> >> ever64_out_r: 115080 cycles per call
> >> ever64_out_rm: 113957 cycles per call
> >> [+] Setting non-boot CPUs to status 1
> >> [+] Setting no-turbo to status 0
> >>
> >> Slightly faster.
> >
> > Huh, that's actually a pretty nice speedup for just changing register
> > allocation... What CPU is this?
>
> It's an "Intel(R) Core(TM) i9-9900 CPU @ 3.10GHz" But shouldn't matter
> all that much, as the inline ASM is integer ops only.
Yet it does. Here are results on a tigerlake, i7-11850H:
[+] Setting no-turbo to status 1
[+] Setting non-boot CPUs to status 0
[+] Inserting module to run tests
[+] Gathering results
donna64: 141081 cycles per call
hacl64: 135645 cycles per call
fiat64: 132547 cycles per call
sandy2x: 126933 cycles per call
precomp_bmi2: 120276 cycles per call
precomp_adx: 113578 cycles per call
ever64: 106943 cycles per call
ever64r: 105924 cycles per call
ever64rm: 108139 cycles per call
fiat32: 297854 cycles per call
donna32: 467667 cycles per call
tweetnacl: 1195385 cycles per call
[+] Setting non-boot CPUs to status 1
[+] Setting no-turbo to status 0
https://xn--4db.cc/c4B65RUy/diff
Looks like Tigerlake is able to make use of free stack accesses thanks
to the new "fast forwarding" feature.
^ permalink raw reply [flat|nested] 35+ messages in thread
end of thread, other threads:[~2021-12-13 16:55 UTC | newest]
Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-06 13:27 [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Mathias Krause
2021-07-06 13:27 ` [PATCH 1/2] compat: better grsecurity compatibility Mathias Krause
2021-07-06 13:27 ` [PATCH 2/2] curve25519-x86_64: solve register constraints with reserved registers Mathias Krause
2021-08-08 20:53 ` [PATCH 0/2] wireguard-linux-compat: grsecurity compat patches Jason A. Donenfeld
2021-08-09 10:13 ` Mathias Krause
2021-12-03 22:20 ` Jason A. Donenfeld
2021-12-03 22:25 ` Jason A. Donenfeld
2021-12-06 14:04 ` Mathias Krause
2021-12-06 14:48 ` Jason A. Donenfeld
2021-12-06 16:24 ` Mathias Krause
2021-12-06 16:27 ` Jason A. Donenfeld
2021-12-06 18:18 ` Mathias Krause
2021-12-06 18:55 ` Jason A. Donenfeld
2021-12-06 19:28 ` Jason A. Donenfeld
2021-12-06 20:54 ` Mathias Krause
2021-12-08 14:56 ` Jason A. Donenfeld
2021-12-06 21:00 ` Mathias Krause
2021-12-08 14:56 ` Jason A. Donenfeld
2021-12-09 7:59 ` Mathias Krause
2021-12-10 22:36 ` Jason A. Donenfeld
2021-12-10 22:58 ` Jason A. Donenfeld
2021-12-11 16:35 ` Aymeric Fromherz
2021-12-12 21:43 ` Jason A. Donenfeld
2021-12-13 7:54 ` Mathias Krause
2021-12-13 11:36 ` Jason A. Donenfeld
2021-12-13 16:29 ` Jason A. Donenfeld
2021-12-13 16:46 ` Mathias Krause
2021-12-13 7:44 ` Mathias Krause
2021-12-13 14:20 ` Aymeric Fromherz
2021-12-13 14:33 ` Mathias Krause
2021-12-13 14:37 ` Jason A. Donenfeld
2021-12-13 16:32 ` Mathias Krause
2021-12-13 16:33 ` Jason A. Donenfeld
2021-12-13 16:39 ` Mathias Krause
2021-12-13 16:53 ` Jason A. Donenfeld
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).