[Qemu-devel] [PATCHv3 0/2] Preparing safe sigprocmask wrapper on qemu-user

[Qemu-devel] [PATCHv3 0/2] Preparing safe sigprocmask wrapper on qemu-user

[Alex Barcelo]

qemu-user needs SIGSEGV (at least) for some internal use. If the guest
application masks it and does unsafe sigprocmask, then the application
crashes. Problems happen in applications with self-modifying code (who
also change the signal mask). Other guest applications may have related
problems if they use the SIGSEGV.

A way to be more safe is adding a wrapper for all sigprocmask calls from
the guest. The wrapper proposed here is quite simple, but the code can
be improved, here I try to ensure that the wrapper is set up properly.

Changes in v3:
 - Wrapping also sigreturn's sigprocmask calls (on signal.c)

Here, a test case where qemu-user goes wrong:

////////////
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/mman.h>
#include <malloc.h>
#include <signal.h>

unsigned char *testfun;

int main ( void )
{
    unsigned int ra;
    testfun=memalign(getpagesize(),1024);
    // We block the SIGSEGV signal, used by qemu-user
    sigset_t set;
    sigemptyset(&set);
    sigaddset(&set, 11);
    sigprocmask(SIG_BLOCK, &set, NULL);
    mprotect(testfun, 1024, PROT_READ|PROT_EXEC|PROT_WRITE);

    //400687: b8 0d 00 00 00          mov    $0xd,%eax
    //40068d: c3                      retq
    testfun[ 0]=0xb8;
    testfun[ 1]=0x0d;
    testfun[ 2]=0x00;
    testfun[ 3]=0x00;
    testfun[ 4]=0x00;
    testfun[ 5]=0xc3;
    printf ( "0x%02X\n",
             ((unsigned int (*)())testfun)() );

    //400687: b8 20 00 00 00          mov    $0x20,%eax
    //40068d: c3                      retq
    // This self-modifying code will break because of the sigsegv signal block
    testfun[ 1]=0x20;
    printf ( "0x%02X\n",
             ((unsigned int (*)())testfun)() );
}
////////////

On an i386 native host:
0x0D
0x20

On a non-patched qemu-i386:
0x0D
Segmentation fault

Alex Barcelo (2):
  signal: added a wrapper for sigprocmask function
  signal: sigsegv protection on do_sigprocmask

 linux-user/qemu.h    |    1 +
 linux-user/signal.c  |   27 +++++++++++++++++++++++++++
 linux-user/syscall.c |   14 +++++++-------
 3 files changed, 35 insertions(+), 7 deletions(-)

-- 
1.7.5.4

[Qemu-devel] [PATCHv3 1/2] signal: added a wrapper for sigprocmask function

[Alex Barcelo]

Create a wrapper for signal mask changes initiated by the guest;
(this includes syscalls and also the sigreturns from signal.c)
this will give us a place to put code which prevents the guest
from changing the handling of signals used by QEMU itself
internally.

The wrapper is called from all the guest-initiated sigprocmask, but
is not called from internal qemu sigprocmask calls.

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 linux-user/qemu.h    |    1 +
 linux-user/signal.c  |   10 ++++++++++
 linux-user/syscall.c |   14 +++++++-------
 3 files changed, 18 insertions(+), 7 deletions(-)

diff --git a/linux-user/qemu.h b/linux-user/qemu.h
index 5e53dca..1d59458 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -240,6 +240,7 @@ int host_to_target_signal(int sig);
 long do_sigreturn(CPUArchState *env);
 long do_rt_sigreturn(CPUArchState *env);
 abi_long do_sigaltstack(abi_ulong uss_addr, abi_ulong uoss_addr, abi_ulong sp);
+int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset);
 
 #ifdef TARGET_I386
 /* vm86.c */
diff --git a/linux-user/signal.c b/linux-user/signal.c
index 95e2ffa..172de9a 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5482,6 +5482,16 @@ long do_rt_sigreturn(CPUArchState *env)
 
 #endif
 
