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 <address@hidden> 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
>