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>
