]> git.ipfire.org Git - thirdparty/valgrind.git/commitdiff
Fix crash in linux [rt_]sigaction wrapper with bad old/new sigaction handler.
authorMark Wielaard <mark@klomp.org>
Sat, 1 Oct 2016 11:54:49 +0000 (11:54 +0000)
committerMark Wielaard <mark@klomp.org>
Sat, 1 Oct 2016 11:54:49 +0000 (11:54 +0000)
Since we try to modify the old/new sigaction handler before passing it
to the kernel we must make sure that (if they aren't NULL) it is safe
to use. If not we should bail out early with EFAULT.

Bug #369362
Found by LTP testcases/kernel/syscalls/rt_sigaction/rt_sigaction02.

git-svn-id: svn://svn.valgrind.org/valgrind/trunk@15994

NEWS
coregrind/m_syswrap/syswrap-linux.c

diff --git a/NEWS b/NEWS
index 051c1a6a4f35c8b4b545ab18da8925e5647792a4..c79e9f36dd638eb1d0f9aadd7340f89c4f186b6b 100644 (file)
--- a/NEWS
+++ b/NEWS
@@ -184,6 +184,7 @@ where XXXXXX is the bug number as listed below.
 369359  msghdr_foreachfield can crash when handling bad iovec
 369360  Bad sigprocmask old or new sets can crash valgrind
 369361  vmsplice syscall wrapper crashes on bad iovec
+369362  Bad sigaction arguments crash valgrind
 
 n-i-bz Fix incorrect (or infinite loop) unwind on RHEL7 x86 and amd64
 n-i-bz massif --pages-as-heap=yes does not report peak caused by mmap+munmap
index 6f9a8beedd5b1dc0cbaff4f21d77192526d68c1a..e53de6c9858368bbbf05879751f1c448f270ac62 100644 (file)
@@ -3277,7 +3277,7 @@ PRE(sys_sigaction)
       PRE_MEM_READ( "sigaction(act->sa_handler)", (Addr)&sa->ksa_handler, sizeof(sa->ksa_handler));
       PRE_MEM_READ( "sigaction(act->sa_mask)", (Addr)&sa->sa_mask, sizeof(sa->sa_mask));
       PRE_MEM_READ( "sigaction(act->sa_flags)", (Addr)&sa->sa_flags, sizeof(sa->sa_flags));
-      if (ML_(safe_to_deref)(sa,sizeof(sa)) 
+      if (ML_(safe_to_deref)(sa,sizeof(sa))
           && (sa->sa_flags & VKI_SA_RESTORER))
          PRE_MEM_READ( "sigaction(act->sa_restorer)", (Addr)&sa->sa_restorer, sizeof(sa->sa_restorer));
    }
@@ -3287,26 +3287,43 @@ PRE(sys_sigaction)
       oldp = &old;
    }
 
