[Valgrind-developers] [valgrind] FreeBSD syscall: harden sysctl kern.proc.pathname

[Paul F. <pa...@so...>]

https://sourceware.org/cgit/valgrind/commit/?id=924c0eadf1a948292ff1eeab162f85a3b2da3eb0

commit 924c0eadf1a948292ff1eeab162f85a3b2da3eb0
Author: Paul Floyd <pj...@wa...>
Date:   Thu Jul 3 22:06:49 2025 +0200

    FreeBSD syscall: harden sysctl kern.proc.pathname
    
    Add handling for NULL len pointer and not enough space
    for path. Also extend the bug470713 with a few more checks.
    
    Need to add some more inaccessible memory checks.

Diff:
---
 coregrind/m_syswrap/syswrap-freebsd.c       | 45 +++++++++++++++++++----------
 memcheck/tests/freebsd/bug470713.cpp        | 42 ++++++++++++++++++++-------
 memcheck/tests/freebsd/bug470713.stdout.exp |  6 ++--
 3 files changed, 66 insertions(+), 27 deletions(-)

diff --git a/coregrind/m_syswrap/syswrap-freebsd.c b/coregrind/m_syswrap/syswrap-freebsd.c
index 3f4b6ca712..22053da7f5 100644
--- a/coregrind/m_syswrap/syswrap-freebsd.c
+++ b/coregrind/m_syswrap/syswrap-freebsd.c
@@ -1927,29 +1927,40 @@ static void sysctl_kern_usrstack(SizeT* out, SizeT* outlen)
    *outlen = sizeof(ULong);
 }
 
-static Bool sysctl_kern_proc_pathname(HChar *out, SizeT *len)
+static Int sysctl_kern_proc_pathname(HChar *out, SizeT *len)
 {
    const HChar *exe_name = VG_(resolved_exename);
+   // assert that exe_name is an absolute path
+   vg_assert(exe_name && exe_name[0] == '/');
 
    if (!len) {
-      return False;
+      return VKI_ENOMEM;
    }
 
+   if (!ML_(safe_to_deref)(len, sizeof(len))) {
+      // ???? check
+      return VKI_ENOMEM;
+   }
+
+   SizeT exe_name_length = VG_(strlen)(exe_name)+1;
    if (!out) {
-      HChar tmp[VKI_PATH_MAX];
-      if (!VG_(realpath)(exe_name, tmp)) {
-         return False;
-      }
-      *len = VG_(strlen)(tmp)+1;
-      return True;
+      *len = exe_name_length;
+      return 0;
    }
 
-   if (!VG_(realpath)(exe_name, out)) {
-      return False;
+   if (*len < exe_name_length) {
+      return VKI_ENOMEM;
    }
 
-   *len = VG_(strlen)(out)+1;
-   return True;
+   if (ML_(safe_to_deref)(out, exe_name_length)) {
+      VG_(strncpy)(out, exe_name, exe_name_length);
+   } else {
+      // ???? check
+      return VKI_EFAULT;
+   }
+
+   *len = exe_name_length;
+   return 0;
 }
 
 // SYS___sysctl   202
@@ -2031,7 +2042,7 @@ PRE(sys___sysctl)
    if (SARG2 == 2 && ML_(safe_to_deref)(name, 2*sizeof(int))) {
       if (name[0] == 1 && name[1] == 32) {
          if (sysctl_kern_ps_strings((SizeT*)ARG3, (SizeT*)ARG4)) {
-           SET_STATUS_Success(0);
+            SET_STATUS_Success(0);
          }
       }
    }
@@ -2043,8 +2054,12 @@ PRE(sys___sysctl)
       if (name[0] == 1 && name[1] == 14 && name[2] == 12) {
          vki_pid_t pid = (vki_pid_t)name[3];
          if (pid == -1 || pid == VG_(getpid)()) {
-            sysctl_kern_proc_pathname((HChar *)ARG3, (SizeT *)ARG4);
-            SET_STATUS_Success(0);
+            int res = sysctl_kern_proc_pathname((HChar *)ARG3, (SizeT *)ARG4);
+            if (res == 0) {
+               SET_STATUS_Success(0);
+            } else {
+               SET_STATUS_Failure(res);
+            }
          }
       }
    }
