[PATCH v6 0/3] x86_64: aarch64: Set call number just before syscall

[PATCH v6 0/3] x86_64: aarch64: Set call number just before syscall

[Joe Simmons-Talbott]

To make binary call tree analysis easier place the syscall number into
the register just before the syscall is made.  Only do this if the
syscall number is a constant integer.

Compiler optimizations can place quite a few instructions between the
setting of the syscall number and the syscall instruction.  During call
tree analysis the number of instructions between the two can lead to
more difficulty for both tools and humans in properly identifying the
syscall number.  Having the syscall number set in the prior instruction
to the syscall instruction makes this task easier and less error prone.
Being able to reliably identify syscalls made by a given API will make
it easier to understand and verify the safety and security of glibc.

Tested on x86_64 and aarch64.

Chnages to v5:
  * aarch64: Remove '\n\t' from asm.
  * x86_64: Remove unused macros.

Changes to v4:
  * aarch64: Move generated comment.  Remove unused macros from earlier
  version.

  * Added patch
    - nptl: Convert indirect syscall numbers to compile time numeric
      values.

Changes to v3:
  * x86_64: Combine two inline asms into one.
    - Suggested by "H.J. Lu" <hjl.tools@gmail.com>

Changes to v2:
  * Add a more detailed commit message.
  
Changes to v1:
  * aarch64: Combine two inline asms into one.  Avoid loading name twice.

Joe Simmons-Talbott (3):
  x86_64: Set the syscall register right before doing the syscall.
  aarch64: Set the syscall register right before doing the syscall.
  nptl: Use direct syscall numbers in setxid

 nptl/nptl_setxid.c                       | 136 ++++++++++++++++++++++-
 sysdeps/unix/sysv/linux/aarch64/sysdep.h |  29 +++--
 sysdeps/unix/sysv/linux/x86_64/sysdep.h  |  24 ++--
 3 files changed, 164 insertions(+), 25 deletions(-)

-- 
2.39.2

[PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Joe Simmons-Talbott]

To make identifying syscalls easier during call tree analysis load the
syscall number just before performing the syscall.

Compiler optimizations can place quite a few instructions between the
setting of the syscall number and the syscall instruction.  During call
tree analysis the number of instructions between the two can lead to
more difficulty for both tools and humans in properly identifying the
syscall number.  Having the syscall number set in the prior instruction
to the syscall instruction makes this task easier and less error prone.
Being able to reliably identify syscalls made by a given API will make
it easier to understand and verify the safety and security of glibc.
---
 sysdeps/unix/sysv/linux/x86_64/sysdep.h | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
index cfb51be8c5..0db8660531 100644
--- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
@@ -257,9 +257,9 @@
     TYPEFY (arg1, __arg1) = ARGIFY (arg1);			 	\
     register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
     asm volatile (							\
-    "syscall\n\t"							\
+    "movl %1, %k0\n\tsyscall\n\t"					\
     : "=a" (resultvar)							\
-    : "0" (number), "r" (_a1)						\
+    : "g" (number), "r" (_a1)						\
     : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
     (long int) resultvar;						\
 })
@@ -273,9 +273,9 @@
     register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
     register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
     asm volatile (							\
-    "syscall\n\t"							\
+    "movl %1, %k0\n\tsyscall\n\t"					\
     : "=a" (resultvar)							\
-    : "0" (number), "r" (_a1), "r" (_a2)				\
+    : "g" (number), "r" (_a1), "r" (_a2)				\
     : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
     (long int) resultvar;						\
 })
@@ -291,9 +291,9 @@
     register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
     register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
     asm volatile (							\
-    "syscall\n\t"							\
+    "movl %1, %k0\n\tsyscall\n\t"					\
     : "=a" (resultvar)							\
-    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3)			\
+    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3)			\
     : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
     (long int) resultvar;						\
 })
@@ -311,9 +311,9 @@
     register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
     register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
     asm volatile (							\
-    "syscall\n\t"							\
+    "movl %1, %k0\n\tsyscall\n\t"					\
     : "=a" (resultvar)							\
-    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)		\
+    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)		\
     : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
     (long int) resultvar;						\
 })
