| Message ID | 1408706366-43407-1-git-send-email-agraf@suse.de |
|---|---|
| State | New |
| Headers | show |
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
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
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
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
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
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
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
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
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];
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(-)