wireguard.lists.zx2c4.com archive mirror
 help / color / mirror / Atom feed
* [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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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; 19+ 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] 19+ 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
  0 siblings, 0 replies; 19+ 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] 19+ messages in thread

end of thread, other threads:[~2021-12-09  8:02 UTC | newest]

Thread overview: 19+ 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

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).