@@ -333,9 +333,9 @@
     register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
     register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
     asm volatile (							\
-    "syscall\n\t"							\
+    "movl %1, %k0\n\tsyscall\n\t"					\
     : "=a" (resultvar)							\
-    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
+    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
       "r" (_a5)								\
     : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
     (long int) resultvar;						\
@@ -358,9 +358,9 @@
     register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
     register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
     asm volatile (							\
-    "syscall\n\t"							\
+    "movl %1, %k0\n\tsyscall\n\t"					\
     : "=a" (resultvar)							\
-    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
+    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
       "r" (_a5), "r" (_a6)						\
     : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
     (long int) resultvar;						\
-- 
2.39.2

[PATCH v6 2/3] aarch64: Set the syscall register right before doing the syscall.

[Joe Simmons-Talbott]

To make identifying syscalls easier during call tree analysis load the
syscall number just before performing the syscall.

Compiler optimizations can place quite a few instructions between the
setting of the syscall number and the syscall instruction.  During call
tree analysis the number of instructions between the two can lead to
more difficulty for both tools and humans in properly identifying the
syscall number.  Having the syscall number set in the prior instruction
to the syscall instruction makes this task easier and less error prone.
Being able to reliably identify syscalls made by a given API will make
it easier to understand and verify the safety and security of glibc.
---
 sysdeps/unix/sysv/linux/aarch64/sysdep.h | 29 ++++++++++++++++--------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
index e94d1703ad..6fe40aaf89 100644
--- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
+++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
@@ -168,15 +168,26 @@
 # define HAVE_CLONE3_WRAPPER		1
 
 # undef INTERNAL_SYSCALL_RAW
-# define INTERNAL_SYSCALL_RAW(name, nr, args...)		\
-  ({ long _sys_result;						\
-     {								\
-       LOAD_ARGS_##nr (args)					\
-       register long _x8 asm ("x8") = (name);			\
-       asm volatile ("svc	0	// syscall " # name     \
-		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
-       _sys_result = _x0;					\
-     }								\
+# define INTERNAL_SYSCALL_RAW(name, nr, args...)			\
+  ({ long _sys_result;							\
+     {									\
+       LOAD_ARGS_##nr (args)						\
+       if (__builtin_constant_p(name))					\
+         asm volatile ("mov	x8, %1	// syscall " # name "\n"	\
+		       "svc	0"					\
+                       : "=r" (_x0)					\
+		       : "i" (name) ASM_ARGS_##nr			\
+		       : "x8", "memory");				\
+       else								\
+         {								\
+           register long _x8 asm ("x8") = (name);			\
+           asm volatile ("svc	0"					\
+		         : "=r" (_x0)					\
+		         : "r"(_x8) ASM_ARGS_##nr			\
+		         : "memory");					\
+         }								\
+       _sys_result = _x0;						\
+     }									\
      _sys_result; })
 
 # undef INTERNAL_SYSCALL
-- 
2.39.2

[PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Joe Simmons-Talbott]

Make all internal glibc syscalls use direct compile time numeric values
rather than variables.  This will make the syscall number easier to
identify during static analysis.
---
 nptl/nptl_setxid.c | 136 +++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 132 insertions(+), 4 deletions(-)

diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
index 4bfcfe4188..797015def1 100644
--- a/nptl/nptl_setxid.c
+++ b/nptl/nptl_setxid.c
@@ -66,8 +66,72 @@ __nptl_setxid_sighandler (int sig, siginfo_t *si, void *ctx)
       || si->si_code != SI_TKILL)
     return;
 
-  result = INTERNAL_SYSCALL_NCS (xidcmd->syscall_no, 3, xidcmd->id[0],
-				 xidcmd->id[1], xidcmd->id[2]);
+  switch(xidcmd->syscall_no)
+  {
+#ifdef __NR_setuid32
+    case __NR_setuid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setuid32, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#endif /* __NR_setuid32 */
+    case __NR_setuid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setuid, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#ifdef __NR_setreuid32
+    case __NR_setreuid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setreuid32, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#endif /* __NR_setreuid32 */
+    case __NR_setreuid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setreuid, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#ifdef __NR_setresgid32
+    case __NR_setresgid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setresgid32, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#endif /* __NR_setresgid32 */
+    case __NR_setresgid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setresgid, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#ifdef __NR_setregid32
+    case __NR_setregid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setregid32, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#endif /* __NR_setregid32 */
+    case __NR_setregid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setregid, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#ifdef __NR_setgid32
+    case __NR_setgid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgid32, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#endif /* __NR_setgid32 */
+    case __NR_setgid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgid, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#ifdef __NR_setgroups32
+    case __NR_setgroups32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgroups32, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+#endif /* __NR_setgroups32 */
+    case __NR_setgroups:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+      break;
+   default:
+      result = INTERNAL_SYSCALL_NCS (xidcmd->syscall_no, 3, xidcmd->id[0],
+				     xidcmd->id[1], xidcmd->id[2]);
+  }
   int error = 0;
   if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
     error = INTERNAL_SYSCALL_ERRNO (result);
@@ -262,8 +326,72 @@ __nptl_setxid (struct xid_command *cmdp)
 
   /* This must be last, otherwise the current thread might not have
      permissions to send SIGSETXID syscall to the other threads.  */
-  result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
-                                 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
+  switch(cmdp->syscall_no)
+  {
+#ifdef __NR_setuid32
+    case __NR_setuid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setuid32, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#endif /* __NR_setuid32 */
+    case __NR_setuid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setuid, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#ifdef __NR_setreuid32
+    case __NR_setreuid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setreuid32, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#endif /* __NR_setreuid32 */
+    case __NR_setreuid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setreuid, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#ifdef __NR_setresgid32
+    case __NR_setresgid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setresgid32, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#endif /* __NR_setresgid32 */
+    case __NR_setresgid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setresgid, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#ifdef __NR_setregid32
+    case __NR_setregid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setregid32, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#endif /* __NR_setregid32 */
+    case __NR_setregid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setregid, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#ifdef __NR_setgid32
+    case __NR_setgid32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgid32, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#endif /* __NR_setgid32 */
+    case __NR_setgid:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgid, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#ifdef __NR_setgroups32
+    case __NR_setgroups32:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgroups32, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+#endif /* __NR_setgroups32 */
+    case __NR_setgroups:
+      result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+      break;
+   default:
+      result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3, cmdp->id[0],
+				     cmdp->id[1], cmdp->id[2]);
+  }
   int error = 0;
   if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
     {
-- 
2.39.2

Re: [PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Xi Ruoyao]

On Mon, 2023-04-24 at 11:03 -0400, Joe Simmons-Talbott via Libc-alpha
wrote:
> Make all internal glibc syscalls use direct compile time numeric values
> rather than variables.  This will make the syscall number easier to
> identify during static analysis.

This is making the code much more bloated and slower.  Do we really want
to make everyone's system slower for some debug tools?

> ---
>  nptl/nptl_setxid.c | 136 +++++++++++++++++++++++++++++++++++++++++++-
> -
>  1 file changed, 132 insertions(+), 4 deletions(-)
> 
> diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
> index 4bfcfe4188..797015def1 100644
> --- a/nptl/nptl_setxid.c
> +++ b/nptl/nptl_setxid.c
> @@ -66,8 +66,72 @@ __nptl_setxid_sighandler (int sig, siginfo_t *si,
> void *ctx)
>        || si->si_code != SI_TKILL)
>      return;
>  
> -  result = INTERNAL_SYSCALL_NCS (xidcmd->syscall_no, 3, xidcmd-
> >id[0],
> -                                xidcmd->id[1], xidcmd->id[2]);
> +  switch(xidcmd->syscall_no)
> +  {
> +#ifdef __NR_setuid32
> +    case __NR_setuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid32, 3, xidcmd->id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setuid32 */
> +    case __NR_setuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid, 3, xidcmd->id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setreuid32
> +    case __NR_setreuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid32, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setreuid32 */
> +    case __NR_setreuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid, 3, xidcmd->id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setresgid32
> +    case __NR_setresgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid32, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setresgid32 */
> +    case __NR_setresgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setregid32
> +    case __NR_setregid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid32, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setregid32 */
> +    case __NR_setregid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid, 3, xidcmd->id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setgid32
> +    case __NR_setgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid32, 3, xidcmd->id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setgid32 */
> +    case __NR_setgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid, 3, xidcmd->id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setgroups32
> +    case __NR_setgroups32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups32, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setgroups32 */
> +    case __NR_setgroups:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +   default:
> +      result = INTERNAL_SYSCALL_NCS (xidcmd->syscall_no, 3, xidcmd-
> >id[0],
> +                                    xidcmd->id[1], xidcmd->id[2]);
> +  }
>    int error = 0;
>    if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
>      error = INTERNAL_SYSCALL_ERRNO (result);
> @@ -262,8 +326,72 @@ __nptl_setxid (struct xid_command *cmdp)
>  
>    /* This must be last, otherwise the current thread might not have
>       permissions to send SIGSETXID syscall to the other threads.  */
> -  result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> -                                 cmdp->id[0], cmdp->id[1], cmdp-
> >id[2]);
> +  switch(cmdp->syscall_no)
> +  {
> +#ifdef __NR_setuid32
> +    case __NR_setuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid32, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setuid32 */
> +    case __NR_setuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setreuid32
> +    case __NR_setreuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid32, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setreuid32 */
> +    case __NR_setreuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setresgid32
> +    case __NR_setresgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid32, 3, cmdp-
> >id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setresgid32 */
> +    case __NR_setresgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setregid32
> +    case __NR_setregid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid32, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setregid32 */
> +    case __NR_setregid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setgid32
> +    case __NR_setgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid32, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setgid32 */
> +    case __NR_setgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setgroups32
> +    case __NR_setgroups32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups32, 3, cmdp-
> >id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setgroups32 */
> +    case __NR_setgroups:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 3, cmdp->id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +      break;
> +   default:
> +      result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3, cmdp-
> >id[0],
> +                                    cmdp->id[1], cmdp->id[2]);
> +  }
>    int error = 0;
>    if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
>      {

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Szabolcs Nagy]

The 04/24/2023 23:17, Xi Ruoyao via Libc-alpha wrote:
> On Mon, 2023-04-24 at 11:03 -0400, Joe Simmons-Talbott via Libc-alpha
> wrote:
> > Make all internal glibc syscalls use direct compile time numeric values
> > rather than variables.  This will make the syscall number easier to
> > identify during static analysis.
> 
> This is making the code much more bloated and slower.  Do we really want
> to make everyone's system slower for some debug tools?

the switch statement overhead is many orders of magnitude smaller
than sending a signal to a thread and executing a syscall there
(which is where the switch statement happens).

i dont know if the change is justified, but from a target port
perspective it's useful if inline syscalls are guaranteed to use
a syscall name that can expand to an integer constant or named
function.

Re: [PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Cristian Rodríguez]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

On Mon, Apr 24, 2023 at 11:17 AM Xi Ruoyao via Libc-alpha <
libc-alpha@sourceware.org> wrote:

> On Mon, 2023-04-24 at 11:03 -0400, Joe Simmons-Talbott via Libc-alpha
> wrote:
> > Make all internal glibc syscalls use direct compile time numeric values
> > rather than variables.  This will make the syscall number easier to
> > identify during static analysis.
>
> This is making the code much more bloated and slower.  Do we really want
> to make everyone's system slower for some debug tools?
>
>
The switch statement will add little overhead. my concern with this is that
it looks ugly and error prone for no other reason than help some
limited debug tool.. there must be a nicer, less verbose  way to get what
you want..

Re: [PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Szabolcs Nagy]

The 04/26/2023 08:39, Cristian Rodríguez via Libc-alpha wrote:
> On Mon, Apr 24, 2023 at 11:17 AM Xi Ruoyao via Libc-alpha <
> libc-alpha@sourceware.org> wrote:
> 
> > On Mon, 2023-04-24 at 11:03 -0400, Joe Simmons-Talbott via Libc-alpha
> > wrote:
> > > Make all internal glibc syscalls use direct compile time numeric values
> > > rather than variables.  This will make the syscall number easier to
> > > identify during static analysis.
> >
> > This is making the code much more bloated and slower.  Do we really want
> > to make everyone's system slower for some debug tools?
> >
> >
> The switch statement will add little overhead. my concern with this is that
> it looks ugly and error prone for no other reason than help some
> limited debug tool.. there must be a nicer, less verbose  way to get what
> you want..

it allows glibc to drop the INTERNAL_SYSCALL_NCS code.
which can clean up some target code.

Re: [PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Florian Weimer]

* Szabolcs Nagy via Libc-alpha:

> The 04/24/2023 23:17, Xi Ruoyao via Libc-alpha wrote:
>> On Mon, 2023-04-24 at 11:03 -0400, Joe Simmons-Talbott via Libc-alpha
>> wrote:
>> > Make all internal glibc syscalls use direct compile time numeric values
>> > rather than variables.  This will make the syscall number easier to
>> > identify during static analysis.
>> 
>> This is making the code much more bloated and slower.  Do we really want
>> to make everyone's system slower for some debug tools?
>
> the switch statement overhead is many orders of magnitude smaller
> than sending a signal to a thread and executing a syscall there
> (which is where the switch statement happens).
>
> i dont know if the change is justified, but from a target port
> perspective it's useful if inline syscalls are guaranteed to use
> a syscall name that can expand to an integer constant or named
> function.

If the only place we have dynamic system calls in glibc is the syscall
function, we can add a check there to block system calls using it unless
the symbol has been bound before.  Together with BTI/IBT, this would
prevent issuing arbitrary system calls using machine code fragments from
glibc.

Not sure if that is important consideration.  If it does not matter, we
could change the pthread code to call an internal alias of the syscall
function instead.

Thanks,
Florian

Re: [PATCH v6 0/3] x86_64: aarch64: Set call number just before syscall

[Joe Simmons-Talbott]

Is there anything further I need to do to get this patchset reviewed?

Thanks,
Joe
On Mon, Apr 24, 2023 at 11:03:50AM -0400, Joe Simmons-Talbott wrote:
> To make binary call tree analysis easier place the syscall number into
> the register just before the syscall is made.  Only do this if the
> syscall number is a constant integer.
> 
> Compiler optimizations can place quite a few instructions between the
> setting of the syscall number and the syscall instruction.  During call
> tree analysis the number of instructions between the two can lead to
> more difficulty for both tools and humans in properly identifying the
> syscall number.  Having the syscall number set in the prior instruction
> to the syscall instruction makes this task easier and less error prone.
> Being able to reliably identify syscalls made by a given API will make
> it easier to understand and verify the safety and security of glibc.
> 
> Tested on x86_64 and aarch64.
> 
> Chnages to v5:
>   * aarch64: Remove '\n\t' from asm.
>   * x86_64: Remove unused macros.
> 
> Changes to v4:
>   * aarch64: Move generated comment.  Remove unused macros from earlier
>   version.
> 
>   * Added patch
>     - nptl: Convert indirect syscall numbers to compile time numeric
>       values.
> 
> Changes to v3:
>   * x86_64: Combine two inline asms into one.
>     - Suggested by "H.J. Lu" <hjl.tools@gmail.com>
> 
> Changes to v2:
>   * Add a more detailed commit message.
>   
> Changes to v1:
>   * aarch64: Combine two inline asms into one.  Avoid loading name twice.
> 
> Joe Simmons-Talbott (3):
>   x86_64: Set the syscall register right before doing the syscall.
>   aarch64: Set the syscall register right before doing the syscall.
>   nptl: Use direct syscall numbers in setxid
> 
>  nptl/nptl_setxid.c                       | 136 ++++++++++++++++++++++-
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h |  29 +++--
>  sysdeps/unix/sysv/linux/x86_64/sysdep.h  |  24 ++--
>  3 files changed, 164 insertions(+), 25 deletions(-)
> 
> -- 
> 2.39.2
> 

Re: [PATCH v6 2/3] aarch64: Set the syscall register right before doing the syscall.

[Szabolcs Nagy]

The 04/24/2023 11:03, Joe Simmons-Talbott via Libc-alpha wrote:
> To make identifying syscalls easier during call tree analysis load the
> syscall number just before performing the syscall.
> 
> Compiler optimizations can place quite a few instructions between the
> setting of the syscall number and the syscall instruction.  During call
> tree analysis the number of instructions between the two can lead to
> more difficulty for both tools and humans in properly identifying the
> syscall number.  Having the syscall number set in the prior instruction
> to the syscall instruction makes this task easier and less error prone.
> Being able to reliably identify syscalls made by a given API will make
> it easier to understand and verify the safety and security of glibc.

This is OK to commit.

Reviewed-by: Szabolcs Nagy <szabolcs.nagy@arm.com>


> ---
>  sysdeps/unix/sysv/linux/aarch64/sysdep.h | 29 ++++++++++++++++--------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/aarch64/sysdep.h b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> index e94d1703ad..6fe40aaf89 100644
> --- a/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/aarch64/sysdep.h
> @@ -168,15 +168,26 @@
>  # define HAVE_CLONE3_WRAPPER		1
>  
>  # undef INTERNAL_SYSCALL_RAW
> -# define INTERNAL_SYSCALL_RAW(name, nr, args...)		\
> -  ({ long _sys_result;						\
> -     {								\
> -       LOAD_ARGS_##nr (args)					\
> -       register long _x8 asm ("x8") = (name);			\
> -       asm volatile ("svc	0	// syscall " # name     \
> -		     : "=r" (_x0) : "r"(_x8) ASM_ARGS_##nr : "memory");	\
> -       _sys_result = _x0;					\
> -     }								\
> +# define INTERNAL_SYSCALL_RAW(name, nr, args...)			\
> +  ({ long _sys_result;							\
> +     {									\
> +       LOAD_ARGS_##nr (args)						\
> +       if (__builtin_constant_p(name))					\
> +         asm volatile ("mov	x8, %1	// syscall " # name "\n"	\
> +		       "svc	0"					\
> +                       : "=r" (_x0)					\
> +		       : "i" (name) ASM_ARGS_##nr			\
> +		       : "x8", "memory");				\
> +       else								\
> +         {								\
> +           register long _x8 asm ("x8") = (name);			\
> +           asm volatile ("svc	0"					\
> +		         : "=r" (_x0)					\
> +		         : "r"(_x8) ASM_ARGS_##nr			\
> +		         : "memory");					\
> +         }								\
> +       _sys_result = _x0;						\
> +     }									\
>       _sys_result; })
>  
>  # undef INTERNAL_SYSCALL
> -- 
> 2.39.2
> 

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Joe Simmons-Talbott]

Hi H.J.,

Is there anything else you are looking for on x86_64 WRT this patch?

Thanks,
Joe
On Mon, Apr 24, 2023 at 11:03:51AM -0400, Joe Simmons-Talbott wrote:
> To make identifying syscalls easier during call tree analysis load the
> syscall number just before performing the syscall.
> 
> Compiler optimizations can place quite a few instructions between the
> setting of the syscall number and the syscall instruction.  During call
> tree analysis the number of instructions between the two can lead to
> more difficulty for both tools and humans in properly identifying the
> syscall number.  Having the syscall number set in the prior instruction
> to the syscall instruction makes this task easier and less error prone.
> Being able to reliably identify syscalls made by a given API will make
> it easier to understand and verify the safety and security of glibc.
> ---
>  sysdeps/unix/sysv/linux/x86_64/sysdep.h | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> index cfb51be8c5..0db8660531 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> @@ -257,9 +257,9 @@
>      TYPEFY (arg1, __arg1) = ARGIFY (arg1);			 	\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1)						\
> +    : "g" (number), "r" (_a1)						\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -273,9 +273,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2)				\
> +    : "g" (number), "r" (_a1), "r" (_a2)				\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -291,9 +291,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3)			\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3)			\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -311,9 +311,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)		\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)		\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -333,9 +333,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
>        "r" (_a5)								\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
> @@ -358,9 +358,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
>        "r" (_a5), "r" (_a6)						\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
> -- 
> 2.39.2
> 

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[H.J. Lu]

