Re: [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling

[Tom Musta]

On 11/10/2014 12:21 PM, Alexander Graf wrote:
> When creating a timer handle, we give the timer id a special magic offset
> of 0xcafe0000. However, we never mask that offset out of the timer id before
> we start using it to dereference our timer array. So we always end up aborting
> timer operations because the timer id is out of bounds.
> 
> This was not an issue before my patch e52a99f756e ("linux-user: Simplify
> timerid checks on g_posix_timers range") because before we would blindly mask
> anything above the first 16 bits.
> 
> This patch simplifies the code around timer id creation by introducing a 
> proper
> target_timer_id typedef that is s32, just like Linux has it. It also changes 
> the
> magic offset to a value that makes all timer ids be positive.
> 
> Reported-by: Tom Musta <address@hidden>
> Signed-off-by: Alexander Graf <address@hidden>
> 
> ---
> 
> v1 -> v2:
> 
>   - Abort when magic is missing
> 
> v2 -> v3:
> 
>   - Squash into a single patch
>   - Change magic to always have positive IDs
> ---
>  linux-user/syscall.c      | 28 ++++++++++++++++++++++------
>  linux-user/syscall_defs.h |  5 +----
>  2 files changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a175cc1..c21262f 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9573,13 +9573,15 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      }
>  #endif
>  
> +#define TIMER_MAGIC 0x0caf0000
> +#define TIMER_MAGIC_MASK 0xffff0000
> +
>  #ifdef TARGET_NR_timer_create
>      case TARGET_NR_timer_create:
>      {
>          /* args: clockid_t clockid, struct sigevent *sevp, timer_t *timerid 
> */
>  
>          struct sigevent host_sevp = { {0}, }, *phost_sevp = NULL;
> -        struct target_timer_t *ptarget_timer;
>  
>          int clkid = arg1;
>          int timer_index = next_free_host_timer();
> @@ -9601,11 +9603,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>              if (ret) {
>                  phtimer = NULL;
>              } else {
> -                if (!lock_user_struct(VERIFY_WRITE, ptarget_timer, arg3, 1)) 
> {
> +                if (put_user(TIMER_MAGIC | timer_index, arg3, 
> target_timer_t)) {
>                      goto efault;
>                  }
> -                ptarget_timer->ptr = tswap32(0xcafe0000 | timer_index);
> -                unlock_user_struct(ptarget_timer, arg3, 1);
>              }
>          }
>          break;
> @@ -9617,7 +9617,15 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      {
>          /* args: timer_t timerid, int flags, const struct itimerspec 
> *new_value,
>           * struct itimerspec * old_value */
> -        target_ulong timerid = arg1;
> +        target_timer_t timerid = arg1;
> +
> +        /* Convert QEMU provided timer ID back to internal 16bit index 
> format */
> +        if ((timerid & TIMER_MAGIC_MASK) == TIMER_MAGIC) {
> +            timerid &= 0xffff;
> +        } else {
> +            ret = -TARGET_EINVAL;
> +            break;
> +        }
>  
>          if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) {
>              ret = -TARGET_EINVAL;
> @@ -9638,7 +9646,15 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      case TARGET_NR_timer_gettime:
>      {
>          /* args: timer_t timerid, struct itimerspec *curr_value */
> -        target_ulong timerid = arg1;
> +        target_timer_t timerid = arg1;
> +
> +        /* Convert QEMU provided timer ID back to internal 16bit index 
> format */
> +        if ((timerid & TIMER_MAGIC_MASK) == TIMER_MAGIC) {
> +            timerid &= 0xffff;
> +        } else {
> +            ret = -TARGET_EINVAL;
> +            break;
> +        }
>  
>          if (!arg2) {
>              return -TARGET_EFAULT;
> diff --git a/linux-user/syscall_defs.h b/linux-user/syscall_defs.h
> index c9e6323..ebb3be1 100644
> --- a/linux-user/syscall_defs.h
> +++ b/linux-user/syscall_defs.h
> @@ -2564,10 +2564,7 @@ struct target_ucred {
>  
>  #endif
>  
> -
> -struct target_timer_t {
> -    abi_ulong ptr;
> -};
> +typedef int32_t target_timer_t;
>  
>  #define TARGET_SIGEV_MAX_SIZE 64
>  
> 

There are two more syscalls that also need this decoding (timer_getoverrun,
timer_delete).  So assuming you add this:

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index c21262f..076131a 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9679,6 +9679,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
         /* args: timer_t timerid */
         target_ulong timerid = arg1;

+        /* Convert QEMU provided timer ID back to internal 16bit index format 
*/
+        if ((timerid & TIMER_MAGIC_MASK) == TIMER_MAGIC) {
+            timerid &= 0xffff;
+        } else {
+            ret = -TARGET_EINVAL;
+            break;
+        }
+
         if (timerid >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {
@@ -9695,6 +9703,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
arg1,
         /* args: timer_t timerid */
         target_ulong timerid = arg1;

+        /* Convert QEMU provided timer ID back to internal 16bit index format 
*/
+        if ((timerid & TIMER_MAGIC_MASK) == TIMER_MAGIC) {
+            timerid &= 0xffff;
+        } else {
+            ret = -TARGET_EINVAL;
+            break;
+        }
+
         if (timerid >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {


Then I will add my:

Reviewed-by: Tom Musta <address@hidden>
Tested-by: Tom Musta <address@hidden>