Warn for execve syscall with argv or argv[0] being NULL.

For execve valgrind would silently fail when argv was NULL or
unadressable. Make sure that this produces a warning under memcheck.

The linux kernel accepts argv[0] being NULL, but most other kernels
don't since posix says it should be non-NULL and it causes argc to
be zero which is unexpected and might cause security issues.

This adjusts some testcases so they don't rely on execve succeeding
when argv is NULL and expect warnings about argv or argv[0] being
NULL or unaddressable.

https://bugs.kde.org/show_bug.cgi?id=450437

diff --git a/NEWS b/NEWS
index 730f2b5ff8b7a53d0fa8ced1856d6b86eae6068d..924032b3c102ae9249c83ae1c055f6c72137298b 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -93,6 +93,7 @@ are not entered into bugzilla tend to get forgotten about or ignored.
 449838  sigsegv liburing the 'impossible' happened for io_uring_setup
 450025  Powerc: ACC file not implemented as a logical overlay of the VSR
         registers.
+450437  Warn for execve syscall with argv or argv[0] being NULL
 450536  Powerpc: valgrind throws 'facility scv unavailable exception'
 451626  Syscall param bpf(attr->raw_tracepoint.name) points to unaddressable byte(s)
 451827  [ppc64le] VEX temporary storage exhausted with several vbpermq instructions

