From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:46129) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnsLo-0004ap-6s for qemu-devel@nongnu.org; Mon, 10 Nov 2014 12:02:06 -0500 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XnsLi-0001AO-06 for qemu-devel@nongnu.org; Mon, 10 Nov 2014 12:02:00 -0500 Received: from cantor2.suse.de ([195.135.220.15]:41619 helo=mx2.suse.de) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XnsLh-0001AF-Qf for qemu-devel@nongnu.org; Mon, 10 Nov 2014 12:01:53 -0500 Message-ID: <5460EF7F.5010303@suse.de> Date: Mon, 10 Nov 2014 18:01:52 +0100 From: Alexander Graf MIME-Version: 1.0 References: <1415637969-47244-1-git-send-email-agraf@suse.de> <1415637969-47244-3-git-send-email-agraf@suse.de> In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 2.2 2/2] linux-user: Properly handle timer magic offset List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: Tom Musta , Riku Voipio , QEMU Developers On 10.11.14 17:55, Peter Maydell wrote: > On 10 November 2014 16:46, 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 is superior to the plain masking in that it also adds validity checks >> against the timer id to ensure we're always dealing with an actual timer id >> created by QEMU. > > The commit message says this... > >> @@ -9619,6 +9622,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, >> * struct itimerspec * old_value */ >> target_ulong timerid = arg1; >> >> + /* Convert QEMU provided timer ID back to internal 16bit index format */ >> + if ((timerid & TIMER_MAGIC_MASK) == TIMER_MAGIC) { >> + timerid &= 0xffff; >> + } > > ...but the code doesn't actually fail EINVAL in the case that the > magic value doesn't match, so if you just pass in a small integer > for the timerid that will work even though the magic is wrong. > (You can't actually make it overflow the array this way, obviously.) Well, it's not exactly horribly obvious, but... > >> + >> if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) { ... this check will catch cases where the magic is not masked out ;). So the only case we don't catch is when a user passes a number < 32 as timer id - that will work still and IMHO it's desirable to maintain that behavior as Linux (some times) treats timer ids as u16. Alex >> ret = -TARGET_EINVAL; >> } else { >> @@ -9640,6 +9648,11 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, >> /* args: timer_t timerid, struct itimerspec *curr_value */ >> target_ulong timerid = arg1; >> >> + /* Convert QEMU provided timer ID back to internal 16bit index format */ >> + if ((timerid & TIMER_MAGIC_MASK) == TIMER_MAGIC) { >> + timerid &= 0xffff; >> + } >> + > > Same here. > > thanks > - PMM >