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

[Qemu-devel] [PATCHv2 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.

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] [PATCHv2 1/2] signal: added a wrapper for sigprocmask function

[Alex Barcelo]

Create a wrapper for signal mask changes initiated by the guest;
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 fc4cc00..e2dd6a6 100644
--- a/linux-user/qemu.h
+++ b/linux-user/qemu.h
@@ -237,6 +237,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 15bc4e8..3d25b7d 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5461,6 +5461,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 471d060..49c0beb 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -5875,7 +5875,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;
         }
@@ -5886,10 +5886,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;
         }
@@ -5920,7 +5920,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;
@@ -5954,7 +5954,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;
@@ -5994,7 +5994,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;
@@ -7870,7 +7870,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] [PATCHv2 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 3d25b7d..b965d89 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5468,7 +5468,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] [PATCHv2 1/2] signal: added a wrapper for sigprocmask function

[Peter Maydell]

On 17 October 2012 15:18, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> Create a wrapper for signal mask changes initiated by the guest;
> 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>

In my comments on v1 of this patch I wrote:
"I think all the uses of sigprocmask() in linux-user/signal.c also
need to be do_sigprocmask(), as they are the guest trying to control
its signal mask (via the mask it specifies for running signal handlers,
or the mask it passes back when restoring context on return from a
signal handler)."

...but I don't see those changes here.

-- PMM

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

[Alex Barcelo]

On Wed, Oct 17, 2012 at 5:01 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 17 October 2012 15:18, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>> Create a wrapper for signal mask changes initiated by the guest;
>> 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>
>
> In my comments on v1 of this patch I wrote:
> "I think all the uses of sigprocmask() in linux-user/signal.c also
> need to be do_sigprocmask(), as they are the guest trying to control
> its signal mask (via the mask it specifies for running signal handlers,
> or the mask it passes back when restoring context on return from a
> signal handler)."

I saw sigprocmask being used only inside sigreturn functions (hope I
checked it correctly). I thought (maybe wrongly) that sigreturn should
not be wrapped, just as internal sigprocmask calls are not proxyfied
through do_sigprocmask.

Sigreturn functions should not be called from guest directly, so they
should not be a threat. And if some application uses it... well, then
it is its fault, as POSIX does not guarantee any behavior (am I
right?)

Do you think, despite that, that those calls should be done through
do_sigprocmask? The scenario where this may be relevant is in SIGSEGV
interrupt service routines (because the sigreturn may tamper with
SIGSEGV). In this scenario... I don't know how will anything behave,
so I can't tell if it is worth doing it, or if there is any real case
where qemu-user behavior is improving.

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

[Peter Maydell]

On 17 October 2012 23:06, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> On Wed, Oct 17, 2012 at 5:01 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
>> In my comments on v1 of this patch I wrote:
>> "I think all the uses of sigprocmask() in linux-user/signal.c also
>> need to be do_sigprocmask(), as they are the guest trying to control
>> its signal mask (via the mask it specifies for running signal handlers,
>> or the mask it passes back when restoring context on return from a
>> signal handler)."
>
> I saw sigprocmask being used only inside sigreturn functions (hope I
> checked it correctly). I thought (maybe wrongly) that sigreturn should
> not be wrapped, just as internal sigprocmask calls are not proxyfied
> through do_sigprocmask.
>
> Sigreturn functions should not be called from guest directly, so they
> should not be a threat. And if some application uses it... well, then
> it is its fault, as POSIX does not guarantee any behavior (am I
> right?)

sigreturn functions operate on a structure passed in from the guest.
When the kernel enters a signal handler it stores the state of the
interrupted process on that process' stack before setting it up
to run the signal handler function. Return from the signal handler
involves the kernel (or in this case QEMU) restoring all that state.
Part of that state is the signal mask. Since the guest might have
changed that state, we must not trust it and so it goes through the
wrapper.

>  I can't tell if it is worth doing it, or if there is any real case
> where qemu-user behavior is improving.

The point is consistency of design. Masks from the guest go through
the wrapper; masks used internally do not.

PS: if you disagree with a point in code review (and reviewers are
not always right!) it is better to send an email making the case
for why you disagree. If you just ignore it and send v2 patches
then you're forcing reviewers to hunt through your patch all
over again to check whether you paid attention the first time
round...

-- PMM