On Mon, May 15, 2023 at 7:15 AM Joe Simmons-Talbott <josimmon@redhat.com> wrote:
>
> Hi H.J.,
>
> Is there anything else you are looking for on x86_64 WRT this patch?
>
> Thanks,
> Joe
> On Mon, Apr 24, 2023 at 11:03:51AM -0400, Joe Simmons-Talbott wrote:
> > To make identifying syscalls easier during call tree analysis load the
> > syscall number just before performing the syscall.
> >
> > Compiler optimizations can place quite a few instructions between the
> > setting of the syscall number and the syscall instruction.  During call
> > tree analysis the number of instructions between the two can lead to
> > more difficulty for both tools and humans in properly identifying the
> > syscall number.  Having the syscall number set in the prior instruction
> > to the syscall instruction makes this task easier and less error prone.
> > Being able to reliably identify syscalls made by a given API will make
> > it easier to understand and verify the safety and security of glibc.
> > ---
> >  sysdeps/unix/sysv/linux/x86_64/sysdep.h | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> > index cfb51be8c5..0db8660531 100644
> > --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> > +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> > @@ -257,9 +257,9 @@
> >      TYPEFY (arg1, __arg1) = ARGIFY (arg1);                           \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1)                                                \
> > +    : "g" (number), "r" (_a1)                                                \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -273,9 +273,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2)                             \
> > +    : "g" (number), "r" (_a1), "r" (_a2)                             \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -291,9 +291,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3)                  \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3)                  \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -311,9 +311,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)               \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)               \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -333,9 +333,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> >        "r" (_a5)                                                              \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> > @@ -358,9 +358,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> >        "r" (_a5), "r" (_a6)                                           \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> > --
> > 2.39.2
> >
>

