[Qemu-devel] [PATCH 2.2 v2 0/3] linux-user: Fix posix timer implementation

[Qemu-devel] [PATCH 2.2 v2 0/3] linux-user: Fix posix timer implementation

[Alexander Graf]

Tom recently realized that I accidently broke the posix timer functions.
With his simple test program he was able to show that we indeed do break
posix timers since my last commit touching them:

  http://csgraf.de/agraf/timer.c

These two patches fix it up properly. With these I can successfully run
his test case in any endian combination.

Please apply for 2.2.

v1 -> v2:

  - remove useless struct, change timer id type to s32
  - new patch: Use target_timer_t type for timer parameters
  - abort when magic is missing

Alex

Alexander Graf (3):
  linux-user: Fix timer_create timer id return type
  linux-user: Properly handle timer magic offset
  linux-user: Use target_timer_t type for timer parameters

 linux-user/syscall.c      | 28 ++++++++++++++++++++++------
 linux-user/syscall_defs.h |  5 +----
 2 files changed, 23 insertions(+), 10 deletions(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 2.2 v2 1/3] linux-user: Fix timer_create timer id return type

[Alexander Graf]

The linux syscall to create_timer really gets an s32* parameter to store
its timer id into, not an abi_ulong*. Remove the useless struct encapsulation
and directly write the 32bit value into memory.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall.c      | 5 +----
 linux-user/syscall_defs.h | 5 +----
 2 files changed, 2 insertions(+), 8 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a175cc1..d012c71 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9579,7 +9579,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 +9600,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(0xcafe0000 | timer_index, arg3, target_timer_t)) {
                     goto efault;
                 }
-                ptarget_timer->ptr = tswap32(0xcafe0000 | timer_index);
-                unlock_user_struct(ptarget_timer, arg3, 1);
             }
         }
         break;
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

Re: [Qemu-devel] [PATCH 2.2 v2 1/3] linux-user: Fix timer_create timer id return type

[Peter Maydell]

On 10 November 2014 17:44, Alexander Graf <agraf@suse.de> wrote:
> The linux syscall to create_timer really gets an s32* parameter to store
> its timer id into, not an abi_ulong*. Remove the useless struct encapsulation
> and directly write the 32bit value into memory.

> @@ -9601,11 +9600,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(0xcafe0000 | timer_index, arg3, target_timer_t)) {
>                      goto efault;
>                  }
> -                ptarget_timer->ptr = tswap32(0xcafe0000 | timer_index);
> -                unlock_user_struct(ptarget_timer, arg3, 1);
>              }
>          }
>          break;
> 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;

This means that all our valid timer_t values will be negative
(since the magic value we've picked happens to have bit 31 set),
whereas the kernel is careful to keep all its valid timer_t
values >= 0. That kind of worries me, since I could see userspace
code making signedness assumptions. Can we use a mask value
that doesn't set bit 31?

(It also makes it easier to reason about the bits of QEMU
code which take timer_t arguments; in particular notice that
        target_ulong timerid = arg1;
is likely to end up with 0xffffffffcafexxxx due to sign
extension on 64 bit guests.)

-- PMM

[Qemu-devel] [PATCH 2.2 v2 2/3] linux-user: Properly handle timer magic offset

[Alexander Graf]

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.

Reported-by: Tom Musta <tommusta@gmail.com>
Signed-off-by: Alexander Graf <agraf@suse.de>

---

v1 -> v2:

  - Abort when magic is missing
---
 linux-user/syscall.c | 21 ++++++++++++++++++++-
 1 file changed, 20 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index d012c71..b7f12b6 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9573,6 +9573,9 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     }
 #endif
 
+#define TIMER_MAGIC 0xcafe0000
+#define TIMER_MAGIC_MASK 0xffff0000
+
 #ifdef TARGET_NR_timer_create
     case TARGET_NR_timer_create:
     {
@@ -9600,7 +9603,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
             if (ret) {
                 phtimer = NULL;
             } else {
-                if (put_user(0xcafe0000 | timer_index, arg3, target_timer_t)) {
+                if (put_user(TIMER_MAGIC | timer_index, arg3, target_timer_t)) {
                     goto efault;
                 }
             }
@@ -9616,6 +9619,14 @@ 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;
+        } else {
+            ret = -TARGET_EINVAL;
+            break;
+        }
+
         if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {
@@ -9637,6 +9648,14 @@ 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;
+        } else {
+            ret = -TARGET_EINVAL;
+            break;
+        }
+
         if (!arg2) {
             return -TARGET_EFAULT;
         } else if (timerid >= ARRAY_SIZE(g_posix_timers)) {
-- 
1.7.12.4

[Qemu-devel] [PATCH 2.2 v2 3/3] linux-user: Use target_timer_t type for timer parameters

[Alexander Graf]

The timer_gettime and timer_settime syscalls get a timer_t parameter. Since
we now have an internal wrapper type for it, let's make use of it.

We do not need to check for signed < 0 overflows, because we already mask out
the upper bits of the value, rendering the number always unsigned.

Signed-off-by: Alexander Graf <agraf@suse.de>
---
 linux-user/syscall.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index b7f12b6..96d74df 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9617,7 +9617,7 @@ 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) {
@@ -9646,7 +9646,7 @@ 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) {
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH 2.2 v2 3/3] linux-user: Use target_timer_t type for timer parameters

[Peter Maydell]

On 10 November 2014 17:44, Alexander Graf <agraf@suse.de> wrote:
> The timer_gettime and timer_settime syscalls get a timer_t parameter. Since
> we now have an internal wrapper type for it, let's make use of it.
>
> We do not need to check for signed < 0 overflows, because we already mask out
> the upper bits of the value, rendering the number always unsigned.
>
> Signed-off-by: Alexander Graf <agraf@suse.de>

Personally I think these three patches would actually be easier
to review merged into a single patch at this point.

-- PMM