* [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling
@ 2014-11-10 18:21 Alexander Graf
2014-11-10 18:24 ` Peter Maydell
2014-11-10 19:33 ` Tom Musta
0 siblings, 2 replies; 3+ messages in thread
From: Alexander Graf @ 2014-11-10 18:21 UTC (permalink / raw)
To: qemu-devel; +Cc: tommusta, riku.voipio, peter.maydell
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 <tommusta@gmail.com>
Signed-off-by: Alexander Graf <agraf@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
--
1.7.12.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling
2014-11-10 18:21 [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling Alexander Graf
@ 2014-11-10 18:24 ` Peter Maydell
2014-11-10 19:33 ` Tom Musta
1 sibling, 0 replies; 3+ messages in thread
From: Peter Maydell @ 2014-11-10 18:24 UTC (permalink / raw)
To: Alexander Graf; +Cc: Tom Musta, Riku Voipio, QEMU Developers
On 10 November 2014 18:21, Alexander Graf <agraf@suse.de> 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 <tommusta@gmail.com>
> Signed-off-by: Alexander Graf <agraf@suse.de>
Reviewed-by: Peter Maydell <peter.maydell@linaro.org>
Thanks for putting up with all my nitpicking...
-- PMM
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling
2014-11-10 18:21 [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling Alexander Graf
2014-11-10 18:24 ` Peter Maydell
@ 2014-11-10 19:33 ` Tom Musta
1 sibling, 0 replies; 3+ messages in thread
From: Tom Musta @ 2014-11-10 19:33 UTC (permalink / raw)
To: Alexander Graf, qemu-devel; +Cc: peter.maydell, riku.voipio
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 <tommusta@gmail.com>
> Signed-off-by: Alexander Graf <agraf@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 <tommusta@gmail.com>
Tested-by: Tom Musta <tommusta@gmail.com>
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2014-11-10 19:33 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-11-10 18:21 [Qemu-devel] [PATCH 2.2 v3] linux-user: Fix up timer id handling Alexander Graf
2014-11-10 18:24 ` Peter Maydell
2014-11-10 19:33 ` Tom Musta
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).