I have no more comments.

-- 
H.J.

Re: [PATCH v6 3/3] nptl: Use direct syscall numbers in setxid

[Joe Simmons-Talbott]

ping.

Thanks,
Joe

On Mon, Apr 24, 2023 at 11:03:53AM -0400, Joe Simmons-Talbott wrote:
> Make all internal glibc syscalls use direct compile time numeric values
> rather than variables.  This will make the syscall number easier to
> identify during static analysis.
> ---
>  nptl/nptl_setxid.c | 136 +++++++++++++++++++++++++++++++++++++++++++--
>  1 file changed, 132 insertions(+), 4 deletions(-)
> 
> diff --git a/nptl/nptl_setxid.c b/nptl/nptl_setxid.c
> index 4bfcfe4188..797015def1 100644
> --- a/nptl/nptl_setxid.c
> +++ b/nptl/nptl_setxid.c
> @@ -66,8 +66,72 @@ __nptl_setxid_sighandler (int sig, siginfo_t *si, void *ctx)
>        || si->si_code != SI_TKILL)
>      return;
>  
> -  result = INTERNAL_SYSCALL_NCS (xidcmd->syscall_no, 3, xidcmd->id[0],
> -				 xidcmd->id[1], xidcmd->id[2]);
> +  switch(xidcmd->syscall_no)
> +  {
> +#ifdef __NR_setuid32
> +    case __NR_setuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid32, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setuid32 */
> +    case __NR_setuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setreuid32
> +    case __NR_setreuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid32, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setreuid32 */
> +    case __NR_setreuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setresgid32
> +    case __NR_setresgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid32, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setresgid32 */
> +    case __NR_setresgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setregid32
> +    case __NR_setregid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid32, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setregid32 */
> +    case __NR_setregid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setgid32
> +    case __NR_setgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid32, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setgid32 */
> +    case __NR_setgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#ifdef __NR_setgroups32
> +    case __NR_setgroups32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups32, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +#endif /* __NR_setgroups32 */
> +    case __NR_setgroups:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +      break;
> +   default:
> +      result = INTERNAL_SYSCALL_NCS (xidcmd->syscall_no, 3, xidcmd->id[0],
> +				     xidcmd->id[1], xidcmd->id[2]);
> +  }
>    int error = 0;
>    if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
>      error = INTERNAL_SYSCALL_ERRNO (result);
> @@ -262,8 +326,72 @@ __nptl_setxid (struct xid_command *cmdp)
>  
>    /* This must be last, otherwise the current thread might not have
>       permissions to send SIGSETXID syscall to the other threads.  */
> -  result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3,
> -                                 cmdp->id[0], cmdp->id[1], cmdp->id[2]);
> +  switch(cmdp->syscall_no)
> +  {
> +#ifdef __NR_setuid32
> +    case __NR_setuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid32, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setuid32 */
> +    case __NR_setuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setuid, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setreuid32
> +    case __NR_setreuid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid32, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setreuid32 */
> +    case __NR_setreuid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setreuid, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setresgid32
> +    case __NR_setresgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid32, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setresgid32 */
> +    case __NR_setresgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setresgid, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setregid32
> +    case __NR_setregid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid32, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setregid32 */
> +    case __NR_setregid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setregid, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setgid32
> +    case __NR_setgid32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid32, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setgid32 */
> +    case __NR_setgid:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgid, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#ifdef __NR_setgroups32
> +    case __NR_setgroups32:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups32, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +#endif /* __NR_setgroups32 */
> +    case __NR_setgroups:
> +      result = INTERNAL_SYSCALL_NCS (__NR_setgroups, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +      break;
> +   default:
> +      result = INTERNAL_SYSCALL_NCS (cmdp->syscall_no, 3, cmdp->id[0],
> +				     cmdp->id[1], cmdp->id[2]);
> +  }
>    int error = 0;
>    if (__glibc_unlikely (INTERNAL_SYSCALL_ERROR_P (result)))
>      {
> -- 
> 2.39.2
> 

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Joe Simmons-Talbott]

ping.

Thanks,
Joe
On Mon, Apr 24, 2023 at 11:03:51AM -0400, Joe Simmons-Talbott wrote:
> To make identifying syscalls easier during call tree analysis load the
> syscall number just before performing the syscall.
> 
> Compiler optimizations can place quite a few instructions between the
> setting of the syscall number and the syscall instruction.  During call
> tree analysis the number of instructions between the two can lead to
> more difficulty for both tools and humans in properly identifying the
> syscall number.  Having the syscall number set in the prior instruction
> to the syscall instruction makes this task easier and less error prone.
> Being able to reliably identify syscalls made by a given API will make
> it easier to understand and verify the safety and security of glibc.
> ---
>  sysdeps/unix/sysv/linux/x86_64/sysdep.h | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> index cfb51be8c5..0db8660531 100644
> --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> @@ -257,9 +257,9 @@
>      TYPEFY (arg1, __arg1) = ARGIFY (arg1);			 	\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1)						\
> +    : "g" (number), "r" (_a1)						\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -273,9 +273,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2)				\
> +    : "g" (number), "r" (_a1), "r" (_a2)				\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -291,9 +291,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3)			\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3)			\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -311,9 +311,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)		\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)		\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
>  })
> @@ -333,9 +333,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
>        "r" (_a5)								\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
> @@ -358,9 +358,9 @@
>      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;			\
>      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;			\
>      asm volatile (							\
> -    "syscall\n\t"							\
> +    "movl %1, %k0\n\tsyscall\n\t"					\
>      : "=a" (resultvar)							\
> -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
> +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),		\
>        "r" (_a5), "r" (_a6)						\
>      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);			\
>      (long int) resultvar;						\
> -- 
> 2.39.2
> 

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Noah Goldstein]

