| Message ID | 20180123144807.5618-8-laurent@vivier.eu (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 23 January 2018 at 14:48, Laurent Vivier <laurent@vivier.eu> wrote: > From: Samuel Thibault <samuel.thibault@ens-lyon.org> > > sched_get/setaffinity linux-user syscalls were missing conversions for > little/big endian, which is hairy since longs may not be the same size > either. > > For simplicity, this just introduces loops to convert bit by bit like is > done for select. > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > Message-Id: <20180109201643.1479-1-samuel.thibault@ens-lyon.org> > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > --- > @@ -10395,9 +10463,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > ret = arg2; > } > > - if (copy_to_user(arg3, mask, ret)) { > - goto efault; > - } > + ret = host_to_target_cpu_mask(mask, mask_size, arg3, arg2); > } > } > break; Hi -- Coverity spots that in this change, we now have a case where we set "ret = arg2;" which then immediately is replaced by "ret = host_to_target_cpu_mask(mask, mask_size, arg3, arg2);", making the first assignment pointless. It looks like we're now ignoring the host filled buffer size that is returned by sys_sched_getaffinity() and then adjusted by this bit of code. Shouldn't we be using that value in this new host_to_target_cpu_mask() code? thanks -- PMM
Peter Maydell, on ven. 26 janv. 2018 18:23:02 +0000, wrote: > On 23 January 2018 at 14:48, Laurent Vivier <laurent@vivier.eu> wrote: > > From: Samuel Thibault <samuel.thibault@ens-lyon.org> > > > > sched_get/setaffinity linux-user syscalls were missing conversions for > > little/big endian, which is hairy since longs may not be the same size > > either. > > > > For simplicity, this just introduces loops to convert bit by bit like is > > done for select. > > > > Signed-off-by: Samuel Thibault <samuel.thibault@ens-lyon.org> > > Reviewed-by: Laurent Vivier <laurent@vivier.eu> > > Message-Id: <20180109201643.1479-1-samuel.thibault@ens-lyon.org> > > Signed-off-by: Laurent Vivier <laurent@vivier.eu> > > --- > > > @@ -10395,9 +10463,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > > ret = arg2; > > } > > > > - if (copy_to_user(arg3, mask, ret)) { > > - goto efault; > > - } > > + ret = host_to_target_cpu_mask(mask, mask_size, arg3, arg2); > > } > > } > > break; > > Hi -- Coverity spots that in this change, we now have a case > where we set "ret = arg2;" which then immediately is replaced > by "ret = host_to_target_cpu_mask(mask, mask_size, arg3, arg2);", > making the first assignment pointless. > > It looks like we're now ignoring the host filled buffer size > that is returned by sys_sched_getaffinity() and then adjusted > by this bit of code. Shouldn't we be using that value in this > new host_to_target_cpu_mask() code? Indeed, will send a patch against this. Samuel
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 41ded90ee6..143e4a959d 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -7731,6 +7731,73 @@ static TargetFdTrans target_inotify_trans = { }; #endif +static int target_to_host_cpu_mask(unsigned long *host_mask, + size_t host_size, + abi_ulong target_addr, + size_t target_size) +{ + unsigned target_bits = sizeof(abi_ulong) * 8; + unsigned host_bits = sizeof(*host_mask) * 8; + abi_ulong *target_mask; + unsigned i, j; + + assert(host_size >= target_size); + + target_mask = lock_user(VERIFY_READ, target_addr, target_size, 1); + if (!target_mask) { + return -TARGET_EFAULT; + } + memset(host_mask, 0, host_size); + + for (i = 0 ; i < target_size / sizeof(abi_ulong); i++) { + unsigned bit = i * target_bits; + abi_ulong val; + + __get_user(val, &target_mask[i]); + for (j = 0; j < target_bits; j++, bit++) { + if (val & (1UL << j)) { + host_mask[bit / host_bits] |= 1UL << (bit % host_bits); + } + } + } + + unlock_user(target_mask, target_addr, 0); + return 0; +} + +static int host_to_target_cpu_mask(const unsigned long *host_mask, + size_t host_size, + abi_ulong target_addr, + size_t target_size) +{ + unsigned target_bits = sizeof(abi_ulong) * 8; + unsigned host_bits = sizeof(*host_mask) * 8; + abi_ulong *target_mask; + unsigned i, j; + + assert(host_size >= target_size); + + target_mask = lock_user(VERIFY_WRITE, target_addr, target_size, 0); + if (!target_mask) { + return -TARGET_EFAULT; + } + + for (i = 0 ; i < target_size / sizeof(abi_ulong); i++) { + unsigned bit = i * target_bits; + abi_ulong val = 0; + + for (j = 0; j < target_bits; j++, bit++) { + if (host_mask[bit / host_bits] & (1UL << (bit % host_bits))) { + val |= 1UL << j; + } + } + __put_user(val, &target_mask[i]); + } + + unlock_user(target_mask, target_addr, target_size); + return 0; +} + /* do_syscall() should always have a single exit point at the end so that actions, such as logging of syscall results, can be performed. All errnos that do_syscall() returns must be -TARGET_<errcode>. */ @@ -10376,6 +10443,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, mask_size = (arg2 + (sizeof(*mask) - 1)) & ~(sizeof(*mask) - 1); mask = alloca(mask_size); + memset(mask, 0, mask_size); ret = get_errno(sys_sched_getaffinity(arg1, mask_size, mask)); if (!is_error(ret)) { @@ -10395,9 +10463,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, ret = arg2; } - if (copy_to_user(arg3, mask, ret)) { - goto efault; - } + ret = host_to_target_cpu_mask(mask, mask_size, arg3, arg2); } } break; @@ -10415,13 +10481,12 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, break; } mask_size = (arg2 + (sizeof(*mask) - 1)) & ~(sizeof(*mask) - 1); - mask = alloca(mask_size); - if (!lock_user_struct(VERIFY_READ, p, arg3, 1)) { - goto efault; + + ret = target_to_host_cpu_mask(mask, mask_size, arg3, arg2); + if (ret) { + break; } - memcpy(mask, p, arg2); - unlock_user_struct(p, arg2, 0); ret = get_errno(sys_sched_setaffinity(arg1, mask_size, mask)); }