-   if (ARG2 != 0) {
-      struct vki_old_sigaction *oldnew = (struct vki_old_sigaction *)ARG2;
+   /* If the new or old sigaction is not NULL, but the structs
+      aren't accessible then sigaction returns EFAULT and we cannot
+      use either struct for our own bookkeeping. Just fail early. */
+   if (ARG2 != 0
+       && ! ML_(safe_to_deref)((void *)ARG2,
+                               sizeof(struct vki_old_sigaction))) {
+      VG_(umsg)("Warning: bad act handler address %p in sigaction()\n",
+                (void *)ARG2);
+      SET_STATUS_Failure ( VKI_EFAULT );
+   } else if ((ARG3 != 0
+               && ! ML_(safe_to_deref)((void *)ARG3,
+                                       sizeof(struct vki_old_sigaction)))) {
+      VG_(umsg)("Warning: bad oldact handler address %p in sigaction()\n",
+                (void *)ARG3);
+      SET_STATUS_Failure ( VKI_EFAULT );
+   } else {
+      if (ARG2 != 0) {
+         struct vki_old_sigaction *oldnew = (struct vki_old_sigaction *)ARG2;
 
-      new.ksa_handler = oldnew->ksa_handler;
-      new.sa_flags = oldnew->sa_flags;
-      new.sa_restorer = oldnew->sa_restorer;
-      convert_sigset_to_rt(&oldnew->sa_mask, &new.sa_mask);
-      newp = &new;
-   }
+         new.ksa_handler = oldnew->ksa_handler;
+         new.sa_flags = oldnew->sa_flags;
+         new.sa_restorer = oldnew->sa_restorer;
+         convert_sigset_to_rt(&oldnew->sa_mask, &new.sa_mask);
+         newp = &new;
+      }
 
-   SET_STATUS_from_SysRes( VG_(do_sys_sigaction)(ARG1, newp, oldp) );
+      SET_STATUS_from_SysRes( VG_(do_sys_sigaction)(ARG1, newp, oldp) );
 
-   if (ARG3 != 0 && SUCCESS && RES == 0) {
-      struct vki_old_sigaction *oldold = (struct vki_old_sigaction *)ARG3;
+      if (ARG3 != 0 && SUCCESS && RES == 0) {
+         struct vki_old_sigaction *oldold = (struct vki_old_sigaction *)ARG3;
 
-      oldold->ksa_handler = oldp->ksa_handler;
-      oldold->sa_flags = oldp->sa_flags;
-      oldold->sa_restorer = oldp->sa_restorer;
-      oldold->sa_mask = oldp->sa_mask.sig[0];
-   }
+         oldold->ksa_handler = oldp->ksa_handler;
+         oldold->sa_flags = oldp->sa_flags;
+         oldold->sa_restorer = oldp->sa_restorer;
+         oldold->sa_mask = oldp->sa_mask.sig[0];
+      }
+  }
 }
 POST(sys_sigaction)
 {
@@ -3373,20 +3390,39 @@ PRE(sys_rt_sigaction)
       PRE_MEM_READ( "rt_sigaction(act->sa_handler)", (Addr)&sa->ksa_handler, sizeof(sa->ksa_handler));
       PRE_MEM_READ( "rt_sigaction(act->sa_mask)", (Addr)&sa->sa_mask, sizeof(sa->sa_mask));
       PRE_MEM_READ( "rt_sigaction(act->sa_flags)", (Addr)&sa->sa_flags, sizeof(sa->sa_flags));
-      if (sa->sa_flags & VKI_SA_RESTORER)
+      if (ML_(safe_to_deref)(sa,sizeof(sa))
+          && (sa->sa_flags & VKI_SA_RESTORER))
          PRE_MEM_READ( "rt_sigaction(act->sa_restorer)", (Addr)&sa->sa_restorer, sizeof(sa->sa_restorer));
    }
    if (ARG3 != 0)
       PRE_MEM_WRITE( "rt_sigaction(oldact)", ARG3, sizeof(vki_sigaction_fromK_t));
 
-   // XXX: doesn't seem right to be calling do_sys_sigaction for
-   // sys_rt_sigaction... perhaps this function should be renamed
-   // VG_(do_sys_rt_sigaction)()  --njn
+   /* If the new or old sigaction is not NULL, but the structs
+      aren't accessible then sigaction returns EFAULT and we cannot
+      use either struct for our own bookkeeping. Just fail early. */
+   if (ARG2 != 0
+       && ! ML_(safe_to_deref)((void *)ARG2,
+                               sizeof(vki_sigaction_toK_t))) {
+      VG_(umsg)("Warning: bad act handler address %p in rt_sigaction()\n",
+                (void *)ARG2);
+      SET_STATUS_Failure ( VKI_EFAULT );
+   } else if ((ARG3 != 0
+               && ! ML_(safe_to_deref)((void *)ARG3,
+                                       sizeof(vki_sigaction_fromK_t)))) {
+      VG_(umsg)("Warning: bad oldact handler address %p in rt_sigaction()\n",
+                (void *)ARG3);
+      SET_STATUS_Failure ( VKI_EFAULT );
+   } else {
 
-   SET_STATUS_from_SysRes(
-      VG_(do_sys_sigaction)(ARG1, (const vki_sigaction_toK_t *)ARG2,
-                            (vki_sigaction_fromK_t *)ARG3)
-   );
+      // XXX: doesn't seem right to be calling do_sys_sigaction for
+      // sys_rt_sigaction... perhaps this function should be renamed
+      // VG_(do_sys_rt_sigaction)()  --njn
+
+      SET_STATUS_from_SysRes(
+         VG_(do_sys_sigaction)(ARG1, (const vki_sigaction_toK_t *)ARG2,
+                               (vki_sigaction_fromK_t *)ARG3)
+      );
+   }
 }
 POST(sys_rt_sigaction)
 {