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

[Tom Musta]

On 11/10/2014 2:33 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
> 
> v3 -> v4:
> 
>   - Also handle timer_getoverrun and timer_delete
>   - Move timer boundary checks into separate function
> 
> v4 -> v5:
> 
>   - Fix stupid thinko that made boundary checks always fail
> ---
>  linux-user/syscall.c      | 54 
> ++++++++++++++++++++++++++++++++---------------
>  linux-user/syscall_defs.h |  5 +----
>  2 files changed, 38 insertions(+), 21 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index a175cc1..aaac6a2 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -5473,6 +5473,27 @@ static int do_openat(void *cpu_env, int dirfd, const 
> char *pathname, int flags,
>      return get_errno(sys_openat(dirfd, path(pathname), flags, mode));
>  }
>  
> +#define TIMER_MAGIC 0x0caf0000
> +#define TIMER_MAGIC_MASK 0xffff0000
> +
> +/* Convert QEMU provided timer ID back to internal 16bit index format */
> +static target_timer_t get_timer_id(abi_long arg)
> +{
> +    target_timer_t timerid = arg;
> +
> +    if ((timerid & TIMER_MAGIC_MASK) != TIMER_MAGIC) {
> +        return -TARGET_EINVAL;
> +    }
> +
> +    timerid &= 0xffff;
> +
> +    if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> +        return -TARGET_EINVAL;
> +    }
> +
> +    return timerid;
> +}
> +
>  /* do_syscall() should always have a single exit point at the end so
>     that actions, such as logging of syscall results, can be performed.
>     All errnos that do_syscall() returns must be -TARGET_<errcode>. */
> @@ -9579,7 +9600,6 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>          /* 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 +9621,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,9 +9635,11 @@ 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 = get_timer_id(arg1);
>  
> -        if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) {
> +        if (timerid < 0) {
> +            ret = timerid;
> +        } else if (arg3 == 0) {
>              ret = -TARGET_EINVAL;
>          } else {
>              timer_t htimer = g_posix_timers[timerid];
> @@ -9638,12 +9658,12 @@ 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 = get_timer_id(arg1);
>  
> -        if (!arg2) {
> -            return -TARGET_EFAULT;
> -        } else if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> -            ret = -TARGET_EINVAL;
> +        if (timerid < 0) {
> +            ret = timerid;
> +        } else if (!arg2) {
> +            ret = -TARGET_EFAULT;
>          } else {
>              timer_t htimer = g_posix_timers[timerid];
>              struct itimerspec hspec;
> @@ -9661,10 +9681,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      case TARGET_NR_timer_getoverrun:
>      {
>          /* args: timer_t timerid */
> -        target_ulong timerid = arg1;
> +        target_timer_t timerid = get_timer_id(arg1);
>  
> -        if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> -            ret = -TARGET_EINVAL;
> +        if (timerid < 0) {
> +            ret = timerid;
>          } else {
>              timer_t htimer = g_posix_timers[timerid];
>              ret = get_errno(timer_getoverrun(htimer));
> @@ -9677,10 +9697,10 @@ abi_long do_syscall(void *cpu_env, int num, abi_long 
> arg1,
>      case TARGET_NR_timer_delete:
>      {
>          /* args: timer_t timerid */
> -        target_ulong timerid = arg1;
> +        target_timer_t timerid = get_timer_id(arg1);
>  
> -        if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> -            ret = -TARGET_EINVAL;
> +        if (timerid < 0) {
> +            ret = timerid;
>          } else {
>              timer_t htimer = g_posix_timers[timerid];
>              ret = get_errno(timer_delete(htimer));
> 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
>  
> 

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