On Thu, May 25, 2023 at 1:07 PM Joe Simmons-Talbott via Libc-alpha
<libc-alpha@sourceware.org> wrote:
>
> ping.
>
> Thanks,
> Joe
> On Mon, Apr 24, 2023 at 11:03:51AM -0400, Joe Simmons-Talbott wrote:
> > To make identifying syscalls easier during call tree analysis load the
> > syscall number just before performing the syscall.
> >
> > Compiler optimizations can place quite a few instructions between the
> > setting of the syscall number and the syscall instruction.  During call
> > tree analysis the number of instructions between the two can lead to
> > more difficulty for both tools and humans in properly identifying the
> > syscall number.  Having the syscall number set in the prior instruction
> > to the syscall instruction makes this task easier and less error prone.
> > Being able to reliably identify syscalls made by a given API will make
> > it easier to understand and verify the safety and security of glibc.
> > ---
> >  sysdeps/unix/sysv/linux/x86_64/sysdep.h | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> >
> > diff --git a/sysdeps/unix/sysv/linux/x86_64/sysdep.h b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> > index cfb51be8c5..0db8660531 100644
> > --- a/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> > +++ b/sysdeps/unix/sysv/linux/x86_64/sysdep.h
> > @@ -257,9 +257,9 @@
> >      TYPEFY (arg1, __arg1) = ARGIFY (arg1);                           \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1)                                                \
> > +    : "g" (number), "r" (_a1)                                                \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -273,9 +273,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2)                             \
> > +    : "g" (number), "r" (_a1), "r" (_a2)                             \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -291,9 +291,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3)                  \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3)                  \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -311,9 +311,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)               \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4)               \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> >  })
> > @@ -333,9 +333,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> >        "r" (_a5)                                                              \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> > @@ -358,9 +358,9 @@
> >      register TYPEFY (arg2, _a2) asm ("rsi") = __arg2;                        \
> >      register TYPEFY (arg1, _a1) asm ("rdi") = __arg1;                        \
> >      asm volatile (                                                   \
> > -    "syscall\n\t"                                                    \
> > +    "movl %1, %k0\n\tsyscall\n\t"                                    \
> >      : "=a" (resultvar)                                                       \
> > -    : "0" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> > +    : "g" (number), "r" (_a1), "r" (_a2), "r" (_a3), "r" (_a4),              \
> >        "r" (_a5), "r" (_a6)                                           \
> >      : "memory", REGISTERS_CLOBBERED_BY_SYSCALL);                     \
> >      (long int) resultvar;                                            \
> > --
> > 2.39.2
> >
>

