Re: syscall(SYS_sysctl) broken in 7.3

2023-05-11 Thread Alexander Bluhm
On Wed, May 10, 2023 at 07:19:52PM -0700, Philip Guenther wrote:
> Ick.  How about this instead, which preserves retguard protection for
> syscall(2) and doesn't copy all of SYS.h?

Much better.  Works for me.  OK bluhm@

> Index: lib/libc/arch/amd64/sys/syscall.S
> ===
> retrieving revision 1.8
> diff -u -p -r1.8 syscall.S
> --- lib/libc/arch/amd64/sys/syscall.S   7 May 2016 19:05:21 -   1.8
> +++ lib/libc/arch/amd64/sys/syscall.S   11 May 2023 02:13:20 -
> @@ -39,4 +39,18 @@
> 
>  #include "SYS.h"
> 
> -RSYSCALL(syscall)
> +SYSENTRY(syscall)
> +   RETGUARD_SETUP(_thread_sys_syscall, r11)
> +#ifdef _RET_PROTECTOR
> +   pushq   8(%rsp) /* repush 6th argument */
> +#endif
> +   RETGUARD_PUSH(r11)
> +   SYSTRAP(syscall)
> +   HANDLE_ERRNO;
> +   RETGUARD_POP(r11)
> +#ifdef _RET_PROTECTOR
> +   addq$8,%rsp /* pop 6th argument */
> +#endif
> +   RETGUARD_CHECK(_thread_sys_syscall, r11)
> +   ret
> +SYSCALL_END(syscall)



Re: syscall(SYS_sysctl) broken in 7.3

2023-05-10 Thread Philip Guenther
On Wed, May 10, 2023 at 1:29 PM Alexander Bluhm 
wrote:

> We use Perl syscall(2) function to implement sysctl(2) system calls.
> This is broken since OpenBSD 7.3.  A sample program looks like this:
>
...

> kdump shows two problems:
> 1. The mib is not printed correctly.
> 2. The final argument newlen should be 0x10, but is 0x8891aa893c5.
>
>  30762 perl CALL  (via syscall)
> sysctl(538970683.538976288.538976288.1718165536,0,0,0x8891d2e1de0,0x8891aa893c5)
>  30762 perl RET   sysctl -1 errno 22 Invalid argument
>
> 1. The syscall code now contains the KTRC_CODE_SYSCALL flag, but
> the kernel checks code == SYS_sysctl.  So the mib is not added to
> ktrace output.  Fix is easy by using KTRC_CODE_MASK.
>

ok guenther@ on the first chunk, the ktrsyscall() change.

The flag is only passed to ktrsyscall(), not ktrsysret(), so the second
chunk of your diff is unnecessary.



> 2. Was introdced by this commit in libc.
> 
> revision 1.21
> date: 2023/01/11 01:55:17;  author: mortimer;  state: Exp;  lines: +24
> -13;  commitid: 72pYktDvmJhq7OyF;
> Add retguard to amd64 syscalls.
>
> Since we got rid of padded syscalls we have enough registers to do this.
>
> ok deraadt@ ok kettenis@
> 
> It assumes that syscalls only have 6 parameters, which is not true
> if syscall(2) adds the syscall number.  Now the final argument
> contains retguard values from the stack.
>

This is why we can't have nice things.



> The easies way to fix this without reverting retguard for all
> syscalls, is to use revision 1.20 of SYS.h in libc for syscall.S.
> Just manually include the old code instead of current SYS.h.
>
> I am aware that syscall(2) should go way, but currently I need it.
>
> ok?
>

Ick.  How about this instead, which preserves retguard protection for
syscall(2) and doesn't copy all of SYS.h?

Index: lib/libc/arch/amd64/sys/syscall.S
===
retrieving revision 1.8
diff -u -p -r1.8 syscall.S
--- lib/libc/arch/amd64/sys/syscall.S   7 May 2016 19:05:21 -   1.8
+++ lib/libc/arch/amd64/sys/syscall.S   11 May 2023 02:13:20 -
@@ -39,4 +39,18 @@

 #include "SYS.h"

-RSYSCALL(syscall)
+SYSENTRY(syscall)
+   RETGUARD_SETUP(_thread_sys_syscall, r11)
+#ifdef _RET_PROTECTOR
+   pushq   8(%rsp) /* repush 6th argument */
+#endif
+   RETGUARD_PUSH(r11)
+   SYSTRAP(syscall)
+   HANDLE_ERRNO;
+   RETGUARD_POP(r11)
+#ifdef _RET_PROTECTOR
+   addq$8,%rsp /* pop 6th argument */
+#endif
+   RETGUARD_CHECK(_thread_sys_syscall, r11)
+   ret
+SYSCALL_END(syscall)


syscall(SYS_sysctl) broken in 7.3

2023-05-10 Thread Alexander Bluhm
Hi,

We use Perl syscall(2) function to implement sysctl(2) system calls.
This is broken since OpenBSD 7.3.  A sample program looks like this:

