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 <tommu...@gmail.com> > Signed-off-by: Alexander Graf <ag...@suse.de> > > --- > > 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 <tommu...@gmail.com> Tested-by: Tom Musta <tommu...@gmail.com>