PR11234: Ensure __get_argv doesn't overflow

That function was calling strlcpy as if the return value was the number
of bytes copied, but strlcpy actually returns the length of the input
string.  We now use min() to handle the case when it's bigger than the
buffer length, and drop out of the loop when that happens.

diff --git a/tapset/aux_syscalls.stp b/tapset/aux_syscalls.stp
index 4577d64e088f0ce9337a4e39c64017a97b12076d..a0ab557bae4452f02a6bbe3e5cce0dd372695dcb 100644 (file)
--- a/tapset/aux_syscalls.stp
+++ b/tapset/aux_syscalls.stp
@@ -401,20 +401,20 @@ function __sem_flags:string(semflg:long)
 /* This function copies an argv from userspace. */
 function __get_argv:string(a:long, first:long)
 %{ /* pure */
-       char __user *__user *argv = (char __user *__user *)(long)THIS->a;
+       char __user *__user *argv = (char __user *__user *)(long)THIS->a;
        char __user *vstr;
        int space, rc, len = MAXSTRINGLEN;
        char *str = THIS->__retvalue;
        char buf[80];
        char *ptr = buf;
 
-       
+
        if (THIS->first && argv)
                argv++;
 
-       while (argv != NULL) {
+       while (argv != NULL && len) {
                if (__stp_get_user (vstr, argv))
-                       break;
+                       break;
 
                if (vstr == NULL)
                        break;
@@ -443,8 +443,8 @@ function __get_argv:string(a:long, first:long)
                        *str++='\"';
                        len--;
                }
-       
-               rc = strlcpy (str, buf, len); 
+
+               rc = min(len, (int) strlcpy (str, buf, len));
                str += rc;
                len -= rc;
 
@@ -455,13 +455,15 @@ function __get_argv:string(a:long, first:long)
 
                argv++;
        }
+       if (!len)
+               --str;
        *str = 0;
 %}
 /* This function copies an argv from userspace. */
 function __get_compat_argv:string(a:long, first:long)
 %{ /* pure */
 #ifdef CONFIG_COMPAT
-       compat_uptr_t __user *__user *argv = (compat_uptr_t __user *__user *)(long)THIS->a;
+       compat_uptr_t __user *__user *argv = (compat_uptr_t __user *__user *)(long)THIS->a;
        compat_uptr_t __user *vstr;
        int space, rc, len = MAXSTRINGLEN;
        char *str = THIS->__retvalue;
@@ -471,9 +473,9 @@ function __get_compat_argv:string(a:long, first:long)
        if (THIS->first && argv)
                argv++;
 
-       while (argv != NULL) {
+       while (argv != NULL && len) {
                if (__stp_get_user (vstr, argv))
-                       break;
+                       break;
 
                if (vstr == NULL)
                        break;
@@ -502,8 +504,8 @@ function __get_compat_argv:string(a:long, first:long)
                        *str++='\"';
                        len--;
                }
-       
-               rc = strlcpy (str, buf, len); 
+
+               rc = min(len, (int) strlcpy (str, buf, len));
                str += rc;
                len -= rc;
 
@@ -514,6 +516,8 @@ function __get_compat_argv:string(a:long, first:long)
 
                argv++;
        }
+       if (!len)
+               --str;
        *str = 0;
 #endif
 %}

diff --git a/testsuite/systemtap.base/overflow-get_argv.exp b/testsuite/systemtap.base/overflow-get_argv.exp
new file mode 100644 (file)
index 0000000..ac7fddc
--- /dev/null
+++ b/testsuite/systemtap.base/overflow-get_argv.exp
@@ -0,0 +1,5 @@
+# PR11234: __get_argv can overflow its return buffer
+
+set test "overflow-get_argv"
+
+stap_run $srcdir/$subdir/$test.stp no_load $all_pass_string -g -c "/bin/true /usr/bin/*"

diff --git a/testsuite/systemtap.base/overflow-get_argv.stp b/testsuite/systemtap.base/overflow-get_argv.stp
new file mode 100644 (file)
index 0000000..a4d1d21
--- /dev/null
+++ b/testsuite/systemtap.base/overflow-get_argv.stp
@@ -0,0 +1,62 @@
+// PR11234: __get_argv can overflow its return buffer
+
+// __get_argv has a signature like this:
+//    struct function___get_argv_locals {
+//      int64_t a;
+//      int64_t first;
+//      string_t __retvalue;
+//    } function___get_argv;
+//
+// These functions are meant to have an overlap such that we can tell if
+// __get_argv overran its __retvalue.
+//
+//      int64_t x;
+//      int64_t y;
+//      string_t z;
+//      string_t __retvalue;
+//
+// NB: __retvalue[0] always gets cleared on call, but the rest should be
+// untouched, so we can use it as a sentinal.
+
+function clear:string(x:long, y:long, z:string) %{
+  memset(THIS->__retvalue, 0, MAXSTRINGLEN);
+%}
+
+function check:string(x:long, y:long, z:string) %{
+  int i, bad;
+  for (i=1; i<MAXSTRINGLEN; ++i)
+    if (THIS->__retvalue[i])
+      ++bad;
+
+  if (bad)
+    snprintf(THIS->__retvalue, MAXSTRINGLEN, "%d non-zero bytes", bad);
+  else
+    strlcpy(THIS->__retvalue, "ok", MAXSTRINGLEN);
+%}
+
+global result = "untested"
+
+probe syscall.execve {
+  if (pid() != target())
+    next
+
+  clear(0, 0, "")
+  foo = __get_argv($argv, 0)
+  result = check(0, 0, "")
+
+  // ensure that foo isn't optimized away
+  if (foo == "foo")
+    next
+}
+
+probe begin {
+  println("systemtap starting probe")
+}
+
+probe end {
+  println("systemtap ending probe")
+  if (result == "ok")
+    println("systemtap test success")
+  else
+    println("systemtap test failure: ", result)
+}