+/* Wrapper for sigprocmask function
+ * Emulates a sigprocmask in a safe way for the guest. Note that set and oldset
+ * are host signal set, not guest ones. This wraps the sigprocmask host calls
+ * that should be protected (calls originated from guest)
+ */
+int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
+{
+    return sigprocmask(how, set, oldset);
+}
+
 void process_pending_signals(CPUArchState *cpu_env)
 {
     int sig;
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e4291ed..9b57671 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5925,7 +5925,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         {
             sigset_t cur_set;
             abi_ulong target_set;
-            sigprocmask(0, NULL, &cur_set);
+            do_sigprocmask(0, NULL, &cur_set);
             host_to_target_old_sigset(&target_set, &cur_set);
             ret = target_set;
         }
@@ -5936,10 +5936,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         {
             sigset_t set, oset, cur_set;
             abi_ulong target_set = arg1;
-            sigprocmask(0, NULL, &cur_set);
+            do_sigprocmask(0, NULL, &cur_set);
             target_to_host_old_sigset(&set, &target_set);
             sigorset(&set, &set, &cur_set);
-            sigprocmask(SIG_SETMASK, &set, &oset);
+            do_sigprocmask(SIG_SETMASK, &set, &oset);
             host_to_target_old_sigset(&target_set, &oset);
             ret = target_set;
         }
@@ -5970,7 +5970,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             mask = arg2;
             target_to_host_old_sigset(&set, &mask);
 
-            ret = get_errno(sigprocmask(how, &set, &oldset));
+            ret = get_errno(do_sigprocmask(how, &set, &oldset));
             if (!is_error(ret)) {
                 host_to_target_old_sigset(&mask, &oldset);
                 ret = mask;
@@ -6004,7 +6004,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 how = 0;
                 set_ptr = NULL;
             }
-            ret = get_errno(sigprocmask(how, set_ptr, &oldset));
+            ret = get_errno(do_sigprocmask(how, set_ptr, &oldset));
             if (!is_error(ret) && arg3) {
                 if (!(p = lock_user(VERIFY_WRITE, arg3, sizeof(target_sigset_t), 0)))
                     goto efault;
@@ -6044,7 +6044,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 how = 0;
                 set_ptr = NULL;
             }
-            ret = get_errno(sigprocmask(how, set_ptr, &oldset));
+            ret = get_errno(do_sigprocmask(how, set_ptr, &oldset));
             if (!is_error(ret) && arg3) {
                 if (!(p = lock_user(VERIFY_WRITE, arg3, sizeof(target_sigset_t), 0)))
                     goto efault;
@@ -7932,7 +7932,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             }
             mask = arg2;
             target_to_host_old_sigset(&set, &mask);
-            sigprocmask(how, &set, &oldset);
+            do_sigprocmask(how, &set, &oldset);
             host_to_target_old_sigset(&mask, &oldset);
             ret = mask;
         }
-- 
1.7.5.4

[Qemu-devel] [PATCHv3 2/2] signal: sigsegv protection on do_sigprocmask

[Alex Barcelo]

Create a safe wrapper by protecting the signal mask.

Instead of doing a simple passthrough of the sigprocmask, the wrapper
manipulates the signal mask in a safe way for the qemu internal. This
is done by avoiding SIGSEGV bit mask manipulation from the guest.

We also return the same bit on the SIGSEGV. This is not required for
most applications, but if the application checks it, then it will see
that somethings fishy about it (and, in fact, maybe it should). If we
do not want the guest to be aware of those manipulations, then it should
be implemented in another way, but this seems quite clean and consistent.

The wrapper can be improved to add more features for better signal
managing, but this seems enough for "simple" self-modifying code.

Signed-off-by: Alex Barcelo <abarcelo@ac.upc.edu>
---
 linux-user/signal.c |   19 ++++++++++++++++++-
 1 files changed, 18 insertions(+), 1 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index 172de9a..b430ab0 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5489,7 +5489,24 @@ long do_rt_sigreturn(CPUArchState *env)
  */
 int do_sigprocmask(int how, const sigset_t *set, sigset_t *oldset)
 {
-    return sigprocmask(how, set, oldset);
+    int ret;
+    sigset_t val;
+    sigset_t *temp;
+    if (set) {
+        val = *set;
+        temp = &val;
+        sigdelset(temp, SIGSEGV);
+    } else {
+        temp = NULL;
+    }
+    ret = sigprocmask(how, temp, oldset);
+
+    /* Force set state of SIGSEGV, may be best for some apps, maybe not so good
+     * This is not required for qemu to work */
+    if (oldset) {
+        sigaddset(oldset, SIGSEGV);
+    }
+    return ret;
 }
 
 void process_pending_signals(CPUArchState *cpu_env)
-- 
1.7.5.4

Re: [Qemu-devel] [PATCHv3 0/2] Preparing safe sigprocmask wrapper on qemu-user

[Alex Barcelo]

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

ping



On Sat, Oct 20, 2012 at 4:15 PM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:

