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

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

[Alex Barcelo]

The first patch creates a sigprocmask wrapper on signal.c for its use in syscall.c

The second patch changes the wrapper to protect sigsegv bit on the signal mask.

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  |   15 +++++++++++++++
 linux-user/syscall.c |   20 ++++++++++----------
 3 files changed, 26 insertions(+), 10 deletions(-)

-- 
1.7.5.4

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

[Alex Barcelo]

A transparent wrapper for sigprocmask function.

---
 linux-user/qemu.h    |    1 +
 linux-user/signal.c  |    8 ++++++++
 linux-user/syscall.c |   20 ++++++++++----------
 3 files changed, 19 insertions(+), 10 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 7869147..b8b8268 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5463,6 +5463,14 @@ long do_rt_sigreturn(CPUArchState *env)
 
 #endif
 
+/* Wrapper for sigprocmask function
+ * Emulates a sigprocmask in a safe way for the 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..1117bbe 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -4260,7 +4260,7 @@ static void *clone_func(void *arg)
     if (info->parent_tidptr)
         put_user_u32(info->tid, info->parent_tidptr);
     /* Enable signals.  */
-    sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
+    do_sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
     /* Signal to the parent that we're ready.  */
     pthread_mutex_lock(&info->mutex);
     pthread_cond_broadcast(&info->cond);
@@ -4351,12 +4351,12 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
         /* It is not safe to deliver signals until the child has finished
            initializing, so temporarily block all signals.  */
         sigfillset(&sigmask);
-        sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask);
+        do_sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask);
 
         ret = pthread_create(&info.thread, &attr, clone_func, &info);
         /* TODO: Free new CPU state if thread creation failed.  */
 
-        sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
+        do_sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
         pthread_attr_destroy(&attr);
         if (ret == 0) {
             /* Wait for the child to initialize.  */
@@ -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] [PATCH 2/2] signal: sigsegv protection on do_sigprocmask

[Alex Barcelo]

The sigsegv protection is done by forcing the catch (needed in qemu-user) 
and then taking it off from the return mask (well, adding it in fact)

---
 linux-user/signal.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/linux-user/signal.c b/linux-user/signal.c
index b8b8268..8764f57 100644
--- a/linux-user/signal.c
+++ b/linux-user/signal.c
@@ -5468,7 +5468,14 @@ 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 temp = *set;
+    if (set) {
+        sigdelset(&temp, SIGSEGV);
+    }
+    ret = sigprocmask(how, &temp, oldset);
+    sigaddset(oldset, SIGSEGV);
+    return ret;
 }
 
 void process_pending_signals(CPUArchState *cpu_env)
-- 
1.7.5.4

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

[Alex Barcelo]

okay, now I see that this lacks a lot of "presentation".

This is a patch to fix a problem on signal masks, and solves a certain
problem when the client is "playing the game" of sigsegv mask/unmask.
I will explain it a little more on a better patch. This should have
been explained on the 00/00 cover. And, moreover, this is kinda the v2
of a single-mail patch.

Before sending a v2, is there something more that I should correct?
And one netiquete question, test-breaking code should be in the
description (cover, 00/00)? On a next post in the same thread? Another
thread?

On Sat, Sep 29, 2012 at 6:11 PM, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> The first patch creates a sigprocmask wrapper on signal.c for its use in syscall.c
>
> The second patch changes the wrapper to protect sigsegv bit on the signal mask.
>
> 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  |   15 +++++++++++++++
>  linux-user/syscall.c |   20 ++++++++++----------
>  3 files changed, 26 insertions(+), 10 deletions(-)
>
> --
> 1.7.5.4
>

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

[Peter Maydell]

On 8 October 2012 19:42, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
> okay, now I see that this lacks a lot of "presentation".

> Before sending a v2, is there something more that I should correct?

Well, yes, your cover letter could be a little more verbose, but I
think mostly it's just that nobody's got round to reviewing the
patches yet. I'll have a look at them in a moment.

