Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Peter Maydell]

On 8 June 2016 at 07:30, Riku Voipio  wrote:
> At least on Debian jessie, this blows up a selection of architectures:
>
> /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,
>   ^
> /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.

So as far as I can tell this is a combination of:
 * without setup_rt_frame() the compiler makes different decisions
   about whether to inline tswap_siginfo() into host_to_target_siginfo()
   [you can provoke it on all targets by marking tswap_siginfo 'inline']
 * gcc not being able to figure out that the _sigchld fields of the union
   are only read in the tswap_siginfo() switch if they were set in the
   host_to_target_siginfo_noswap() switch (likely because the type info
   is pushed in and out of the top 16 bits of the si_code field)

The simplest fix seems to be to add this to the top of
host_to_target_siginfo_noswap():

+/* This memset serves two purposes:
+ * (1) ensure we don't leak random junk to the guest later
+ * (2) placate false positives from gcc about fields
+ * being used uninitialized if it chooses to inline both this
+ * function and tswap_siginfo() into host_to_target_siginfo().
+ */
+memset(tinfo->_sifields._pad, 0, sizeof(tinfo->_sifields._pad));

I have no idea why gcc only complains about the _sigchld fields and
not any others, though.

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Riku Voipio]

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:
> 
>   CCmicroblazeel-linux-user/linux-user/signal.o
>   CCcris-linux-user/linux-user/signal.o
>   CCmicroblaze-linux-user/linux-user/signal.o
>   CCsparc-linux-user/linux-user/signal.o
>   CCsparc64-linux-user/linux-user/signal.o
>   CCx86_64-linux-user/linux-user/signal.o
>   CCunicore32-linux-user/linux-user/signal.o
>   CCsparc32plus-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 
> > ---
> >  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 .
> >   */
> >  #include "qemu/osdep.h"
> > +#include "qemu/bitops.h"
> >  #include 
> >  #include 
> >  
> > @@ -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) {
> > -

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Riku Voipio]

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:

  CCmicroblazeel-linux-user/linux-user/signal.o
  CCcris-linux-user/linux-user/signal.o
  CCmicroblaze-linux-user/linux-user/signal.o
  CCsparc-linux-user/linux-user/signal.o
  CCsparc64-linux-user/linux-user/signal.o
  CCx86_64-linux-user/linux-user/signal.o
  CCunicore32-linux-user/linux-user/signal.o
  CCsparc32plus-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,
  ^
/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 
> ---
>  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 .
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/bitops.h"
>  #include 
>  #include 
>  
> @@ -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 

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Peter Maydell]

On 7 June 2016 at 20:22, Laurent Vivier  wrote:
> Where is coming from QEMU_SI_TIMER?
> It is not used elsewhere.

It's the enum constant that goes with "we use the
.sifields.timer fields of the union". At the moment we
don't have any cases which cause us to think we should
use those (and we didn't before this patch either), so
the case in this case statement is purely for completeness.
(I suspect the _timer fields are wrong anyway, since they're
pretty much dead code.)

The awkward thing about SI_TIMER is that because glibc
can call rt_sigqueueinfo() with a si_code of SI_TIMER[*] we
have no way to tell "this is a SI_TIMER signal from
the kernel with valid .timer fields" from "this is a
SI_TIMER from rt_sigqueueinfo with valid .rt fields".
So we assume it's always the latter.

[*] for instance, see thread_expire_timer() in
http://osxr.org:8080/glibc/source/nptl/sysdeps/pthread/timer_routines.c

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2 17/19] linux-user: Use both si_code and si_signo when converting siginfo_t

[Laurent Vivier]

Le 27/05/2016 à 16:51, Peter Maydell a écrit :
> 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.
> 
> Signed-off-by: Peter Maydell 
> ---
>  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 .
>   */
>  #include "qemu/osdep.h"
> +#include "qemu/bitops.h"
>  #include 
>  #include 
>  
> @@ -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 */
> -