diff --git a/memcheck/tests/freebsd/bug470713.cpp b/memcheck/tests/freebsd/bug470713.cpp
index e07dba76b5..b49fb16642 100644
--- a/memcheck/tests/freebsd/bug470713.cpp
+++ b/memcheck/tests/freebsd/bug470713.cpp
@@ -18,29 +18,51 @@ int main(int argc, char **argv)
    int mib[] = { CTL_KERN, KERN_PROC, KERN_PROC_PATHNAME, -1};
    size_t len;
 
-   if (sysctl(mib, 4, NULL, &len, NULL, 0) != 0) {
-      cout << "sysctl failed to get path length: " << std::strerror(errno) << '\n';
+   if (argc < 2)
+   {
+      std::cout << "ERROR: missing argument, expected \"" << argv[0] << " {absolute path of " << argv[0] << "}\"\n";
       return -1;
    }
 
+   if (sysctl(mib, 4, nullptr, &len, nullptr, 0) != 0) {
+      cout << "ERROR: sysctl failed to get path length: " << std::strerror(errno) << '\n';
+      return -1;
+   }
+
+   // not for regtest, path length may vary
+   // std::cout << "read length " << len << '\n';
+
    std::unique_ptr<char[]> aResult(new char[len]);
 
-   if (sysctl(mib, 4, aResult.get(), &len, NULL, 0) != 0)  {
-      cout << "sysctl failed to get path: " << strerror(errno) << '\n';
+   if (sysctl(mib, 4, aResult.get(), &len, nullptr, 0) != 0)  {
+      cout << "ERROR: sysctl failed to get path: " << strerror(errno) << '\n';
       return -1;
    }
 
    if (string(aResult.get()) == argv[1]) {
-      cout << "OK\n";
+      cout << "OK: got expected pathname\n";
+   } else {
+      cout << "ERROR: aResult " << aResult.get() << " argv[1] " << argv[1] << '\n';
+   }
+
+   if (sysctl(mib, 4, aResult.get(), nullptr, nullptr, 0) != 0)  {
+      cout << "OK: sysctl failed with nullptr length: " << strerror(errno) << '\n';
+   } else {
+      cout << "ERROR: nullptr length sysctl succeeded when it should have failed\n";
+   }
+
+   size_t bad_len = len - 3U;
+   if (sysctl(mib, 4, aResult.get(), &bad_len, nullptr, 0) != 0)  {
+      cout << "OK: sysctl failed to get path with bad_len: " << strerror(errno) << '\n';
    } else {
-      cout << "Not OK aResult " << aResult.get() << " argv[1] " << argv[1] << '\n';
+      cout << "ERROR: bad_len sysctl succeeded when it should have failed\n";
+      return -1;
    }
 
-   if (sysctl(mib, 4, NULL, NULL, NULL, 0) != -1) {
-      cout << "OK syscall failed\n";
+   if (sysctl(mib, 4, nullptr, &len, nullptr, 0) != -1) {
+      cout << "OK: sysctl failed with nullptr name: " << strerror(errno) << '\n';
       return -1;
    } else {
-      cout << "sysctl succeeded when it should have failed\n";
+      cout << "ERROR: nullptr name sysctl succeeded when it should have failed\n";
    }
 }
-
diff --git a/memcheck/tests/freebsd/bug470713.stdout.exp b/memcheck/tests/freebsd/bug470713.stdout.exp
index 2ba70ed13d..76dc05b5a0 100644
--- a/memcheck/tests/freebsd/bug470713.stdout.exp
+++ b/memcheck/tests/freebsd/bug470713.stdout.exp
@@ -1,2 +1,4 @@
-OK
-OK syscall failed
+OK: got expected pathname
+OK: sysctl failed with nullptr length: Cannot allocate memory
+OK: sysctl failed to get path with bad_len: Cannot allocate memory
+OK: sysctl failed with nullptr name: Cannot allocate memory