From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:50199) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnwfT-0000xN-FZ for qemu-devel@nongnu.org; Mon, 10 Nov 2014 16:38:41 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XnwfN-0007sd-Oz for qemu-devel@nongnu.org; Mon, 10 Nov 2014 16:38:35 -0500 Received: from mail-qg0-x22d.google.com ([2607:f8b0:400d:c04::22d]:60587) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnwfN-0007sW-J3 for qemu-devel@nongnu.org; Mon, 10 Nov 2014 16:38:29 -0500 Received: by mail-qg0-f45.google.com with SMTP id z107so6120811qgd.4 for ; Mon, 10 Nov 2014 13:38:29 -0800 (PST) Message-ID: <5461304D.3080100@gmail.com> Date: Mon, 10 Nov 2014 15:38:21 -0600 From: Tom Musta MIME-Version: 1.0 References: <1415651583-64332-1-git-send-email-agraf@suse.de> In-Reply-To: <1415651583-64332-1-git-send-email-agraf@suse.de> Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH] 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 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 > 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 > > 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_. */ > @@ -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 Tested-by: Tom Musta