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

[Riku Voipio <riku.voipio@iki.fi>]

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,
          ^
/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
>