Re: [Qemu-devel] [PATCH 1/3] linux-user: Restrict usage of sa_restorer

[Laurent Vivier]

Le 11/03/2017 à 04:42, Richard Henderson a écrit :
> Reading and writing to an sa_restorer member that isn't supposed to
> exist corrupts user memory.  Introduce TARGET_ARCH_HAS_SA_RESTORER,
> similar to the kernel's __ARCH_HAS_SA_RESTORER.
> 
> Reported-by: Helge Deller <address@hidden>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  linux-user/signal.c       |  4 ++--
>  linux-user/syscall_defs.h | 13 +++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
> 
> diff --git a/linux-user/signal.c b/linux-user/signal.c
> index a67db04..c6b043b 100644
> --- a/linux-user/signal.c
> +++ b/linux-user/signal.c
> @@ -777,7 +777,7 @@ int do_sigaction(int sig, const struct target_sigaction 
> *act,
>      if (oact) {
>          __put_user(k->_sa_handler, &oact->_sa_handler);
>          __put_user(k->sa_flags, &oact->sa_flags);
> -#if !defined(TARGET_MIPS)
> +#ifdef TARGET_ARCH_HAS_SA_RESTORER
>          __put_user(k->sa_restorer, &oact->sa_restorer);
>  #endif
>          /* Not swapped.  */
> @@ -787,7 +787,7 @@ int do_sigaction(int sig, const struct target_sigaction 
> *act,
>          /* FIXME: This is not threadsafe.  */
>          __get_user(k->_sa_handler, &act->_sa_handler);
>          __get_user(k->sa_flags, &act->sa_flags);
> -#if !defined(TARGET_MIPS)
> +#ifdef TARGET_ARCH_HAS_SA_RESTORER
>          __get_user(k->sa_restorer, &act->sa_restorer);
>  #endif
>          /* To be swapped in target_to_host_sigset.  */
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index 40c5027..8b1ad74 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -445,6 +445,7 @@ int do_sigaction(int sig, const struct target_sigaction 
> *act,
>  #define TARGET_SA_RESTART      2u
>  #define TARGET_SA_NODEFER      0x20u
>  #define TARGET_SA_RESETHAND    4u
> +#define TARGET_ARCH_HAS_SA_RESTORER 1
>  #elif defined(TARGET_MIPS)
>  #define TARGET_SA_NOCLDSTOP  0x00000001
>  #define TARGET_SA_NOCLDWAIT  0x00010000
> @@ -483,6 +484,10 @@ int do_sigaction(int sig, const struct target_sigaction 
> *act,
>  #define TARGET_SA_RESTORER   0x04000000
>  #endif
>  
> +#ifdef TARGET_SA_RESTORER
> +#define TARGET_ARCH_HAS_SA_RESTORER 1
> +#endif
> +
>  #if defined(TARGET_ALPHA)
>  
>  #define TARGET_SIGHUP            1
> @@ -718,19 +723,27 @@ struct target_sigaction {
>       abi_ulong       _sa_handler;
>  #endif
>       target_sigset_t sa_mask;
> +#ifdef TARGET_ARCH_HAS_SA_RESTORER
> +        /* ??? This is always present, but ignored unless O32.  */

If it is always present, why it is defined conditionally?

TARGET_ARCH_HAS_SA_RESTORER is defined only for O32.

> +        abi_ulong sa_restorer;
> +#endif
>  };
>  #else
>  struct target_old_sigaction {
>          abi_ulong _sa_handler;
>          abi_ulong sa_mask;
>          abi_ulong sa_flags;
> +#ifdef TARGET_ARCH_HAS_SA_RESTORER
>          abi_ulong sa_restorer;
> +#endif
>  };
>  
>  struct target_sigaction {
>          abi_ulong _sa_handler;
>          abi_ulong sa_flags;
> +#ifdef TARGET_ARCH_HAS_SA_RESTORER
>          abi_ulong sa_restorer;
> +#endif
>          target_sigset_t sa_mask;
>  };
>  #endif
> 

TARGET_ALPHA has a sa_restorer field, but TARGET_ARCH_HAS_SA_RESTORER is
not defined.

Laurent