I'm minorly opposed to this patch. Even if GLIBC guarantees all
syscalls will set the number the instruction before, that's no guarantee
for the entire program. Furthermore in the event of:
   `movl $VAL, %eax; syscall`
It's still not safe to *always* assume that `VAL` correspond to the
syscall number as a jump (direct or indirect) could still go between
the instructions (i.e there is no guarantee in the assembly that the
`mov` dominates the `syscall).
So at the end of the day, we are bloating the library without, AFAICT,
providing any real guarantee. Maybe I'm missing something?

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Florian Weimer]

* Noah Goldstein via Libc-alpha:

> I'm minorly opposed to this patch. Even if GLIBC guarantees all
> syscalls will set the number the instruction before, that's no guarantee
> for the entire program. Furthermore in the event of:
>    `movl $VAL, %eax; syscall`
> It's still not safe to *always* assume that `VAL` correspond to the
> syscall number as a jump (direct or indirect) could still go between
> the instructions (i.e there is no guarantee in the assembly that the
> `mov` dominates the `syscall).
> So at the end of the day, we are bloating the library without, AFAICT,
> providing any real guarantee. Maybe I'm missing something?

Joe, is there a size change to libc.so.6 as the result of this change?

Thanks,
Florian

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Joe Simmons-Talbott]

