[Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Alexander Graf]

We check whether the passed in counter value is negative on all calls
that involve g_posix_timers. However, we AND the value down to 16 bits
right before the check, so they can never be negative.

Simplify all the checks and remove the useless negativity check.

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

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index f6c887f..bb68dd4 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -9509,7 +9509,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 */
         arg1 &= 0xffff;
-        if (arg3 == 0 || arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
+        if (arg3 == 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {
             timer_t htimer = g_posix_timers[arg1];
@@ -9531,7 +9531,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
         arg1 &= 0xffff;
         if (!arg2) {
             return -TARGET_EFAULT;
-        } else if (arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
+        } else if (arg1 >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {
             timer_t htimer = g_posix_timers[arg1];
@@ -9551,7 +9551,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     {
         /* args: timer_t timerid */
         arg1 &= 0xffff;
-        if (arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
+        if (arg1 >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {
             timer_t htimer = g_posix_timers[arg1];
@@ -9566,7 +9566,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
     {
         /* args: timer_t timerid */
         arg1 &= 0xffff;
-        if (arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
+        if (arg1 >= ARRAY_SIZE(g_posix_timers)) {
             ret = -TARGET_EINVAL;
         } else {
             timer_t htimer = g_posix_timers[arg1];
-- 
1.7.12.4

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Peter Maydell]

On 22 August 2014 12:19, Alexander Graf <agraf@suse.de> wrote:
> We check whether the passed in counter value is negative on all calls
> that involve g_posix_timers. However, we AND the value down to 16 bits
> right before the check, so they can never be negative.

...but why exactly are we doing that AND with 0xffff ?? It seems
unlikely that the kernel really allows random garbage in the top
half of the timer ID arguments, so maybe we should drop the
mask and keep the <0 bounds checks?

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Andreas Färber]

Am 22.08.2014 13:33, schrieb Peter Maydell:
> On 22 August 2014 12:19, Alexander Graf <agraf@suse.de> wrote:
>> We check whether the passed in counter value is negative on all calls
>> that involve g_posix_timers. However, we AND the value down to 16 bits
>> right before the check, so they can never be negative.
> 
> ...but why exactly are we doing that AND with 0xffff ?? It seems
> unlikely that the kernel really allows random garbage in the top
> half of the timer ID arguments, so maybe we should drop the
> mask and keep the <0 bounds checks?

Or maybe just use a local int16_t variable? I.e., should 0xffff match
the <0 check there or not?

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Alexander Graf]

On 22.08.14 13:33, Peter Maydell wrote:
> On 22 August 2014 12:19, Alexander Graf <agraf@suse.de> wrote:
>> We check whether the passed in counter value is negative on all calls
>> that involve g_posix_timers. However, we AND the value down to 16 bits
>> right before the check, so they can never be negative.
> 
> ...but why exactly are we doing that AND with 0xffff ?? It seems
> unlikely that the kernel really allows random garbage in the top
> half of the timer ID arguments, so maybe we should drop the
> mask and keep the <0 bounds checks?

Or we drop the AND and and the <0 check and treat arg1 as unsigned ;).


Alex

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Peter Maydell]

On 22 August 2014 12:42, Alexander Graf <agraf@suse.de> wrote:
> On 22.08.14 13:33, Peter Maydell wrote:
>> On 22 August 2014 12:19, Alexander Graf <agraf@suse.de> wrote:
>>> We check whether the passed in counter value is negative on all calls
>>> that involve g_posix_timers. However, we AND the value down to 16 bits
>>> right before the check, so they can never be negative.
>>
>> ...but why exactly are we doing that AND with 0xffff ?? It seems
>> unlikely that the kernel really allows random garbage in the top
>> half of the timer ID arguments, so maybe we should drop the
>> mask and keep the <0 bounds checks?
>
> Or we drop the AND and and the <0 check and treat arg1 as unsigned ;).

That probably just requires equally many changes to
code that is currently correct because the arg* are
signed but would need changes if they became unsigned.

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Peter Maydell]

On 22 August 2014 12:36, Andreas Färber <afaerber@suse.de> wrote:
> Am 22.08.2014 13:33, schrieb Peter Maydell:
>> On 22 August 2014 12:19, Alexander Graf <agraf@suse.de> wrote:
>>> We check whether the passed in counter value is negative on all calls
>>> that involve g_posix_timers. However, we AND the value down to 16 bits
>>> right before the check, so they can never be negative.
>>
>> ...but why exactly are we doing that AND with 0xffff ?? It seems
>> unlikely that the kernel really allows random garbage in the top
>> half of the timer ID arguments, so maybe we should drop the
>> mask and keep the <0 bounds checks?
>
> Or maybe just use a local int16_t variable? I.e., should 0xffff match
> the <0 check there or not?

The kernel seems to use 'int' for the timer id type, which suggests
that this mask is just wrong.

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Alexander Graf]

On 22.08.14 13:44, Peter Maydell wrote:
> On 22 August 2014 12:42, Alexander Graf <agraf@suse.de> wrote:
>> On 22.08.14 13:33, Peter Maydell wrote:
>>> On 22 August 2014 12:19, Alexander Graf <agraf@suse.de> wrote:
>>>> We check whether the passed in counter value is negative on all calls
>>>> that involve g_posix_timers. However, we AND the value down to 16 bits
>>>> right before the check, so they can never be negative.
>>>
>>> ...but why exactly are we doing that AND with 0xffff ?? It seems
>>> unlikely that the kernel really allows random garbage in the top
>>> half of the timer ID arguments, so maybe we should drop the
>>> mask and keep the <0 bounds checks?
>>
>> Or we drop the AND and and the <0 check and treat arg1 as unsigned ;).
> 
> That probably just requires equally many changes to
> code that is currently correct because the arg* are
> signed but would need changes if they became unsigned.

Well, I do have a downstream patch that makes them unsigned, so I'd
rather like to make the code as stable to that as I can ;).


Alex

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Peter Maydell]

On 22 August 2014 12:45, Alexander Graf <agraf@suse.de> wrote:
> On 22.08.14 13:44, Peter Maydell wrote:
>> On 22 August 2014 12:42, Alexander Graf <agraf@suse.de> wrote:
>>> Or we drop the AND and and the <0 check and treat arg1 as unsigned ;).
>>
>> That probably just requires equally many changes to
>> code that is currently correct because the arg* are
>> signed but would need changes if they became unsigned.
>
> Well, I do have a downstream patch that makes them unsigned, so I'd
> rather like to make the code as stable to that as I can ;).

Yeah, I know. When I was looking through your patch tree
I saw that one and my reaction was "why on earth did
you do that?"...

-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Simplify boundary checks on g_posix_timers range

[Alexander Graf]

On 22.08.14 13:49, Peter Maydell wrote:
> On 22 August 2014 12:45, Alexander Graf <agraf@suse.de> wrote:
>> On 22.08.14 13:44, Peter Maydell wrote:
>>> On 22 August 2014 12:42, Alexander Graf <agraf@suse.de> wrote:
>>>> Or we drop the AND and and the <0 check and treat arg1 as unsigned ;).
>>>
>>> That probably just requires equally many changes to
>>> code that is currently correct because the arg* are
>>> signed but would need changes if they became unsigned.
>>
>> Well, I do have a downstream patch that makes them unsigned, so I'd
>> rather like to make the code as stable to that as I can ;).
> 
> Yeah, I know. When I was looking through your patch tree
> I saw that one and my reaction was "why on earth did
> you do that?"...

I don't fully remember all the glorious details either - and there's a
good reason I never pushed it upstream :). It seemed to make the code
more robust though.

Maybe we'll just ditch it again sooner or later. Or push it upstream and
make unsigned the default (which IMHO is a lot more sane, you get way
less unwanted side effects).


Alex