diff --git a/coregrind/m_syswrap/syswrap-generic.c b/coregrind/m_syswrap/syswrap-generic.c
index bc3fa6fe9ff8affe4684941ba68df823f838b73f..44a60bf12828eb4c8fdc4e30ad8088346d78a92a 100644 (file)
--- a/coregrind/m_syswrap/syswrap-generic.c
+++ b/coregrind/m_syswrap/syswrap-generic.c
@@ -2933,6 +2933,7 @@ void handle_pre_sys_execve(ThreadId tid, SyscallStatus *status, Addr pathname,
    Bool         setuid_allowed, trace_this_child;
    const char   *str;
    char         str2[30], str3[30];
+   Addr         arg_2_check = arg_2;
 
    switch (execveType) {
    case EXECVE:
@@ -2951,15 +2952,26 @@ void handle_pre_sys_execve(ThreadId tid, SyscallStatus *status, Addr pathname,
    VG_(strcpy)(str2, str);
    VG_(strcpy)(str3, str);
 
-   if (arg_2 != 0) {
-       /* At least the terminating NULL must be addressable. */
-      if (!ML_(safe_to_deref)((HChar **) (Addr)arg_2, sizeof(HChar *))) {
-         SET_STATUS_Failure(VKI_EFAULT);
-         return;
+   VG_(strcat)(str2, "(argv)");
+   VG_(strcat)(str3, "(argv[0])");
+
+   /* argv[] should not be NULL and valid.  */
+   PRE_MEM_READ(str2, arg_2_check, sizeof(Addr));
+
+   /* argv[0] should not be NULL and valid.  */
+   if (ML_(safe_to_deref)((HChar **) (Addr)arg_2_check, sizeof(HChar *))) {
+      Addr argv0 = *(Addr*)arg_2_check;
+      PRE_MEM_RASCIIZ( str3, argv0 );
+      /* The rest of argv can be NULL or a valid string pointer.  */
+      if (VG_(am_is_valid_for_client)(arg_2_check, sizeof(HChar), VKI_PROT_READ)) {
+         arg_2_check += sizeof(HChar*);
+         str3[VG_(strlen)(str)] = '\0';
+         VG_(strcat)(str3, "(argv[i])");
+         ML_(pre_argv_envp)( arg_2_check, tid, str2, str3 );
       }
-      VG_(strcat)(str2, "(argv)");
-      VG_(strcat)(str3, "(argv[i])");
-      ML_(pre_argv_envp)( arg_2, tid, str2, str3 );
+   } else {
+      SET_STATUS_Failure(VKI_EFAULT);
+      return;
    }
    // Reset helper strings to syscall name.
    str2[VG_(strlen)(str)] = '\0';

diff --git a/memcheck/tests/arm64-linux/scalar.stderr.exp b/memcheck/tests/arm64-linux/scalar.stderr.exp
index 66975efcb2bc8c125565d0b0accceaed80d65bf2..4c81819b64f7ee435e878282ea155b31b24c6995 100644 (file)
--- a/memcheck/tests/arm64-linux/scalar.stderr.exp
+++ b/memcheck/tests/arm64-linux/scalar.stderr.exp
@@ -75,6 +75,11 @@ Syscall param execve(filename) points to unaddressable byte(s)
    by 0x........: main (scalar.c:91)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd
 
+Syscall param execve(argv) points to unaddressable byte(s)
+   ...
+   by 0x........: main (scalar.c:91)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+
 -----------------------------------------------------
  49:          __NR_chdir 1s 1m
 -----------------------------------------------------
@@ -576,13 +581,13 @@ Syscall param getpriority(who) contains uninitialised byte(s)
 -----------------------------------------------------
 140:    __NR_setpriority 3s 0m
 -----------------------------------------------------
+
+More than 100 errors detected.  Subsequent errors
+will still be recorded, but in less detail than before.
 Syscall param setpriority(which) contains uninitialised byte(s)
    ...
    by 0x........: main (scalar.c:458)
 
-
-More than 100 errors detected.  Subsequent errors
-will still be recorded, but in less detail than before.
 Syscall param setpriority(who) contains uninitialised byte(s)
    ...
    by 0x........: main (scalar.c:458)

diff --git a/memcheck/tests/execve1.c b/memcheck/tests/execve1.c
index 83e058a2f33327054bbd85af3c6cccdefee7d2e4..df36f145e057e67cee8a85f4453b31ce4652eac8 100644 (file)
--- a/memcheck/tests/execve1.c
+++ b/memcheck/tests/execve1.c
@@ -4,7 +4,7 @@ int main(void)
 {
    char* null_filename = NULL;
    char* bad[2]  = { (char*)1, NULL };
-   char* good[1] = {           NULL };
+   char* good[2] = { "true",   NULL };
 
    execve(null_filename, bad, bad);
    execve("/bin/true", good, good);

diff --git a/memcheck/tests/execve1.stderr.exp b/memcheck/tests/execve1.stderr.exp
index 37a91b83a3c36a617763eb26d99bcc0bdd20cb19..eebc1e5ebd4419af2a97995a297702a6d0378032 100644 (file)
--- a/memcheck/tests/execve1.stderr.exp
+++ b/memcheck/tests/execve1.stderr.exp
@@ -3,7 +3,7 @@ Syscall param execve(filename) points to unaddressable byte(s)
    by 0x........: main (execve1.c:9)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd
 
-Syscall param execve(argv[i]) points to unaddressable byte(s)
+Syscall param execve(argv[0]) points to unaddressable byte(s)
    ...
    by 0x........: main (execve1.c:9)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd

diff --git a/memcheck/tests/execve2.stderr.exp b/memcheck/tests/execve2.stderr.exp
index cd98593f7c5f1b8a437bb0c541c992c8d0c6eeb4..f9d7c359269a82a8a9faf6f95aef999691b4335a 100644 (file)
--- a/memcheck/tests/execve2.stderr.exp
+++ b/memcheck/tests/execve2.stderr.exp
@@ -3,3 +3,8 @@ Syscall param execve(filename) points to unaddressable byte(s)
    by 0x........: main (execve2.c:9)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd
 
+Syscall param execve(argv) points to unaddressable byte(s)
+   ...
+   by 0x........: main (execve2.c:9)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+

diff --git a/memcheck/tests/linux/sys-execveat.stderr.exp b/memcheck/tests/linux/sys-execveat.stderr.exp
index a58b0fb6aedf2bc7346dd7422ce862b36efead08..b49b9be9810008bbb0138cb7ffccce6b6c6d5c0a 100644 (file)
--- a/memcheck/tests/linux/sys-execveat.stderr.exp
+++ b/memcheck/tests/linux/sys-execveat.stderr.exp
@@ -17,3 +17,15 @@ Syscall param execveat(argv) points to uninitialised byte(s)
    at 0x........: malloc (vg_replace_malloc.c:...)
    by 0x........: main (sys-execveat.c:41)
 
+Syscall param execveat(argv[0]) points to unaddressable byte(s)
+   ...
+   by 0x........: sys_execveat (sys-execveat.c:16)
+   by 0x........: main (sys-execveat.c:51)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+
+Syscall param execveat(argv) points to unaddressable byte(s)
+   ...
+   by 0x........: sys_execveat (sys-execveat.c:16)
+   by 0x........: main (sys-execveat.c:52)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+

diff --git a/memcheck/tests/x86-linux/scalar.c b/memcheck/tests/x86-linux/scalar.c
index 52f0d4e3536fe44f95fe49c0ebb6412f71aa6de2..54d0e0443ae2c77197fee16f42a2557aef6ac5b0 100644 (file)
--- a/memcheck/tests/x86-linux/scalar.c
+++ b/memcheck/tests/x86-linux/scalar.c
@@ -95,9 +95,9 @@ int main(void)
    char *argv_envp[] = {(char *) (x0 + 1), NULL};
    GO(__NR_execve, "4s 2m");
    SY(__NR_execve, x0 + 1, x0 + argv_envp, x0); FAIL;
-
+   char *argv_ok[] = {"frob", NULL};
    GO(__NR_execve, "4s 2m");
-   SY(__NR_execve, x0 + 1, x0, x0 + argv_envp); FAIL;
+   SY(__NR_execve, x0 + 1, x0 + argv_ok, x0 + argv_envp); FAIL;
 
    // __NR_chdir 12
    GO(__NR_chdir, "1s 1m");

diff --git a/memcheck/tests/x86-linux/scalar.stderr.exp b/memcheck/tests/x86-linux/scalar.stderr.exp
index 470023f0e0226d80039c2abad94be5f3636b550c..b9202a8c2f9cacf8c8812671584c3b2aa599d6f5 100644 (file)
--- a/memcheck/tests/x86-linux/scalar.stderr.exp
+++ b/memcheck/tests/x86-linux/scalar.stderr.exp
@@ -170,6 +170,11 @@ Syscall param execve(filename) points to unaddressable byte(s)
    by 0x........: main (scalar.c:90)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd
 
+Syscall param execve(argv) points to unaddressable byte(s)
+   ...
+   by 0x........: main (scalar.c:90)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+
 -----------------------------------------------------
  11:         __NR_execve 3s 1m
 -----------------------------------------------------
@@ -190,6 +195,11 @@ Syscall param execve(filename) points to unaddressable byte(s)
    by 0x........: main (scalar.c:93)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd
 
+Syscall param execve(argv) points to unaddressable byte(s)
+   ...
+   by 0x........: main (scalar.c:93)
+ Address 0x........ is not stack'd, malloc'd or (recently) free'd
+
 -----------------------------------------------------
  11:         __NR_execve 4s 2m
 -----------------------------------------------------
@@ -216,7 +226,7 @@ Syscall param execve(argv) points to uninitialised byte(s)
  Address 0x........ is on thread 1's stack
  in frame #1, created by main (scalar.c:29)
 
-Syscall param execve(argv[i]) points to unaddressable byte(s)
+Syscall param execve(argv[0]) points to unaddressable byte(s)
    ...
    by 0x........: main (scalar.c:97)
  Address 0x........ is not stack'd, malloc'd or (recently) free'd
@@ -564,6 +574,9 @@ Syscall param pipe(filedes) contains uninitialised byte(s)
    ...
    by 0x........: main (scalar.c:225)
 
+
+More than 100 errors detected.  Subsequent errors
+will still be recorded, but in less detail than before.
 Syscall param pipe(filedes) points to unaddressable byte(s)
    ...
    by 0x........: main (scalar.c:225)
@@ -576,9 +589,6 @@ Syscall param times(buf) contains uninitialised byte(s)
    ...
    by 0x........: main (scalar.c:229)
 
-
-More than 100 errors detected.  Subsequent errors
-will still be recorded, but in less detail than before.
 Syscall param times(buf) points to unaddressable byte(s)
    ...
    by 0x........: main (scalar.c:229)

diff --git a/none/tests/execve.c b/none/tests/execve.c
index 950842da29adb209794a8324b89b54cef5ef3b15..a1af72fd9e4924711ade0b48e59bb68d3f680cbc 100644 (file)
--- a/none/tests/execve.c
+++ b/none/tests/execve.c
@@ -7,20 +7,42 @@ int main(int argc, char **argv)
    if (argc == 1)
    {
       // This tests the case where argv and envp are NULL, which is easy to
-      // get wrong because it's an unusual case.
+      // get wrong because it's an unusual case. It is also bad and only
+      // "worked" by accident with the linux kernel.
 
-#if defined(VGO_solaris)
-      // Solaris requires non-NULL argv parameter
       char *const argv_exe[] = {"true", NULL};
-      if (execve("/bin/true", argv_exe, NULL) < 0)
+      char *const v_null[] = { NULL };
+      char *const v_minus_one[] = { (char *const) -1, NULL };
+
+#if defined(VGO_solaris)
+      const char *exe = "/bin/true";
 #elif defined(VGO_darwin)
-      if (execve("/usr/bin/true", NULL, NULL) < 0)
+      const char *exe = "/usr/bin/true";
 #elif defined(VGO_freebsd)
-      char *const argv_exe[] = {"true", NULL};
-      if (execve("/usr/bin/true", argv_exe, NULL) < 0)
+      const char *exe = "/usr/bin/true";
 #else
-      if (execve("/bin/true", NULL, NULL) < 0)
+      const char *exe = "/bin/true";
 #endif
+
+      /* Try some bad argv and envp arguments, make sure the executable
+        doesn't actually exists, so execve doesn't accidentally succeeds.  */
+      if (execve("/%/", NULL, NULL) >= 0)
+       printf ("WHAT?");
+      if (execve("/%/", (void *)-1, NULL) >= 0)
+       printf ("WHAT?");
+      if (execve("/%/", v_null, NULL) >= 0)
+       printf ("WHAT?");
+      if (execve("/%/", v_null, v_null) >= 0)
+       printf ("WHAT?");
+      if (execve("/%/", v_minus_one, NULL) >= 0)
+       printf ("WHAT?");
+      if (execve("/%/", v_minus_one, v_null) >= 0)
+       printf ("WHAT?");
+      if (execve("/%/", v_minus_one, v_minus_one) >= 0)
+       printf ("WHAT?");
+
+      /* Finally a correct execve.  */
+      if (execve(exe, argv_exe, NULL) < 0)
       {
          perror("execve");
          exit(1);