#!/usr/bin/perl
use constant SYS_sysctl => 202;
# net.inet6.ip6.soiikey
my @mib = (4, 24, 41, 54);
my $miblen = scalar(@mib);
my $mib = pack("i$miblen", @mib);
$new = pack("H32", "0123456789abcdef0123456789abcdef");
$newlen = length($new);
syscall(SYS_sysctl(), $mib, $miblen, 0, 0, $new, $newlen);

kdump shows two problems:
1. The mib is not printed correctly.
2. The final argument newlen should be 0x10, but is 0x8891aa893c5.

 30762 perl CALL  (via syscall) 
sysctl(538970683.538976288.538976288.1718165536,0,0,0x8891d2e1de0,0x8891aa893c5)
 30762 perl RET   sysctl -1 errno 22 Invalid argument

1. The syscall code now contains the KTRC_CODE_SYSCALL flag, but
the kernel checks code == SYS_sysctl.  So the mib is not added to
ktrace output.  Fix is easy by using KTRC_CODE_MASK.

2. Was introdced by this commit in libc.

revision 1.21
date: 2023/01/11 01:55:17;  author: mortimer;  state: Exp;  lines: +24 -13;  
commitid: 72pYktDvmJhq7OyF;
Add retguard to amd64 syscalls.

Since we got rid of padded syscalls we have enough registers to do this.

ok deraadt@ ok kettenis@

It assumes that syscalls only have 6 parameters, which is not true
if syscall(2) adds the syscall number.  Now the final argument
contains retguard values from the stack.

The easies way to fix this without reverting retguard for all
syscalls, is to use revision 1.20 of SYS.h in libc for syscall.S.
Just manually include the old code instead of current SYS.h.

I am aware that syscall(2) should go way, but currently I need it.

ok?

bluhm

Index: sys/kern/kern_ktrace.c
===
RCS file: /data/mirror/openbsd/cvs/src/sys/kern/kern_ktrace.c,v
retrieving revision 1.111
diff -u -p -r1.111 kern_ktrace.c
--- sys/kern/kern_ktrace.c  16 Feb 2023 08:50:57 -  1.111
+++ sys/kern/kern_ktrace.c  10 May 2023 16:28:39 -
@@ -160,7 +160,7 @@ ktrsyscall(struct proc *p, register_t co
u_int nargs = 0;
int i;
 
-   if (code == SYS_sysctl) {
+   if ((code & KTRC_CODE_MASK) == SYS_sysctl) {
/*
 * The sysctl encoding stores the mib[]
 * array because it is interesting.
@@ -198,7 +198,7 @@ ktrsysret(struct proc *p, register_t cod
ktp.ktr_error = error;
if (error)
len = 0;
-   else if (code == SYS_lseek)
+   else if ((code & KTRC_CODE_MASK) == SYS_lseek)
/* the one exception: lseek on ILP32 needs more */
len = sizeof(long long);
else
Index: lib/libc/arch/amd64/sys/syscall.S
===
RCS file: /data/mirror/openbsd/cvs/src/lib/libc/arch/amd64/sys/syscall.S,v
retrieving revision 1.8
diff -u -p -r1.8 syscall.S
--- lib/libc/arch/amd64/sys/syscall.S   7 May 2016 19:05:21 -   1.8
+++ lib/libc/arch/amd64/sys/syscall.S   10 May 2023 19:29:17 -
@@ -37,6 +37,80 @@
 
 #include 
 
-#include "SYS.h"
+#include "DEFS.h"
+#include 
+
+/* offsetof(struct tib, tib_errno) - offsetof(struct tib, __tib_tcb) */
+#defineTCB_OFFSET_ERRNO32
+
+#define SYSTRAP(x) movl $(SYS_ ## x),%eax; movq %rcx, %r10; syscall
+
+#define SYSENTRY(x)\
+   SYSENTRY_HIDDEN(x); \
+   WEAK_ALIAS(x, _thread_sys_##x)
+#define SYSENTRY_HIDDEN(x) \
+   ENTRY(_thread_sys_ ## x)
+
+#defineSYSCALL_END_HIDDEN(x)   
\
+   END(_thread_sys_ ## x); \
+   _HIDDEN_FALIAS(x,_thread_sys_##x);  \
+   END(_HIDDEN(x))
+#defineSYSCALL_END(x)  SYSCALL_END_HIDDEN(x); END(x)
+
+
+#define SET_ERRNO  \
+   movl%eax,%fs:(TCB_OFFSET_ERRNO);\
+   movq$-1, %rax
+#define HANDLE_ERRNO   \
+   jnc 99f;\
+   SET_ERRNO;  \
+   99:
+
+#define _SYSCALL_NOERROR(x,y)  \
+   SYSENTRY(x);\
+   SYSTRAP(y)
+#define _SYSCALL_HIDDEN_NOERROR(x,y)   \
+   SYSENTRY_HIDDEN(x); \
+   SYSTRAP(y)
+
+#define SYSCALL_NOERROR(x) \
+   _SYSCALL_NOERROR(x,x)
+
+#define SYSCALL_HIDDEN(x)  \
+   _SYSCALL_HIDDEN_NOERROR(x,x);