| Message ID | 1350483518-5789-2-git-send-email-abarcelo@ac.upc.edu |
|---|---|
| State | New |
| Headers | show |
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
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.
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
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; }
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(-)