From: Riku Voipio <riku.voipio@iki.fi>
To: Peter Maydell <peter.maydell@linaro.org>
Cc: qemu-devel@nongnu.org, patches@linaro.org,
Timothy Edward Baldwin <T.E.Baldwin99@members.leeds.ac.uk>
Subject: Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t
Date: Wed, 8 Jun 2016 10:11:21 +0300 [thread overview]
Message-ID: <20160608071121.GA28561@beaming.home> (raw)
In-Reply-To: <20160608063035.GB27512@beaming.home>
On Wed, Jun 08, 2016 at 09:30:35AM +0300, Riku Voipio wrote:
> On Fri, May 27, 2016 at 03:51:59PM +0100, Peter Maydell wrote:
> > The siginfo_t struct includes a union. The correct way to identify
> > which fields of the union are relevant is complicated, because we
> > have to use a combination of the si_code and si_signo to figure out
> > which of the union's members are valid. (Within the host kernel it
> > is always possible to tell, but the kernel carefully avoids giving
> > userspace the high 16 bits of si_code, so we don't have the
> > information to do this the easy way...) We therefore make our best
> > guess, bearing in mind that a guest can spoof most of the si_codes
> > via rt_sigqueueinfo() if it likes. Once we have made our guess, we
> > record it in the top 16 bits of the si_code, so that tswap_siginfo()
> > later can use it. tswap_siginfo() then strips these top bits out
> > before writing si_code to the guest (sign-extending the lower bits).
> >
> > This fixes a bug where fields were sometimes wrong; in particular
> > the LTP kill10 test went into an infinite loop because its signal
> > handler got a si_pid value of 0 rather than the pid of the sending
> > process.
> >
> > As part of this change, we switch to using __put_user() in the
> > tswap_siginfo code which writes out the byteswapped values to
> > the target memory, in case the target memory pointer is not
> > sufficiently aligned for the host CPU's requirements.
>
> At least on Debian jessie, this blows up a selection of architectures:
>
> CC microblazeel-linux-user/linux-user/signal.o
> CC cris-linux-user/linux-user/signal.o
> CC microblaze-linux-user/linux-user/signal.o
> CC sparc-linux-user/linux-user/signal.o
> CC sparc64-linux-user/linux-user/signal.o
> CC x86_64-linux-user/linux-user/signal.o
> CC unicore32-linux-user/linux-user/signal.o
> CC sparc32plus-linux-user/linux-user/signal.o
> /home/voipio/linaro/qemu/linux-user/signal.c: In function ‘host_to_target_siginfo’:
> /home/voipio/linaro/qemu/linux-user/signal.c:387:10: error: ‘tgt_tmp._sifields._sigchld._stime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> __put_user(info->_sifields._sigchld._stime,
On the second look, this compile fail only happens if the next patch
that introduces tgt_tmp variable is also applied. Dropping that patch
instead.
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._stime’ was declared here
> target_siginfo_t tgt_tmp;
> ^
> /home/voipio/linaro/qemu/linux-user/signal.c:385:10: error: ‘tgt_tmp._sifields._sigchld._utime’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> __put_user(info->_sifields._sigchld._utime,
> ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._utime’ was declared here
> target_siginfo_t tgt_tmp;
> ^
> /home/voipio/linaro/qemu/linux-user/signal.c:383:10: error: ‘tgt_tmp._sifields._sigchld._status’ may be used uninitialized in this function [-Werror=maybe-uninitialized]
> __put_user(info->_sifields._sigchld._status,
> ^
> /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: ‘tgt_tmp._sifields._sigchld._status’ was declared here
> target_siginfo_t tgt_tmp;
> ^
> cc1: all warnings being treated as errors
>
> These appear to be the architectures where setup_rt_frame isn't implemented.
>
>
> > Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> > ---
> > linux-user/signal.c | 165 ++++++++++++++++++++++++++++++++--------------
> > linux-user/syscall_defs.h | 15 +++++
> > 2 files changed, 131 insertions(+), 49 deletions(-)
> >
> > diff --git a/linux-user/signal.c b/linux-user/signal.c
> > index b21d6bf..8ea0cbf 100644
> > --- a/linux-user/signal.c
> > +++ b/linux-user/signal.c
> > @@ -17,6 +17,7 @@
> > * along with this program; if not, see <http://www.gnu.org/licenses/>.
> > */
> > #include "qemu/osdep.h"
> > +#include "qemu/bitops.h"
> > #include <sys/ucontext.h>
> > #include <sys/resource.h>
> >
> > @@ -274,70 +275,129 @@ static inline void host_to_target_siginfo_noswap(target_siginfo_t *tinfo,
> > const siginfo_t *info)
> > {
> > int sig = host_to_target_signal(info->si_signo);
> > + int si_code = info->si_code;
> > + int si_type;
> > tinfo->si_signo = sig;
> > tinfo->si_errno = 0;
> > tinfo->si_code = info->si_code;
> >
> > - if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> > - || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> > - /* Should never come here, but who knows. The information for
> > - the target is irrelevant. */
> > - tinfo->_sifields._sigfault._addr = 0;
> > - } else if (sig == TARGET_SIGIO) {
> > - tinfo->_sifields._sigpoll._band = info->si_band;
> > - tinfo->_sifields._sigpoll._fd = info->si_fd;
> > - } else if (sig == TARGET_SIGCHLD) {
> > - tinfo->_sifields._sigchld._pid = info->si_pid;
> > - tinfo->_sifields._sigchld._uid = info->si_uid;
> > - tinfo->_sifields._sigchld._status
> > + /* This is awkward, because we have to use a combination of
> > + * the si_code and si_signo to figure out which of the union's
> > + * members are valid. (Within the host kernel it is always possible
> > + * to tell, but the kernel carefully avoids giving userspace the
> > + * high 16 bits of si_code, so we don't have the information to
> > + * do this the easy way...) We therefore make our best guess,
> > + * bearing in mind that a guest can spoof most of the si_codes
> > + * via rt_sigqueueinfo() if it likes.
> > + *
> > + * Once we have made our guess, we record it in the top 16 bits of
> > + * the si_code, so that tswap_siginfo() later can use it.
> > + * tswap_siginfo() will strip these top bits out before writing
> > + * si_code to the guest (sign-extending the lower bits).
> > + */
> > +
> > + switch (si_code) {
> > + case SI_USER:
> > + case SI_TKILL:
> > + case SI_KERNEL:
> > + /* Sent via kill(), tkill() or tgkill(), or direct from the kernel.
> > + * These are the only unspoofable si_code values.
> > + */
> > + tinfo->_sifields._kill._pid = info->si_pid;
> > + tinfo->_sifields._kill._uid = info->si_uid;
> > + si_type = QEMU_SI_KILL;
> > + break;
> > + default:
> > + /* Everything else is spoofable. Make best guess based on signal */
> > + switch (sig) {
> > + case TARGET_SIGCHLD:
> > + tinfo->_sifields._sigchld._pid = info->si_pid;
> > + tinfo->_sifields._sigchld._uid = info->si_uid;
> > + tinfo->_sifields._sigchld._status
> > = host_to_target_waitstatus(info->si_status);
> > - tinfo->_sifields._sigchld._utime = info->si_utime;
> > - tinfo->_sifields._sigchld._stime = info->si_stime;
> > - } else if (sig >= TARGET_SIGRTMIN) {
> > - tinfo->_sifields._rt._pid = info->si_pid;
> > - tinfo->_sifields._rt._uid = info->si_uid;
> > - /* XXX: potential problem if 64 bit */
> > - tinfo->_sifields._rt._sigval.sival_ptr
> > + tinfo->_sifields._sigchld._utime = info->si_utime;
> > + tinfo->_sifields._sigchld._stime = info->si_stime;
> > + si_type = QEMU_SI_CHLD;
> > + break;
> > + case TARGET_SIGIO:
> > + tinfo->_sifields._sigpoll._band = info->si_band;
> > + tinfo->_sifields._sigpoll._fd = info->si_fd;
> > + si_type = QEMU_SI_POLL;
> > + break;
> > + default:
> > + /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() source. */
> > + tinfo->_sifields._rt._pid = info->si_pid;
> > + tinfo->_sifields._rt._uid = info->si_uid;
> > + /* XXX: potential problem if 64 bit */
> > + tinfo->_sifields._rt._sigval.sival_ptr
> > = (abi_ulong)(unsigned long)info->si_value.sival_ptr;
> > + si_type = QEMU_SI_RT;
> > + break;
> > + }
> > + break;
> > }
> > +
> > + tinfo->si_code = deposit32(si_code, 16, 16, si_type);
> > }
> >
> > static void tswap_siginfo(target_siginfo_t *tinfo,
> > const target_siginfo_t *info)
> > {
> > - int sig = info->si_signo;
> > - tinfo->si_signo = tswap32(sig);
> > - tinfo->si_errno = tswap32(info->si_errno);
> > - tinfo->si_code = tswap32(info->si_code);
> > -
> > - if (sig == TARGET_SIGILL || sig == TARGET_SIGFPE || sig == TARGET_SIGSEGV
> > - || sig == TARGET_SIGBUS || sig == TARGET_SIGTRAP) {
> > - tinfo->_sifields._sigfault._addr
> > - = tswapal(info->_sifields._sigfault._addr);
> > - } else if (sig == TARGET_SIGIO) {
> > - tinfo->_sifields._sigpoll._band
> > - = tswap32(info->_sifields._sigpoll._band);
> > - tinfo->_sifields._sigpoll._fd = tswap32(info->_sifields._sigpoll._fd);
> > - } else if (sig == TARGET_SIGCHLD) {
> > - tinfo->_sifields._sigchld._pid
> > - = tswap32(info->_sifields._sigchld._pid);
> > - tinfo->_sifields._sigchld._uid
> > - = tswap32(info->_sifields._sigchld._uid);
> > - tinfo->_sifields._sigchld._status
> > - = tswap32(info->_sifields._sigchld._status);
> > - tinfo->_sifields._sigchld._utime
> > - = tswapal(info->_sifields._sigchld._utime);
> > - tinfo->_sifields._sigchld._stime
> > - = tswapal(info->_sifields._sigchld._stime);
> > - } else if (sig >= TARGET_SIGRTMIN) {
> > - tinfo->_sifields._rt._pid = tswap32(info->_sifields._rt._pid);
> > - tinfo->_sifields._rt._uid = tswap32(info->_sifields._rt._uid);
> > - tinfo->_sifields._rt._sigval.sival_ptr
> > - = tswapal(info->_sifields._rt._sigval.sival_ptr);
> > + int si_type = extract32(info->si_code, 16, 16);
> > + int si_code = sextract32(info->si_code, 0, 16);
> > +
> > + __put_user(info->si_signo, &tinfo->si_signo);
> > + __put_user(info->si_errno, &tinfo->si_errno);
> > + __put_user(si_code, &tinfo->si_code);
> > +
> > + /* We can use our internal marker of which fields in the structure
> > + * are valid, rather than duplicating the guesswork of
> > + * host_to_target_siginfo_noswap() here.
> > + */
> > + switch (si_type) {
> > + case QEMU_SI_KILL:
> > + __put_user(info->_sifields._kill._pid, &tinfo->_sifields._kill._pid);
> > + __put_user(info->_sifields._kill._uid, &tinfo->_sifields._kill._uid);
> > + break;
> > + case QEMU_SI_TIMER:
> > + __put_user(info->_sifields._timer._timer1,
> > + &tinfo->_sifields._timer._timer1);
> > + __put_user(info->_sifields._timer._timer2,
> > + &tinfo->_sifields._timer._timer2);
> > + break;
> > + case QEMU_SI_POLL:
> > + __put_user(info->_sifields._sigpoll._band,
> > + &tinfo->_sifields._sigpoll._band);
> > + __put_user(info->_sifields._sigpoll._fd,
> > + &tinfo->_sifields._sigpoll._fd);
> > + break;
> > + case QEMU_SI_FAULT:
> > + __put_user(info->_sifields._sigfault._addr,
> > + &tinfo->_sifields._sigfault._addr);
> > + break;
> > + case QEMU_SI_CHLD:
> > + __put_user(info->_sifields._sigchld._pid,
> > + &tinfo->_sifields._sigchld._pid);
> > + __put_user(info->_sifields._sigchld._uid,
> > + &tinfo->_sifields._sigchld._uid);
> > + __put_user(info->_sifields._sigchld._status,
> > + &tinfo->_sifields._sigchld._status);
> > + __put_user(info->_sifields._sigchld._utime,
> > + &tinfo->_sifields._sigchld._utime);
> > + __put_user(info->_sifields._sigchld._stime,
> > + &tinfo->_sifields._sigchld._stime);
> > + break;
> > + case QEMU_SI_RT:
> > + __put_user(info->_sifields._rt._pid, &tinfo->_sifields._rt._pid);
> > + __put_user(info->_sifields._rt._uid, &tinfo->_sifields._rt._uid);
> > + __put_user(info->_sifields._rt._sigval.sival_ptr,
> > + &tinfo->_sifields._rt._sigval.sival_ptr);
> > + break;
> > + default:
> > + g_assert_not_reached();
> > }
> > }
> >
> > -
> > void host_to_target_siginfo(target_siginfo_t *tinfo, const siginfo_t *info)
> > {
> > host_to_target_siginfo_noswap(tinfo, info);
> > @@ -505,6 +565,13 @@ int queue_signal(CPUArchState *env, int sig, target_siginfo_t *info)
> >
> > trace_user_queue_signal(env, sig);
> >
> > + /* Currently all callers define siginfo structures which
> > + * use the _sifields._sigfault union member, so we can
> > + * set the type here. If that changes we should push this
> > + * out so the si_type is passed in by callers.
> > + */
> > + info->si_code = deposit32(info->si_code, 16, 16, QEMU_SI_FAULT);
> > +
> > ts->sync_signal.info = *info;
> > ts->sync_signal.pending = sig;
> > /* signal that a new signal is pending */
> > diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> > index 34af15a..124754f 100644
> > --- a/linux-user/syscall_defs.h
> > +++ b/linux-user/syscall_defs.h
> > @@ -673,6 +673,21 @@ typedef struct {
> >
> > #define TARGET_SI_PAD_SIZE ((TARGET_SI_MAX_SIZE - TARGET_SI_PREAMBLE_SIZE) / sizeof(int))
> >
> > +/* Within QEMU the top 16 bits of si_code indicate which of the parts of
> > + * the union in target_siginfo is valid. This only applies between
> > + * host_to_target_siginfo_noswap() and tswap_siginfo(); it does not
> > + * appear either within host siginfo_t or in target_siginfo structures
> > + * which we get from the guest userspace program. (The Linux kernel
> > + * does a similar thing with using the top bits for its own internal
> > + * purposes but not letting them be visible to userspace.)
> > + */
> > +#define QEMU_SI_KILL 0
> > +#define QEMU_SI_TIMER 1
> > +#define QEMU_SI_POLL 2
> > +#define QEMU_SI_FAULT 3
> > +#define QEMU_SI_CHLD 4
> > +#define QEMU_SI_RT 5
> > +
> > typedef struct target_siginfo {
> > #ifdef TARGET_MIPS
> > int si_signo;
> > --
> > 1.9.1
> >
next prev parent reply other threads:[~2016-06-08 7:11 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2016-05-27 14:51 [Qemu-devel] [PATCH v2 00/19] linux-user: fix various signal race conditions Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 01/19] linux-user: Factor out handle_signal code from process_pending_signals() Peter Maydell
2016-06-06 21:42 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 02/19] linux-user: Move handle_pending_signal() to avoid need for declaration Peter Maydell
2016-06-06 21:42 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 03/19] linux-user: Fix stray tab-indent Peter Maydell
2016-06-06 21:43 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 04/19] linux-user: Factor out uses of do_sigprocmask() from sigreturn code Peter Maydell
2016-06-06 21:46 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 05/19] linux-user: Define macro for size of host kernel sigset_t Peter Maydell
2016-06-06 21:47 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 06/19] linux-user: Use safe_syscall for sigsuspend syscalls Peter Maydell
2016-06-07 7:20 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 07/19] linux-user: Fix race between multiple signals Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 08/19] linux-user: Remove redundant default action check in queue_signal() Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 09/19] linux-user: Remove redundant gdb_queuesig() Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 10/19] linux-user: Remove real-time signal queuing Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 11/19] linux-user: Queue synchronous signals separately Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 12/19] linux-user: Block signals during sigaction() handling Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 13/19] linux-user: pause() should not pause if signal pending Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 14/19] linux-user: Restart exit() " Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 15/19] linux-user: Use safe_syscall for kill, tkill and tgkill syscalls Peter Maydell
2016-06-07 7:43 ` Laurent Vivier
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 16/19] linux-user: Restart fork() if signals pending Peter Maydell
2016-05-27 14:51 ` [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t Peter Maydell
2016-06-07 19:22 ` Laurent Vivier
2016-06-07 21:06 ` Peter Maydell
2016-06-08 6:30 ` Riku Voipio
2016-06-08 7:11 ` Riku Voipio [this message]
2016-06-09 13:19 ` Peter Maydell
2016-05-27 14:52 ` [Qemu-devel] [PATCH v2 18/19] linux-user: Avoid possible misalignment in host_to_target_siginfo() Peter Maydell
2016-06-07 19:36 ` Laurent Vivier
2016-06-07 21:08 ` Peter Maydell
2016-06-08 9:29 ` Laurent Vivier
2016-06-08 11:29 ` Peter Maydell
2016-05-27 14:52 ` [Qemu-devel] [PATCH v2 19/19] linux-user: Avoid possible misalignment in target_to_host_siginfo() Peter Maydell
2016-06-07 19:40 ` Laurent Vivier
2016-06-06 14:42 ` [Qemu-devel] [PATCH v2 00/19] linux-user: fix various signal race conditions Peter Maydell
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20160608071121.GA28561@beaming.home \
--to=riku.voipio@iki.fi \
--cc=T.E.Baldwin99@members.leeds.ac.uk \
--cc=patches@linaro.org \
--cc=peter.maydell@linaro.org \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).