Patchwork [PATCHv2,1/2] signal: added a wrapper for sigprocmask function

login
register
mail settings
Submitter Alex Barcelo
Date Oct. 17, 2012, 2:18 p.m.
Message ID <1350483518-5789-2-git-send-email-abarcelo@ac.upc.edu>
Download mbox | patch
Permalink /patch/192055/
State New
Headers show

Comments

Alex Barcelo - Oct. 17, 2012, 2:18 p.m.
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(-)
Peter Maydell - Oct. 17, 2012, 3:01 p.m.
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
Alex Barcelo - Oct. 17, 2012, 10:06 p.m.
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.
Peter Maydell - Oct. 18, 2012, 7:46 a.m.
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

Patch

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;
         }