> qemu-user needs SIGSEGV (at least) for some internal use. If the guest
> application masks it and does unsafe sigprocmask, then the application
> crashes. Problems happen in applications with self-modifying code (who
> also change the signal mask). Other guest applications may have related
> problems if they use the SIGSEGV.
>
> A way to be more safe is adding a wrapper for all sigprocmask calls from
> the guest. The wrapper proposed here is quite simple, but the code can
> be improved, here I try to ensure that the wrapper is set up properly.
>
> Changes in v3:
>  - Wrapping also sigreturn's sigprocmask calls (on signal.c)
>
> Here, a test case where qemu-user goes wrong:
>
> ////////////
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/mman.h>
> #include <malloc.h>
> #include <signal.h>
>
> unsigned char *testfun;
>
> int main ( void )
> {
>     unsigned int ra;
>     testfun=memalign(getpagesize(),1024);
>     // We block the SIGSEGV signal, used by qemu-user
>     sigset_t set;
>     sigemptyset(&set);
>     sigaddset(&set, 11);
>     sigprocmask(SIG_BLOCK, &set, NULL);
>     mprotect(testfun, 1024, PROT_READ|PROT_EXEC|PROT_WRITE);
>
>     //400687: b8 0d 00 00 00          mov    $0xd,%eax
>     //40068d: c3                      retq
>     testfun[ 0]=0xb8;
>     testfun[ 1]=0x0d;
>     testfun[ 2]=0x00;
>     testfun[ 3]=0x00;
>     testfun[ 4]=0x00;
>     testfun[ 5]=0xc3;
>     printf ( "0x%02X\n",
>              ((unsigned int (*)())testfun)() );
>
>     //400687: b8 20 00 00 00          mov    $0x20,%eax
>     //40068d: c3                      retq
>     // This self-modifying code will break because of the sigsegv signal
> block
>     testfun[ 1]=0x20;
>     printf ( "0x%02X\n",
>              ((unsigned int (*)())testfun)() );
> }
> ////////////
>
> On an i386 native host:
> 0x0D
> 0x20
>
> On a non-patched qemu-i386:
> 0x0D
> Segmentation fault
>
> Alex Barcelo (2):
>   signal: added a wrapper for sigprocmask function
>   signal: sigsegv protection on do_sigprocmask
>
>  linux-user/qemu.h    |    1 +
>  linux-user/signal.c  |   27 +++++++++++++++++++++++++++
>  linux-user/syscall.c |   14 +++++++-------
>  3 files changed, 35 insertions(+), 7 deletions(-)
>
> --
> 1.7.5.4
>
>

[-- Attachment #2: Type: text/html, Size: 2986 bytes --]

Re: [Qemu-devel] [PATCHv3 0/2] Preparing safe sigprocmask wrapper on qemu-user

[Alex Barcelo]

> FWIW this mail did not arrive as reply to any previous post on the list
> so it may not be obvious for reviewers what "this one" refers to. You
> may want to check what went wrong (also it arrived as HTML so that
> quotes below are broken).

Ok, sorry, gmail backend does nasty things. I should migrate,
definitely gonna do it.

Original thread:
http://lists.gnu.org/archive/html/qemu-devel/2012-10/msg03638.html

I know that I should repatch it for the new head... but I was waiting
to see if there are things to change before proceeding.

Thanks,

On Sat, Oct 20, 2012 at 4:15 PM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>
> qemu-user needs SIGSEGV (at least) for some internal use. If the guest
> application masks it and does unsafe sigprocmask, then the application
> crashes. Problems happen in applications with self-modifying code (who
> also change the signal mask). Other guest applications may have related
> problems if they use the SIGSEGV.
>
> A way to be more safe is adding a wrapper for all sigprocmask calls from
> the guest. The wrapper proposed here is quite simple, but the code can
> be improved, here I try to ensure that the wrapper is set up properly.
>
> Changes in v3:
>  - Wrapping also sigreturn's sigprocmask calls (on signal.c)
>
> Here, a test case where qemu-user goes wrong:
>
> ////////////
> #include <stdio.h>
> #include <stdlib.h>
> #include <string.h>
> #include <sys/mman.h>
> #include <malloc.h>
> #include <signal.h>
>
> unsigned char *testfun;
>
> int main ( void )
> {
>     unsigned int ra;
>     testfun=memalign(getpagesize(),1024);
>     // We block the SIGSEGV signal, used by qemu-user
>     sigset_t set;
>     sigemptyset(&set);
>     sigaddset(&set, 11);
>     sigprocmask(SIG_BLOCK, &set, NULL);
>     mprotect(testfun, 1024, PROT_READ|PROT_EXEC|PROT_WRITE);
>
>     //400687: b8 0d 00 00 00          mov    $0xd,%eax
>     //40068d: c3                      retq
>     testfun[ 0]=0xb8;
>     testfun[ 1]=0x0d;
>     testfun[ 2]=0x00;
>     testfun[ 3]=0x00;
>     testfun[ 4]=0x00;
>     testfun[ 5]=0xc3;
>     printf ( "0x%02X\n",
>              ((unsigned int (*)())testfun)() );
>
>     //400687: b8 20 00 00 00          mov    $0x20,%eax
>     //40068d: c3                      retq
>     // This self-modifying code will break because of the sigsegv signal
> block
>     testfun[ 1]=0x20;
>     printf ( "0x%02X\n",
>              ((unsigned int (*)())testfun)() );
> }
> ////////////
>
> On an i386 native host:
> 0x0D
> 0x20
>
> On a non-patched qemu-i386:
> 0x0D
> Segmentation fault
>
> Alex Barcelo (2):
>   signal: added a wrapper for sigprocmask function
>   signal: sigsegv protection on do_sigprocmask
>
>  linux-user/qemu.h    |    1 +
>  linux-user/signal.c  |   27 +++++++++++++++++++++++++++
>  linux-user/syscall.c |   14 +++++++-------
>  3 files changed, 35 insertions(+), 7 deletions(-)
>
> --
> 1.7.5.4
>