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

[Laurent Vivier]

Hi,

 

as in the kernel timer_t is an "int" (as said PMM), you should cast to "int" to remove garbage on 64bit hosts and check sign ...

 

Regards,

Laurent

> Le 22 août 2014 à 13:56, Alexander Graf <address@hidden> a écrit :
>
>
> We check whether the passed in timer id is negative on all calls
> that involve g_posix_timers.
>
> However, these checks are bogus. First off we limit the timer_id to
> 16 bits which is not what Linux does. Then we check whether it's negative
> which it can't be because we masked it.
>
> We can safely remove the masking. For the negativity check we can just
> treat the timerid as unsigned and only check for upper boundaries.
>
> Signed-off-by: Alexander Graf <address@hidden>
>
> ---
>
> v1 -> v2:
>
> - drop 0xffff mask
> - explicitly cast to unsigned because the mask is missing now
>
> ---
> linux-user/syscall.c | 30 +++++++++++++++++-------------
> 1 file changed, 17 insertions(+), 13 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index f6c887f..92b6a38 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -9508,11 +9508,12 @@ 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)) {
> + target_ulong timerid = arg1;
> +
> + if (arg3 == 0 || timerid >= ARRAY_SIZE(g_posix_timers)) {
> ret = -TARGET_EINVAL;
> } else {
> - timer_t htimer = g_posix_timers[arg1];
> + timer_t htimer = g_posix_timers[timerid];
> struct itimerspec hspec_new = {{0},}, hspec_old = {{0},};
>
> target_to_host_itimerspec(&hspec_new, arg3);
> @@ -9528,13 +9529,14 @@ 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 */
> - arg1 &= 0xffff;
> + target_ulong timerid = arg1;
> +
> if (!arg2) {
> return -TARGET_EFAULT;
> - } else if (arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
> + } else if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> ret = -TARGET_EINVAL;
> } else {
> - timer_t htimer = g_posix_timers[arg1];
> + timer_t htimer = g_posix_timers[timerid];
> struct itimerspec hspec;
> ret = get_errno(timer_gettime(htimer, &hspec));
>
> @@ -9550,11 +9552,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> case TARGET_NR_timer_getoverrun:
> {
> /* args: timer_t timerid */
> - arg1 &= 0xffff;
> - if (arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
> + target_ulong timerid = arg1;
> +
> + if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> ret = -TARGET_EINVAL;
> } else {
> - timer_t htimer = g_posix_timers[arg1];
> + timer_t htimer = g_posix_timers[timerid];
> ret = get_errno(timer_getoverrun(htimer));
> }
> break;
> @@ -9565,13 +9568,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> case TARGET_NR_timer_delete:
> {
> /* args: timer_t timerid */
> - arg1 &= 0xffff;
> - if (arg1 < 0 || arg1 >= ARRAY_SIZE(g_posix_timers)) {
> + target_ulong timerid = arg1;
> +
> + if (timerid >= ARRAY_SIZE(g_posix_timers)) {
> ret = -TARGET_EINVAL;
> } else {
> - timer_t htimer = g_posix_timers[arg1];
> + timer_t htimer = g_posix_timers[timerid];
> ret = get_errno(timer_delete(htimer));
> - g_posix_timers[arg1] = 0;
> + g_posix_timers[timerid] = 0;
> }
> break;
> }
> --
> 1.7.12.4
>
>