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

[Qemu-devel] [PATCH 2.2 0/2] 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.

Alex

Alexander Graf (2):
  linux-user: Fix timer creation tswap
  linux-user: Properly handle timer magic offset

 linux-user/syscall.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
1.7.12.4

[Qemu-devel] [PATCH 2.2 1/2] linux-user: Fix timer creation tswap

[Alexander Graf]

The timer pointer field we're writing a new timer ID into is of field
abi_ulong which means we need to swab 64bits on 64bit ABI targets.

Change the tswap from 32 to l to give us correct foreign-endian results.

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

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index a175cc1..f3e22c8 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9604,7 +9604,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 if (!lock_user_struct(VERIFY_WRITE, ptarget_timer, arg3, 1)) {
                     goto efault;
                 }
-                ptarget_timer->ptr = tswap32(0xcafe0000 | timer_index);
+                ptarget_timer->ptr = tswapl(0xcafe0000 | timer_index);
                 unlock_user_struct(ptarget_timer, arg3, 1);
             }
         }
-- 
1.7.12.4

[Qemu-devel] [PATCH 2.2 2/2] 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>
---
 linux-user/syscall.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f3e22c8..78896dc 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:
     {
@@ -9604,7 +9607,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
                 if (!lock_user_struct(VERIFY_WRITE, ptarget_timer, arg3, 1)) {
                     goto efault;
                 }
-                ptarget_timer->ptr = tswapl(0xcafe0000 | timer_index);
+                ptarget_timer->ptr = tswapl(TIMER_MAGIC | timer_index);
                 unlock_user_struct(ptarget_timer, arg3, 1);
             }
         }
@@ -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;
+        }
+
         if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) {
             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;
+        }
+
         if (!arg2) {
             return -TARGET_EFAULT;
         } else if (timerid >= ARRAY_SIZE(g_posix_timers)) {
-- 
1.7.12.4

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

[Peter Maydell]

On 10 November 2014 16:46, 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 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.)

> +
>          if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) {
>              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

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

[Alexander Graf]

On 10.11.14 17:55, Peter Maydell wrote:
> On 10 November 2014 16:46, 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 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
>