One thing that is definitely missing and is critical is that
you need to include a Signed-off-by: line in your patches'
commit messages: we cannot commit them without one.
(http://wiki.qemu.org/Contribute/SubmitAPatch mentions this and
has some other hints, if you haven't read it.)

> And one netiquete question, test-breaking code should be in the
> description (cover, 00/00)? On a next post in the same thread? Another
> thread?

In the cover letter is fine.

-- PMM

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

[Peter Maydell]

On 29 September 2012 17:11, Alex Barcelo <abarcelo@ac.upc.edu> wrote:

Hi; thanks for this patch.

> A transparent wrapper for sigprocmask function.

As I mentioned in my reply to the cover letter, this needs a
Signed-off-by: line.

The commit message could also be a bit more verbose, for instance
something like:

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.

(To some extent this is a personal style thing, but I tend
to favour fairly verbose commentary and commit messages rather
than terseness.)

>
> ---
>  linux-user/qemu.h    |    1 +
>  linux-user/signal.c  |    8 ++++++++
>  linux-user/syscall.c |   20 ++++++++++----------
>  3 files changed, 19 insertions(+), 10 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 7869147..b8b8268 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5463,6 +5463,14 @@ long do_rt_sigreturn(CPUArchState *env)
>
>  #endif
>
> +/* Wrapper for sigprocmask function
> + * Emulates a sigprocmask in a safe way for the guest

You might like to add:
 * Note that set and oldset are host signal sets, not guest ones.

> + */
> +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..1117bbe 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -4260,7 +4260,7 @@ static void *clone_func(void *arg)
>      if (info->parent_tidptr)
>          put_user_u32(info->tid, info->parent_tidptr);
>      /* Enable signals.  */
> -    sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
> +    do_sigprocmask(SIG_SETMASK, &info->sigmask, NULL);
>      /* Signal to the parent that we're ready.  */
>      pthread_mutex_lock(&info->mutex);
>      pthread_cond_broadcast(&info->cond);
> @@ -4351,12 +4351,12 @@ static int do_fork(CPUArchState *env, unsigned int flags, abi_ulong newsp,
>          /* It is not safe to deliver signals until the child has finished
>             initializing, so temporarily block all signals.  */
>          sigfillset(&sigmask);
> -        sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask);
> +        do_sigprocmask(SIG_BLOCK, &sigmask, &info.sigmask);
>
>          ret = pthread_create(&info.thread, &attr, clone_func, &info);
>          /* TODO: Free new CPU state if thread creation failed.  */
>
> -        sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
> +        do_sigprocmask(SIG_SETMASK, &info.sigmask, NULL);
>          pthread_attr_destroy(&attr);
>          if (ret == 0) {
>              /* Wait for the child to initialize.  */

The three sigprocmask() calls above are all QEMU itself manipulating
its own signal mask (to block signals temporarily across a thread
creation). They're not the guest asking for a signal mask change.
So these three can all just use sigprocmask() and shouldn't go via
do_sigprocmask() I think.

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


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).

-- PMM

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

[Peter Maydell]

On 29 September 2012 17:11, Alex Barcelo <abarcelo@ac.upc.edu> wrote:

> Re: [Qemu-devel] [PATCH 2/2] signal: sigsegv protection on do_sigprocmask

The convention for the initial summary line of a patch is that
it starts with an indication of the subsystem being patched. For
instance, here it might be:

"linux-user: Don't allow guest to block SIGSEGV"

> The sigsegv protection is done by forcing the catch (needed in qemu-user)
> and then taking it off from the return mask (well, adding it in fact)

I'm afraid I couldn't really understand what you're trying to
say in this commit message. Perhaps you could expand it a bit?

(missing Signed-off-by:.)
>
> ---
>  linux-user/signal.c |    9 ++++++++-
>  1 files changed, 8 insertions(+), 1 deletions(-)
>
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index b8b8268..8764f57 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -5468,7 +5468,14 @@ 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 temp = *set;

This is dereferencing set, which will crash if it is NULL.

> +    if (set) {
> +        sigdelset(&temp, SIGSEGV);
> +    }
> +    ret = sigprocmask(how, &temp, oldset);

You need to pass NULL in here rather than &temp if the 'set'
argument was NULL.

> +    sigaddset(oldset, SIGSEGV);
> +    return ret;
>  }

So, we don't permit the guest to block SIGSEGV; that makes sense.
Does it make sense to always tell the guest SIGSEGV is blocked?
(what this patch currently does).
The other options would be "tell the guest SIGSEGV is never blocked"
and "actually maintain somewhere the state of whether the guest
thinks SIGSEGV is blocked so we can return the right answer".

I'm just wondering which would be generally better for guests
and why you picked this approach rather than one of the other
options. (it might be the best, I haven't thought too hard
about it...)

thanks
-- PMM

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

[Alex Barcelo]

Thanks Peter for all the feedback! I have just sent the v2 (have been
on holiday this weekend, a bit offline). I hope that this one is a
better patch.

Sorry Riku, I have doubleposted the new patch to you, I slipped on the
git send-email command, and sent the patch only to you at first (not
to the list). I corrected it but now you may have received twice. Next
time I will double check my git output :(

On Wed, Oct 10, 2012 at 5:37 PM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 8 October 2012 19:42, Alex Barcelo <abarcelo@ac.upc.edu> wrote:
>> okay, now I see that this lacks a lot of "presentation".
>
>> Before sending a v2, is there something more that I should correct?
>
> Well, yes, your cover letter could be a little more verbose, but I
> think mostly it's just that nobody's got round to reviewing the
> patches yet. I'll have a look at them in a moment.
>
> One thing that is definitely missing and is critical is that
> you need to include a Signed-off-by: line in your patches'
> commit messages: we cannot commit them without one.
> (http://wiki.qemu.org/Contribute/SubmitAPatch mentions this and
> has some other hints, if you haven't read it.)
>
>> And one netiquete question, test-breaking code should be in the
>> description (cover, 00/00)? On a next post in the same thread? Another
>> thread?
>
> In the cover letter is fine.
>
> -- PMM