From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53462) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAXeG-0001Ch-Mp for qemu-devel@nongnu.org; Wed, 08 Jun 2016 03:11:34 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1bAXeB-0003Qj-Ez for qemu-devel@nongnu.org; Wed, 08 Jun 2016 03:11:31 -0400 Received: from jessie.kos.to ([212.47.231.226]:55334 helo=pilvi.kos.to) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1bAXeB-0003Ow-5e for qemu-devel@nongnu.org; Wed, 08 Jun 2016 03:11:27 -0400 Date: Wed, 8 Jun 2016 10:11:21 +0300 From: Riku Voipio Message-ID: <20160608071121.GA28561@beaming.home> References: <1464360721-14359-1-git-send-email-peter.maydell@linaro.org> <1464360721-14359-18-git-send-email-peter.maydell@linaro.org> <20160608063035.GB27512@beaming.home> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <20160608063035.GB27512@beaming.home> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: qemu-devel@nongnu.org, patches@linaro.org, Timothy Edward Baldwin 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). > >=20 > > 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. > >=20 > > 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. >=20 > At least on Debian jessie, this blows up a selection of architectures: >=20 > 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 =E2=80=98host= _to_target_siginfo=E2=80=99: > /home/voipio/linaro/qemu/linux-user/signal.c:387:10: error: =E2=80=98tg= t_tmp._sifields._sigchld._stime=E2=80=99 may be used uninitialized in thi= s function [-Werror=3Dmaybe-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: =E2=80=98tgt= _tmp._sifields._sigchld._stime=E2=80=99 was declared here > target_siginfo_t tgt_tmp; > ^ > /home/voipio/linaro/qemu/linux-user/signal.c:385:10: error: =E2=80=98tg= t_tmp._sifields._sigchld._utime=E2=80=99 may be used uninitialized in thi= s function [-Werror=3Dmaybe-uninitialized] > __put_user(info->_sifields._sigchld._utime, > ^ > /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: =E2=80=98tgt= _tmp._sifields._sigchld._utime=E2=80=99 was declared here > target_siginfo_t tgt_tmp; > ^ > /home/voipio/linaro/qemu/linux-user/signal.c:383:10: error: =E2=80=98tg= t_tmp._sifields._sigchld._status=E2=80=99 may be used uninitialized in th= is function [-Werror=3Dmaybe-uninitialized] > __put_user(info->_sifields._sigchld._status, > ^ > /home/voipio/linaro/qemu/linux-user/signal.c:403:22: note: =E2=80=98tgt= _tmp._sifields._sigchld._status=E2=80=99 was declared here > target_siginfo_t tgt_tmp; > ^ > cc1: all warnings being treated as errors >=20 > These appear to be the architectures where setup_rt_frame isn't impleme= nted. >=20 >=20 > > Signed-off-by: Peter Maydell > > --- > > linux-user/signal.c | 165 ++++++++++++++++++++++++++++++++----= ---------- > > linux-user/syscall_defs.h | 15 +++++ > > 2 files changed, 131 insertions(+), 49 deletions(-) > >=20 > > 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 . > > */ > > #include "qemu/osdep.h" > > +#include "qemu/bitops.h" > > #include > > #include > > =20 > > @@ -274,70 +275,129 @@ static inline void host_to_target_siginfo_nosw= ap(target_siginfo_t *tinfo, > > const siginfo_t *in= fo) > > { > > int sig =3D host_to_target_signal(info->si_signo); > > + int si_code =3D info->si_code; > > + int si_type; > > tinfo->si_signo =3D sig; > > tinfo->si_errno =3D 0; > > tinfo->si_code =3D info->si_code; > > =20 > > - if (sig =3D=3D TARGET_SIGILL || sig =3D=3D TARGET_SIGFPE || sig = =3D=3D TARGET_SIGSEGV > > - || sig =3D=3D TARGET_SIGBUS || sig =3D=3D TARGET_SIGTRAP= ) { > > - /* Should never come here, but who knows. The information fo= r > > - the target is irrelevant. */ > > - tinfo->_sifields._sigfault._addr =3D 0; > > - } else if (sig =3D=3D TARGET_SIGIO) { > > - tinfo->_sifields._sigpoll._band =3D info->si_band; > > - tinfo->_sifields._sigpoll._fd =3D info->si_fd; > > - } else if (sig =3D=3D TARGET_SIGCHLD) { > > - tinfo->_sifields._sigchld._pid =3D info->si_pid; > > - tinfo->_sifields._sigchld._uid =3D 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 possi= ble > > + * 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 =3D info->si_pid; > > + tinfo->_sifields._kill._uid =3D info->si_uid; > > + si_type =3D QEMU_SI_KILL; > > + break; > > + default: > > + /* Everything else is spoofable. Make best guess based on si= gnal */ > > + switch (sig) { > > + case TARGET_SIGCHLD: > > + tinfo->_sifields._sigchld._pid =3D info->si_pid; > > + tinfo->_sifields._sigchld._uid =3D info->si_uid; > > + tinfo->_sifields._sigchld._status > > =3D host_to_target_waitstatus(info->si_status); > > - tinfo->_sifields._sigchld._utime =3D info->si_utime; > > - tinfo->_sifields._sigchld._stime =3D info->si_stime; > > - } else if (sig >=3D TARGET_SIGRTMIN) { > > - tinfo->_sifields._rt._pid =3D info->si_pid; > > - tinfo->_sifields._rt._uid =3D info->si_uid; > > - /* XXX: potential problem if 64 bit */ > > - tinfo->_sifields._rt._sigval.sival_ptr > > + tinfo->_sifields._sigchld._utime =3D info->si_utime; > > + tinfo->_sifields._sigchld._stime =3D info->si_stime; > > + si_type =3D QEMU_SI_CHLD; > > + break; > > + case TARGET_SIGIO: > > + tinfo->_sifields._sigpoll._band =3D info->si_band; > > + tinfo->_sifields._sigpoll._fd =3D info->si_fd; > > + si_type =3D QEMU_SI_POLL; > > + break; > > + default: > > + /* Assume a sigqueue()/mq_notify()/rt_sigqueueinfo() sou= rce. */ > > + tinfo->_sifields._rt._pid =3D info->si_pid; > > + tinfo->_sifields._rt._uid =3D info->si_uid; > > + /* XXX: potential problem if 64 bit */ > > + tinfo->_sifields._rt._sigval.sival_ptr > > =3D (abi_ulong)(unsigned long)info->si_value.sival_p= tr; > > + si_type =3D QEMU_SI_RT; > > + break; > > + } > > + break; > > } > > + > > + tinfo->si_code =3D deposit32(si_code, 16, 16, si_type); > > } > > =20 > > static void tswap_siginfo(target_siginfo_t *tinfo, > > const target_siginfo_t *info) > > { > > - int sig =3D info->si_signo; > > - tinfo->si_signo =3D tswap32(sig); > > - tinfo->si_errno =3D tswap32(info->si_errno); > > - tinfo->si_code =3D tswap32(info->si_code); > > - > > - if (sig =3D=3D TARGET_SIGILL || sig =3D=3D TARGET_SIGFPE || sig = =3D=3D TARGET_SIGSEGV > > - || sig =3D=3D TARGET_SIGBUS || sig =3D=3D TARGET_SIGTRAP) { > > - tinfo->_sifields._sigfault._addr > > - =3D tswapal(info->_sifields._sigfault._addr); > > - } else if (sig =3D=3D TARGET_SIGIO) { > > - tinfo->_sifields._sigpoll._band > > - =3D tswap32(info->_sifields._sigpoll._band); > > - tinfo->_sifields._sigpoll._fd =3D tswap32(info->_sifields._s= igpoll._fd); > > - } else if (sig =3D=3D TARGET_SIGCHLD) { > > - tinfo->_sifields._sigchld._pid > > - =3D tswap32(info->_sifields._sigchld._pid); > > - tinfo->_sifields._sigchld._uid > > - =3D tswap32(info->_sifields._sigchld._uid); > > - tinfo->_sifields._sigchld._status > > - =3D tswap32(info->_sifields._sigchld._status); > > - tinfo->_sifields._sigchld._utime > > - =3D tswapal(info->_sifields._sigchld._utime); > > - tinfo->_sifields._sigchld._stime > > - =3D tswapal(info->_sifields._sigchld._stime); > > - } else if (sig >=3D TARGET_SIGRTMIN) { > > - tinfo->_sifields._rt._pid =3D tswap32(info->_sifields._rt._p= id); > > - tinfo->_sifields._rt._uid =3D tswap32(info->_sifields._rt._u= id); > > - tinfo->_sifields._rt._sigval.sival_ptr > > - =3D tswapal(info->_sifields._rt._sigval.sival_ptr); > > + int si_type =3D extract32(info->si_code, 16, 16); > > + int si_code =3D 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 structu= re > > + * 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._ki= ll._pid); > > + __put_user(info->_sifields._kill._uid, &tinfo->_sifields._ki= ll._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(); > > } > > } > > =20 > > - > > 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, tar= get_siginfo_t *info) > > =20 > > trace_user_queue_signal(env, sig); > > =20 > > + /* 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 =3D deposit32(info->si_code, 16, 16, QEMU_SI_FAULT= ); > > + > > ts->sync_signal.info =3D *info; > > ts->sync_signal.pending =3D 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 { > > =20 > > #define TARGET_SI_PAD_SIZE ((TARGET_SI_MAX_SIZE - TARGET_SI_PREAMBLE= _SIZE) / sizeof(int)) > > =20 > > +/* Within QEMU the top 16 bits of si_code indicate which of the part= s 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 structur= es > > + * 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; > > --=20 > > 1.9.1 > >=20