On Fri, May 26, 2023 at 09:04:06AM +0200, Florian Weimer wrote:
> * Noah Goldstein via Libc-alpha:
> 
> > I'm minorly opposed to this patch. Even if GLIBC guarantees all
> > syscalls will set the number the instruction before, that's no guarantee
> > for the entire program. Furthermore in the event of:
> >    `movl $VAL, %eax; syscall`
> > It's still not safe to *always* assume that `VAL` correspond to the
> > syscall number as a jump (direct or indirect) could still go between
> > the instructions (i.e there is no guarantee in the assembly that the
> > `mov` dominates the `syscall).
> > So at the end of the day, we are bloating the library without, AFAICT,
> > providing any real guarantee. Maybe I'm missing something?
> 
> Joe, is there a size change to libc.so.6 as the result of this change?

No, the size is the same with and with out this patchset on x86_64.

Thanks,
Joe

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Noah Goldstein]

On Fri, May 26, 2023 at 5:59 AM Joe Simmons-Talbott <josimmon@redhat.com> wrote:
>
> On Fri, May 26, 2023 at 09:04:06AM +0200, Florian Weimer wrote:
> > * Noah Goldstein via Libc-alpha:
> >
> > > I'm minorly opposed to this patch. Even if GLIBC guarantees all
> > > syscalls will set the number the instruction before, that's no guarantee
> > > for the entire program. Furthermore in the event of:
> > >    `movl $VAL, %eax; syscall`
> > > It's still not safe to *always* assume that `VAL` correspond to the
> > > syscall number as a jump (direct or indirect) could still go between
> > > the instructions (i.e there is no guarantee in the assembly that the
> > > `mov` dominates the `syscall).
> > > So at the end of the day, we are bloating the library without, AFAICT,
> > > providing any real guarantee. Maybe I'm missing something?
> >
> > Joe, is there a size change to libc.so.6 as the result of this change?
>
> No, the size is the same with and with out this patchset on x86_64.
>
There aren't many syscalls so it's only a minor cost (hence the only
minor opposition), but I don't see the value this provides given that it
still won't be safe to assume the syscall number is always set the
instruction beforehand for any robust purpose. So it still feels like
why take any cost at all?

> Thanks,
> Joe
>

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Florian Weimer]

* Noah Goldstein:

> On Fri, May 26, 2023 at 5:59 AM Joe Simmons-Talbott <josimmon@redhat.com> wrote:
>>
>> On Fri, May 26, 2023 at 09:04:06AM +0200, Florian Weimer wrote:
>> > * Noah Goldstein via Libc-alpha:
>> >
>> > > I'm minorly opposed to this patch. Even if GLIBC guarantees all
>> > > syscalls will set the number the instruction before, that's no guarantee
>> > > for the entire program. Furthermore in the event of:
>> > >    `movl $VAL, %eax; syscall`
>> > > It's still not safe to *always* assume that `VAL` correspond to the
>> > > syscall number as a jump (direct or indirect) could still go between
>> > > the instructions (i.e there is no guarantee in the assembly that the
>> > > `mov` dominates the `syscall).
>> > > So at the end of the day, we are bloating the library without, AFAICT,
>> > > providing any real guarantee. Maybe I'm missing something?
>> >
>> > Joe, is there a size change to libc.so.6 as the result of this change?
>>
>> No, the size is the same with and with out this patchset on x86_64.
>>
> There aren't many syscalls so it's only a minor cost (hence the only
> minor opposition), but I don't see the value this provides given that it
> still won't be safe to assume the syscall number is always set the
> instruction beforehand for any robust purpose. So it still feels like
> why take any cost at all?

I think there is any run-time cost at all, only the increased source
complexity.

It's much easier to change glibc than to add full register tracking to a
the static analysis tool that discovers system calls in the disassembly.

Thanks,
Florian

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Noah Goldstein]

On Tue, May 30, 2023 at 5:13 AM Florian Weimer <fweimer@redhat.com> wrote:
>
> * Noah Goldstein:
>
> > On Fri, May 26, 2023 at 5:59 AM Joe Simmons-Talbott <josimmon@redhat.com> wrote:
> >>
> >> On Fri, May 26, 2023 at 09:04:06AM +0200, Florian Weimer wrote:
> >> > * Noah Goldstein via Libc-alpha:
> >> >
> >> > > I'm minorly opposed to this patch. Even if GLIBC guarantees all
> >> > > syscalls will set the number the instruction before, that's no guarantee
> >> > > for the entire program. Furthermore in the event of:
> >> > >    `movl $VAL, %eax; syscall`
> >> > > It's still not safe to *always* assume that `VAL` correspond to the
> >> > > syscall number as a jump (direct or indirect) could still go between
> >> > > the instructions (i.e there is no guarantee in the assembly that the
> >> > > `mov` dominates the `syscall).
> >> > > So at the end of the day, we are bloating the library without, AFAICT,
> >> > > providing any real guarantee. Maybe I'm missing something?
> >> >
> >> > Joe, is there a size change to libc.so.6 as the result of this change?
> >>
> >> No, the size is the same with and with out this patchset on x86_64.
> >>
> > There aren't many syscalls so it's only a minor cost (hence the only
> > minor opposition), but I don't see the value this provides given that it
> > still won't be safe to assume the syscall number is always set the
> > instruction beforehand for any robust purpose. So it still feels like
> > why take any cost at all?
>
> I think there is any run-time cost at all, only the increased source
> complexity.
>
> It's much easier to change glibc than to add full register tracking to a
> the static analysis tool that discovers system calls in the disassembly.
>

