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