From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:55168) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnuiT-0002Xn-MH for qemu-devel@nongnu.org; Mon, 10 Nov 2014 14:33:39 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XnuiN-00051e-TX for qemu-devel@nongnu.org; Mon, 10 Nov 2014 14:33:33 -0500 Received: from mail-qc0-x232.google.com ([2607:f8b0:400d:c01::232]:39271) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnuiN-00051a-Ns for qemu-devel@nongnu.org; Mon, 10 Nov 2014 14:33:27 -0500 Received: by mail-qc0-f178.google.com with SMTP id b13so6959260qcw.23 for ; Mon, 10 Nov 2014 11:33:25 -0800 (PST) Message-ID: <546112FA.20105@gmail.com> Date: Mon, 10 Nov 2014 13:33:14 -0600 From: Tom Musta MIME-Version: 1.0 References: <1415643672-55110-1-git-send-email-agraf@suse.de> In-Reply-To: <1415643672-55110-1-git-send-email-agraf@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org, riku.voipio@iki.fi 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 > Signed-off-by: Alexander Graf > > --- > > 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 Tested-by: Tom Musta