Is the aim only to verify libc.so or to verify arbitrary binaries? If the
former then sure I guess we know we only use the syscall wrapper
so this may help (more on that), but if it's arbitrary binaries there
is no guarantee they are using the GLIBC syscall wrapper for their
syscalls.

If it really is just GLIBC then this change seems unnecessary. Even
if there can be separation between setting rax and the syscall, the
way we have the wrappers setup we know there will always be a
dominating write to rax with the syscall number so would rather see
the case where that isn't trivial to find as a motivator first. Or we could
just do source code level analysis as we will always have the
syscall number in macro invocation.

> Thanks,
> Florian
>

Re: [PATCH v6 1/3] x86_64: Set the syscall register right before doing the syscall.

[Joe Simmons-Talbott]

On Wed, May 31, 2023 at 01:23:44PM -0500, Noah Goldstein wrote:
> On Tue, May 30, 2023 at 5:13 AM Florian Weimer <fweimer@redhat.com> wrote:
> >
> > * Noah Goldstein:
> >
> > > On Fri, May 26, 2023 at 5:59 AM Joe Simmons-Talbott <josimmon@redhat.com> wrote:
> > >>
> > >> On Fri, May 26, 2023 at 09:04:06AM +0200, Florian Weimer wrote:
> > >> > * Noah Goldstein via Libc-alpha:
> > >> >
> > >> > > I'm minorly opposed to this patch. Even if GLIBC guarantees all
> > >> > > syscalls will set the number the instruction before, that's no guarantee
> > >> > > for the entire program. Furthermore in the event of:
> > >> > >    `movl $VAL, %eax; syscall`
> > >> > > It's still not safe to *always* assume that `VAL` correspond to the
> > >> > > syscall number as a jump (direct or indirect) could still go between
> > >> > > the instructions (i.e there is no guarantee in the assembly that the
> > >> > > `mov` dominates the `syscall).
> > >> > > So at the end of the day, we are bloating the library without, AFAICT,
> > >> > > providing any real guarantee. Maybe I'm missing something?
> > >> >
> > >> > Joe, is there a size change to libc.so.6 as the result of this change?
> > >>
> > >> No, the size is the same with and with out this patchset on x86_64.
> > >>
> > > There aren't many syscalls so it's only a minor cost (hence the only
> > > minor opposition), but I don't see the value this provides given that it
> > > still won't be safe to assume the syscall number is always set the
> > > instruction beforehand for any robust purpose. So it still feels like
> > > why take any cost at all?
> >
> > I think there is any run-time cost at all, only the increased source
> > complexity.
> >
> > It's much easier to change glibc than to add full register tracking to a
> > the static analysis tool that discovers system calls in the disassembly.
> >
> 
> Is the aim only to verify libc.so or to verify arbitrary binaries? If the
> former then sure I guess we know we only use the syscall wrapper
> so this may help (more on that), but if it's arbitrary binaries there
> is no guarantee they are using the GLIBC syscall wrapper for their
> syscalls.
> 
> If it really is just GLIBC then this change seems unnecessary. Even
> if there can be separation between setting rax and the syscall, the
> way we have the wrappers setup we know there will always be a
> dominating write to rax with the syscall number so would rather see
> the case where that isn't trivial to find as a motivator first. Or we could
> just do source code level analysis as we will always have the
> syscall number in macro invocation.
> 

Right now we are only concerned with libc.so.  Here's an example case
from my installed libc.so.

000000000003ec70 <__GI___arc4random_buf.part.0>:
   3ec70:       55                      push   %rbp
   3ec71:       41 b8 3e 01 00 00       mov    $0x13e,%r8d
   3ec77:       48 89 e5                mov    %rsp,%rbp
   3ec7a:       41 56                   push   %r14
   3ec7c:       41 55                   push   %r13
   3ec7e:       41 54                   push   %r12
   3ec80:       49 89 fc                mov    %rdi,%r12
   3ec83:       53                      push   %rbx
   3ec84:       48 89 f3                mov    %rsi,%rbx
   3ec87:       48 83 ec 10             sub    $0x10,%rsp
   3ec8b:       64 48 8b 04 25 28 00    mov    %fs:0x28,%rax
   3ec92:       00 00 
   3ec94:       48 89 45 d8             mov    %rax,-0x28(%rbp)
   3ec98:       31 c0                   xor    %eax,%eax
   3ec9a:       31 d2                   xor    %edx,%edx
   3ec9c:       48 89 de                mov    %rbx,%rsi
   3ec9f:       4c 89 e7                mov    %r12,%rdi
   3eca2:       44 89 c0                mov    %r8d,%eax
   3eca5:       0f 05                   syscall

And with my patchset:

0000000000039480 <__GI___arc4random_buf.part.0>:
   39480:       41 55                   push   %r13
   39482:       41 54                   push   %r12
   39484:       55                      push   %rbp
   39485:       48 89 fd                mov    %rdi,%rbp
   39488:       53                      push   %rbx
   39489:       48 89 f3                mov    %rsi,%rbx
   3948c:       48 83 ec 18             sub    $0x18,%rsp
   39490:       31 d2                   xor    %edx,%edx
   39492:       48 89 de                mov    %rbx,%rsi
   39495:       48 89 ef                mov    %rbp,%rdi
   39498:       b8 3e 01 00 00          mov    $0x13e,%eax
   3949d:       0f 05                   syscall 


Thanks,
Joe