| Message ID | 1465239499-5048-15-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | New |
| Headers | show |
Le 06/06/2016 à 20:58, Peter Maydell a écrit : > Use the __get_user() and __put_user() to handle reading and writing the > guest structures in do_ioctl(). This has two benefits: > * avoids possible errors due to misaligned guest pointers > * correctly sign extends signed fields (like l_start in struct flock) > which might be different sizes between guest and host > > To do this we abstract out into copy_from/to_user functions. We > also standardize on always using host flock64 and the F_GETLK64 > etc flock commands, as this means we always have 64 bit offsets > whether the host is 64-bit or 32-bit and we don't need to support > conversion to both host struct flock and struct flock64. > > In passing we fix errors in converting l_type from the host to > the target (where we were doing a byteswap of the host value > before trying to do the convert-bitmasks operation rather than > otherwise, and inexplicably shifting left by 1). I think the ">> 1" is coming from: 43f238d Support fcntl F_GETLK64, F_SETLK64, F_SETLKW64 to convert arm to x86, and should have been removed then in: 2ba7f73 alpha-linux-user: Translate fcntl l_type So yes, the ">> 1" is wrong. I don't understand how it can work. > > Signed-off-by: Peter Maydell <peter.maydell@linaro.org> > --- > linux-user/syscall.c | 280 +++++++++++++++++++++++++++++---------------------- > 1 file changed, 157 insertions(+), 123 deletions(-) > > diff --git a/linux-user/syscall.c b/linux-user/syscall.c > index 4cf67c8..f3a487e 100644 > --- a/linux-user/syscall.c > +++ b/linux-user/syscall.c > @@ -4894,11 +4894,11 @@ static int target_to_host_fcntl_cmd(int cmd) > case TARGET_F_SETFL: > return cmd; > case TARGET_F_GETLK: > - return F_GETLK; > - case TARGET_F_SETLK: > - return F_SETLK; > - case TARGET_F_SETLKW: > - return F_SETLKW; > + return F_GETLK64; > + case TARGET_F_SETLK: > + return F_SETLK64; > + case TARGET_F_SETLKW: > + return F_SETLKW64; > case TARGET_F_GETOWN: > return F_GETOWN; > case TARGET_F_SETOWN: I see no reason to have this in this patch. > @@ -4949,12 +4949,134 @@ static const bitmask_transtbl flock_tbl[] = { > { 0, 0, 0, 0 } > }; > > -static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > +static inline abi_long copy_from_user_flock(struct flock64 *fl, > + abi_ulong target_flock_addr) > { > - struct flock fl; > struct target_flock *target_fl; > + short l_type; > + > + if (!lock_user_struct(VERIFY_READ, target_fl, target_flock_addr, 1)) { > + return -TARGET_EFAULT; > + } > + > + __get_user(l_type, &target_fl->l_type); > + fl->l_type = target_to_host_bitmask(l_type, flock_tbl); > + __get_user(fl->l_whence, &target_fl->l_whence); > + __get_user(fl->l_start, &target_fl->l_start); > + __get_user(fl->l_len, &target_fl->l_len); > + __get_user(fl->l_pid, &target_fl->l_pid); > + unlock_user_struct(target_fl, target_flock_addr, 0); > + return 0; > +} > + > +static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr, > + const struct flock64 *fl) > +{ > + struct target_flock *target_fl; > + short l_type; > + > + if (!lock_user_struct(VERIFY_WRITE, target_fl, target_flock_addr, 0)) { > + return -TARGET_EFAULT; > + } > + > + l_type = host_to_target_bitmask(fl->l_type, flock_tbl); > + __put_user(l_type, &target_fl->l_type); > + __put_user(fl->l_whence, &target_fl->l_whence); > + __put_user(fl->l_start, &target_fl->l_start); > + __put_user(fl->l_len, &target_fl->l_len); > + __put_user(fl->l_pid, &target_fl->l_pid); > + unlock_user_struct(target_fl, target_flock_addr, 1); > + return 0; > +} > + > +typedef abi_long from_flock64_fn(struct flock64 *fl, abi_ulong target_addr); > +typedef abi_long to_flock64_fn(abi_ulong target_addr, const struct flock64 *fl); > + > +#ifdef TARGET_ARM > +static inline abi_long copy_from_user_eabi_flock64(struct flock64 *fl, > + abi_ulong target_flock_addr) > +{ > + struct target_eabi_flock64 *target_fl; > + short l_type; > + > + if (!lock_user_struct(VERIFY_READ, target_fl, target_flock_addr, 1)) { > + return -TARGET_EFAULT; > + } > + > + __get_user(l_type, &target_fl->l_type); > + fl->l_type = target_to_host_bitmask(l_type, flock_tbl); > + __get_user(fl->l_whence, &target_fl->l_whence); > + __get_user(fl->l_start, &target_fl->l_start); > + __get_user(fl->l_len, &target_fl->l_len); > + __get_user(fl->l_pid, &target_fl->l_pid); > + unlock_user_struct(target_fl, target_flock_addr, 0); > + return 0; > +} > + > +static inline abi_long copy_to_user_eabi_flock64(abi_ulong target_flock_addr, > + const struct flock64 *fl) > +{ > + struct target_eabi_flock64 *target_fl; > + short l_type; > + > + if (!lock_user_struct(VERIFY_WRITE, target_fl, target_flock_addr, 0)) { > + return -TARGET_EFAULT; > + } > + > + l_type = host_to_target_bitmask(fl->l_type, flock_tbl); > + __put_user(l_type, &target_fl->l_type); > + __put_user(fl->l_whence, &target_fl->l_whence); > + __put_user(fl->l_start, &target_fl->l_start); > + __put_user(fl->l_len, &target_fl->l_len); > + __put_user(fl->l_pid, &target_fl->l_pid); > + unlock_user_struct(target_fl, target_flock_addr, 1); > + return 0; > +} > +#endif > + > +static inline abi_long copy_from_user_flock64(struct flock64 *fl, > + abi_ulong target_flock_addr) > +{ > + struct target_flock64 *target_fl; > + short l_type; > + > + if (!lock_user_struct(VERIFY_READ, target_fl, target_flock_addr, 1)) { > + return -TARGET_EFAULT; > + } > + > + __get_user(l_type, &target_fl->l_type); > + fl->l_type = target_to_host_bitmask(l_type, flock_tbl); > + __get_user(fl->l_whence, &target_fl->l_whence); > + __get_user(fl->l_start, &target_fl->l_start); > + __get_user(fl->l_len, &target_fl->l_len); > + __get_user(fl->l_pid, &target_fl->l_pid); > + unlock_user_struct(target_fl, target_flock_addr, 0); > + return 0; > +} > + > +static inline abi_long copy_to_user_flock64(abi_ulong target_flock_addr, > + const struct flock64 *fl) > +{ > + struct target_flock64 *target_fl; > + short l_type; > + > + if (!lock_user_struct(VERIFY_WRITE, target_fl, target_flock_addr, 0)) { > + return -TARGET_EFAULT; > + } > + > + l_type = host_to_target_bitmask(fl->l_type, flock_tbl); > + __put_user(l_type, &target_fl->l_type); > + __put_user(fl->l_whence, &target_fl->l_whence); > + __put_user(fl->l_start, &target_fl->l_start); > + __put_user(fl->l_len, &target_fl->l_len); > + __put_user(fl->l_pid, &target_fl->l_pid); > + unlock_user_struct(target_fl, target_flock_addr, 1); > + return 0; > +} > + > +static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > +{ > struct flock64 fl64; > - struct target_flock64 *target_fl64; > #ifdef F_GETOWN_EX > struct f_owner_ex fox; > struct target_f_owner_ex *target_fox; > @@ -4967,77 +5089,41 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) > > switch(cmd) { > case TARGET_F_GETLK: > - if (!lock_user_struct(VERIFY_READ, target_fl, arg, 1)) > + if (copy_from_user_flock(&fl64, arg)) { > return -TARGET_EFAULT; why do you ignore the exact value returned by copy_from_user_flock()? You should return this value instead of guessing it. > - fl.l_type = > - target_to_host_bitmask(tswap16(target_fl->l_type), flock_tbl); > - fl.l_whence = tswap16(target_fl->l_whence); > - fl.l_start = tswapal(target_fl->l_start); > - fl.l_len = tswapal(target_fl->l_len); > - fl.l_pid = tswap32(target_fl->l_pid); > - unlock_user_struct(target_fl, arg, 0); > - ret = get_errno(fcntl(fd, host_cmd, &fl)); > + } > + ret = get_errno(fcntl(fd, host_cmd, &fl64)); > if (ret == 0) { > - if (!lock_user_struct(VERIFY_WRITE, target_fl, arg, 0)) > + if (copy_to_user_flock(arg, &fl64)) { > return -TARGET_EFAULT; > - target_fl->l_type = > - host_to_target_bitmask(tswap16(fl.l_type), flock_tbl); > - target_fl->l_whence = tswap16(fl.l_whence); > - target_fl->l_start = tswapal(fl.l_start); > - target_fl->l_len = tswapal(fl.l_len); > - target_fl->l_pid = tswap32(fl.l_pid); > - unlock_user_struct(target_fl, arg, 1); > + } > } > break; > > case TARGET_F_SETLK: > case TARGET_F_SETLKW: > - if (!lock_user_struct(VERIFY_READ, target_fl, arg, 1)) > + if (copy_from_user_flock(&fl64, arg)) { > return -TARGET_EFAULT; same as above > - fl.l_type = > - target_to_host_bitmask(tswap16(target_fl->l_type), flock_tbl); > - fl.l_whence = tswap16(target_fl->l_whence); > - fl.l_start = tswapal(target_fl->l_start); > - fl.l_len = tswapal(target_fl->l_len); > - fl.l_pid = tswap32(target_fl->l_pid); > - unlock_user_struct(target_fl, arg, 0); > - ret = get_errno(fcntl(fd, host_cmd, &fl)); > + } > + ret = get_errno(fcntl(fd, host_cmd, &fl64)); > break; > > case TARGET_F_GETLK64: > - if (!lock_user_struct(VERIFY_READ, target_fl64, arg, 1)) > + if (copy_from_user_flock64(&fl64, arg)) { > return -TARGET_EFAULT; same as above > - fl64.l_type = > - target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1; The ">> 1" disappears... > - fl64.l_whence = tswap16(target_fl64->l_whence); > - fl64.l_start = tswap64(target_fl64->l_start); > - fl64.l_len = tswap64(target_fl64->l_len); > - fl64.l_pid = tswap32(target_fl64->l_pid); > - unlock_user_struct(target_fl64, arg, 0); > + } > ret = get_errno(fcntl(fd, host_cmd, &fl64)); > if (ret == 0) { > - if (!lock_user_struct(VERIFY_WRITE, target_fl64, arg, 0)) > + if (copy_to_user_flock64(arg, &fl64)) { > return -TARGET_EFAULT; same as above > - target_fl64->l_type = > - host_to_target_bitmask(tswap16(fl64.l_type), flock_tbl) >> 1; > - target_fl64->l_whence = tswap16(fl64.l_whence); > - target_fl64->l_start = tswap64(fl64.l_start); > - target_fl64->l_len = tswap64(fl64.l_len); > - target_fl64->l_pid = tswap32(fl64.l_pid); > - unlock_user_struct(target_fl64, arg, 1); > + } > } > break; > case TARGET_F_SETLK64: > case TARGET_F_SETLKW64: > - if (!lock_user_struct(VERIFY_READ, target_fl64, arg, 1)) > + if (copy_from_user_flock64(&fl64, arg)) { > return -TARGET_EFAULT; same as above > - fl64.l_type = > - target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1; > - fl64.l_whence = tswap16(target_fl64->l_whence); > - fl64.l_start = tswap64(target_fl64->l_start); > - fl64.l_len = tswap64(target_fl64->l_len); > - fl64.l_pid = tswap32(target_fl64->l_pid); > - unlock_user_struct(target_fl64, arg, 0); > + } > ret = get_errno(fcntl(fd, host_cmd, &fl64)); > break; > > @@ -9485,9 +9571,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > { > int cmd; > struct flock64 fl; > - struct target_flock64 *target_fl; > + from_flock64_fn *copyfrom = copy_from_user_flock64; > + to_flock64_fn *copyto = copy_to_user_flock64; > + > #ifdef TARGET_ARM > - struct target_eabi_flock64 *target_efl; > + if (((CPUARMState *)cpu_env)->eabi) { > + copyfrom = copy_from_user_eabi_flock64; > + copyto = copy_to_user_eabi_flock64; > + } > #endif > > cmd = target_to_host_fcntl_cmd(arg2); > @@ -9498,78 +9589,21 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, > > switch(arg2) { > case TARGET_F_GETLK64: > -#ifdef TARGET_ARM > - if (((CPUARMState *)cpu_env)->eabi) { > - if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) > - goto efault; > - fl.l_type = tswap16(target_efl->l_type); > - fl.l_whence = tswap16(target_efl->l_whence); > - fl.l_start = tswap64(target_efl->l_start); > - fl.l_len = tswap64(target_efl->l_len); > - fl.l_pid = tswap32(target_efl->l_pid); > - unlock_user_struct(target_efl, arg3, 0); > - } else > -#endif > - { > - if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) > - goto efault; > - fl.l_type = tswap16(target_fl->l_type); > - fl.l_whence = tswap16(target_fl->l_whence); > - fl.l_start = tswap64(target_fl->l_start); > - fl.l_len = tswap64(target_fl->l_len); > - fl.l_pid = tswap32(target_fl->l_pid); > - unlock_user_struct(target_fl, arg3, 0); > + if (copyfrom(&fl, arg3)) { > + goto efault; > } > ret = get_errno(fcntl(arg1, cmd, &fl)); > if (ret == 0) { > -#ifdef TARGET_ARM > - if (((CPUARMState *)cpu_env)->eabi) { > - if (!lock_user_struct(VERIFY_WRITE, target_efl, arg3, 0)) > - goto efault; > - target_efl->l_type = tswap16(fl.l_type); > - target_efl->l_whence = tswap16(fl.l_whence); > - target_efl->l_start = tswap64(fl.l_start); > - target_efl->l_len = tswap64(fl.l_len); > - target_efl->l_pid = tswap32(fl.l_pid); > - unlock_user_struct(target_efl, arg3, 1); > - } else > -#endif > - { > - if (!lock_user_struct(VERIFY_WRITE, target_fl, arg3, 0)) > - goto efault; > - target_fl->l_type = tswap16(fl.l_type); > - target_fl->l_whence = tswap16(fl.l_whence); > - target_fl->l_start = tswap64(fl.l_start); > - target_fl->l_len = tswap64(fl.l_len); > - target_fl->l_pid = tswap32(fl.l_pid); > - unlock_user_struct(target_fl, arg3, 1); > + if (copyto(arg3, &fl)) { > + goto efault; > } > } > break; > > case TARGET_F_SETLK64: > case TARGET_F_SETLKW64: > -#ifdef TARGET_ARM > - if (((CPUARMState *)cpu_env)->eabi) { > - if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) > - goto efault; > - fl.l_type = tswap16(target_efl->l_type); > - fl.l_whence = tswap16(target_efl->l_whence); > - fl.l_start = tswap64(target_efl->l_start); > - fl.l_len = tswap64(target_efl->l_len); > - fl.l_pid = tswap32(target_efl->l_pid); > - unlock_user_struct(target_efl, arg3, 0); > - } else > -#endif > - { > - if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) > - goto efault; > - fl.l_type = tswap16(target_fl->l_type); > - fl.l_whence = tswap16(target_fl->l_whence); > - fl.l_start = tswap64(target_fl->l_start); > - fl.l_len = tswap64(target_fl->l_len); > - fl.l_pid = tswap32(target_fl->l_pid); > - unlock_user_struct(target_fl, arg3, 0); > + if (copyfrom(&fl, arg3)) { > + goto efault; > } > ret = get_errno(fcntl(arg1, cmd, &fl)); > break; >
On 7 June 2016 at 21:41, Laurent Vivier <laurent@vivier.eu> wrote: > > Le 06/06/2016 à 20:58, Peter Maydell a écrit : >> Use the __get_user() and __put_user() to handle reading and writing the >> guest structures in do_ioctl(). This has two benefits: >> * avoids possible errors due to misaligned guest pointers >> * correctly sign extends signed fields (like l_start in struct flock) >> which might be different sizes between guest and host >> >> To do this we abstract out into copy_from/to_user functions. We >> also standardize on always using host flock64 and the F_GETLK64 >> etc flock commands, as this means we always have 64 bit offsets >> whether the host is 64-bit or 32-bit and we don't need to support >> conversion to both host struct flock and struct flock64. >> >> In passing we fix errors in converting l_type from the host to >> the target (where we were doing a byteswap of the host value >> before trying to do the convert-bitmasks operation rather than >> otherwise, and inexplicably shifting left by 1). > > I think the ">> 1" is coming from: > > 43f238d Support fcntl F_GETLK64, F_SETLK64, F_SETLKW64 > > to convert arm to x86, and should have been removed then in: > > 2ba7f73 alpha-linux-user: Translate fcntl l_type > > So yes, the ">> 1" is wrong. I don't understand how it can work. Thanks for tracking down where it came from. I suspect it just didn't work and nobody noticed, because: * there's not much use of big-on-little-endian * a lot of the time the bug is just going to downgrade an exclusive lock to a shared lock, and you won't notice if there isn't actually any contention on the lock... >> Signed-off-by: Peter Maydell <peter.maydell@linaro.org> >> --- >> linux-user/syscall.c | 280 +++++++++++++++++++++++++++++---------------------- >> 1 file changed, 157 insertions(+), 123 deletions(-) >> >> diff --git a/linux-user/syscall.c b/linux-user/syscall.c >> index 4cf67c8..f3a487e 100644 >> --- a/linux-user/syscall.c >> +++ b/linux-user/syscall.c >> @@ -4894,11 +4894,11 @@ static int target_to_host_fcntl_cmd(int cmd) >> case TARGET_F_SETFL: >> return cmd; >> case TARGET_F_GETLK: >> - return F_GETLK; >> - case TARGET_F_SETLK: >> - return F_SETLK; >> - case TARGET_F_SETLKW: >> - return F_SETLKW; >> + return F_GETLK64; >> + case TARGET_F_SETLK: >> + return F_SETLK64; >> + case TARGET_F_SETLKW: >> + return F_SETLKW64; >> case TARGET_F_GETOWN: >> return F_GETOWN; >> case TARGET_F_SETOWN: > > I see no reason to have this in this patch. The idea is that we want to use only one host flock struct, which means it must be the one which supports 64-bit offsets. On a 32-bit host, that's the flock64 struct, which must be used with the F_GETLK64 fcntl, not F_GETLK. On a 64-bit host, the system headers define that F_GETLK64 and F_GETLK are identical (and that the flock64 struct is flock), so instead of having to specialcase 64-bit hosts, we can just say "use the F_*64 constants and struct flock64 everywhere". If we didn't have this hunk of the patch then on a 32-bit host the code below would go wrong, because when we did a guest F_GETLK we'd end up passing a (host) struct flock64 to the 32-bit F_GETLK. >> case TARGET_F_GETLK: >> - if (!lock_user_struct(VERIFY_READ, target_fl, arg, 1)) >> + if (copy_from_user_flock(&fl64, arg)) { >> return -TARGET_EFAULT; > > why do you ignore the exact value returned by copy_from_user_flock()? > You should return this value instead of guessing it. Yeah, I was being lazy and not wanting to have an extra 'ret' variable floating around. I'll fix this. >> - fl64.l_type = >> - target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1; > > The ">> 1" disappears... ...and it's correct that it disappears, right? thanks -- PMM
Le 07/06/2016 à 23:20, Peter Maydell a écrit : > On 7 June 2016 at 21:41, Laurent Vivier <laurent@vivier.eu> wrote: >>> - fl64.l_type = >>> - target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1; >> >> The ">> 1" disappears... > > ...and it's correct that it disappears, right? Yes. I forgot to remove this when I have understood what was the reason of the shift. Thanks, Laurent
diff --git a/linux-user/syscall.c b/linux-user/syscall.c index 4cf67c8..f3a487e 100644 --- a/linux-user/syscall.c +++ b/linux-user/syscall.c @@ -4894,11 +4894,11 @@ static int target_to_host_fcntl_cmd(int cmd) case TARGET_F_SETFL: return cmd; case TARGET_F_GETLK: - return F_GETLK; - case TARGET_F_SETLK: - return F_SETLK; - case TARGET_F_SETLKW: - return F_SETLKW; + return F_GETLK64; + case TARGET_F_SETLK: + return F_SETLK64; + case TARGET_F_SETLKW: + return F_SETLKW64; case TARGET_F_GETOWN: return F_GETOWN; case TARGET_F_SETOWN: @@ -4949,12 +4949,134 @@ static const bitmask_transtbl flock_tbl[] = { { 0, 0, 0, 0 } }; -static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) +static inline abi_long copy_from_user_flock(struct flock64 *fl, + abi_ulong target_flock_addr) { - struct flock fl; struct target_flock *target_fl; + short l_type; + + if (!lock_user_struct(VERIFY_READ, target_fl, target_flock_addr, 1)) { + return -TARGET_EFAULT; + } + + __get_user(l_type, &target_fl->l_type); + fl->l_type = target_to_host_bitmask(l_type, flock_tbl); + __get_user(fl->l_whence, &target_fl->l_whence); + __get_user(fl->l_start, &target_fl->l_start); + __get_user(fl->l_len, &target_fl->l_len); + __get_user(fl->l_pid, &target_fl->l_pid); + unlock_user_struct(target_fl, target_flock_addr, 0); + return 0; +} + +static inline abi_long copy_to_user_flock(abi_ulong target_flock_addr, + const struct flock64 *fl) +{ + struct target_flock *target_fl; + short l_type; + + if (!lock_user_struct(VERIFY_WRITE, target_fl, target_flock_addr, 0)) { + return -TARGET_EFAULT; + } + + l_type = host_to_target_bitmask(fl->l_type, flock_tbl); + __put_user(l_type, &target_fl->l_type); + __put_user(fl->l_whence, &target_fl->l_whence); + __put_user(fl->l_start, &target_fl->l_start); + __put_user(fl->l_len, &target_fl->l_len); + __put_user(fl->l_pid, &target_fl->l_pid); + unlock_user_struct(target_fl, target_flock_addr, 1); + return 0; +} + +typedef abi_long from_flock64_fn(struct flock64 *fl, abi_ulong target_addr); +typedef abi_long to_flock64_fn(abi_ulong target_addr, const struct flock64 *fl); + +#ifdef TARGET_ARM +static inline abi_long copy_from_user_eabi_flock64(struct flock64 *fl, + abi_ulong target_flock_addr) +{ + struct target_eabi_flock64 *target_fl; + short l_type; + + if (!lock_user_struct(VERIFY_READ, target_fl, target_flock_addr, 1)) { + return -TARGET_EFAULT; + } + + __get_user(l_type, &target_fl->l_type); + fl->l_type = target_to_host_bitmask(l_type, flock_tbl); + __get_user(fl->l_whence, &target_fl->l_whence); + __get_user(fl->l_start, &target_fl->l_start); + __get_user(fl->l_len, &target_fl->l_len); + __get_user(fl->l_pid, &target_fl->l_pid); + unlock_user_struct(target_fl, target_flock_addr, 0); + return 0; +} + +static inline abi_long copy_to_user_eabi_flock64(abi_ulong target_flock_addr, + const struct flock64 *fl) +{ + struct target_eabi_flock64 *target_fl; + short l_type; + + if (!lock_user_struct(VERIFY_WRITE, target_fl, target_flock_addr, 0)) { + return -TARGET_EFAULT; + } + + l_type = host_to_target_bitmask(fl->l_type, flock_tbl); + __put_user(l_type, &target_fl->l_type); + __put_user(fl->l_whence, &target_fl->l_whence); + __put_user(fl->l_start, &target_fl->l_start); + __put_user(fl->l_len, &target_fl->l_len); + __put_user(fl->l_pid, &target_fl->l_pid); + unlock_user_struct(target_fl, target_flock_addr, 1); + return 0; +} +#endif + +static inline abi_long copy_from_user_flock64(struct flock64 *fl, + abi_ulong target_flock_addr) +{ + struct target_flock64 *target_fl; + short l_type; + + if (!lock_user_struct(VERIFY_READ, target_fl, target_flock_addr, 1)) { + return -TARGET_EFAULT; + } + + __get_user(l_type, &target_fl->l_type); + fl->l_type = target_to_host_bitmask(l_type, flock_tbl); + __get_user(fl->l_whence, &target_fl->l_whence); + __get_user(fl->l_start, &target_fl->l_start); + __get_user(fl->l_len, &target_fl->l_len); + __get_user(fl->l_pid, &target_fl->l_pid); + unlock_user_struct(target_fl, target_flock_addr, 0); + return 0; +} + +static inline abi_long copy_to_user_flock64(abi_ulong target_flock_addr, + const struct flock64 *fl) +{ + struct target_flock64 *target_fl; + short l_type; + + if (!lock_user_struct(VERIFY_WRITE, target_fl, target_flock_addr, 0)) { + return -TARGET_EFAULT; + } + + l_type = host_to_target_bitmask(fl->l_type, flock_tbl); + __put_user(l_type, &target_fl->l_type); + __put_user(fl->l_whence, &target_fl->l_whence); + __put_user(fl->l_start, &target_fl->l_start); + __put_user(fl->l_len, &target_fl->l_len); + __put_user(fl->l_pid, &target_fl->l_pid); + unlock_user_struct(target_fl, target_flock_addr, 1); + return 0; +} + +static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) +{ struct flock64 fl64; - struct target_flock64 *target_fl64; #ifdef F_GETOWN_EX struct f_owner_ex fox; struct target_f_owner_ex *target_fox; @@ -4967,77 +5089,41 @@ static abi_long do_fcntl(int fd, int cmd, abi_ulong arg) switch(cmd) { case TARGET_F_GETLK: - if (!lock_user_struct(VERIFY_READ, target_fl, arg, 1)) + if (copy_from_user_flock(&fl64, arg)) { return -TARGET_EFAULT; - fl.l_type = - target_to_host_bitmask(tswap16(target_fl->l_type), flock_tbl); - fl.l_whence = tswap16(target_fl->l_whence); - fl.l_start = tswapal(target_fl->l_start); - fl.l_len = tswapal(target_fl->l_len); - fl.l_pid = tswap32(target_fl->l_pid); - unlock_user_struct(target_fl, arg, 0); - ret = get_errno(fcntl(fd, host_cmd, &fl)); + } + ret = get_errno(fcntl(fd, host_cmd, &fl64)); if (ret == 0) { - if (!lock_user_struct(VERIFY_WRITE, target_fl, arg, 0)) + if (copy_to_user_flock(arg, &fl64)) { return -TARGET_EFAULT; - target_fl->l_type = - host_to_target_bitmask(tswap16(fl.l_type), flock_tbl); - target_fl->l_whence = tswap16(fl.l_whence); - target_fl->l_start = tswapal(fl.l_start); - target_fl->l_len = tswapal(fl.l_len); - target_fl->l_pid = tswap32(fl.l_pid); - unlock_user_struct(target_fl, arg, 1); + } } break; case TARGET_F_SETLK: case TARGET_F_SETLKW: - if (!lock_user_struct(VERIFY_READ, target_fl, arg, 1)) + if (copy_from_user_flock(&fl64, arg)) { return -TARGET_EFAULT; - fl.l_type = - target_to_host_bitmask(tswap16(target_fl->l_type), flock_tbl); - fl.l_whence = tswap16(target_fl->l_whence); - fl.l_start = tswapal(target_fl->l_start); - fl.l_len = tswapal(target_fl->l_len); - fl.l_pid = tswap32(target_fl->l_pid); - unlock_user_struct(target_fl, arg, 0); - ret = get_errno(fcntl(fd, host_cmd, &fl)); + } + ret = get_errno(fcntl(fd, host_cmd, &fl64)); break; case TARGET_F_GETLK64: - if (!lock_user_struct(VERIFY_READ, target_fl64, arg, 1)) + if (copy_from_user_flock64(&fl64, arg)) { return -TARGET_EFAULT; - fl64.l_type = - target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1; - fl64.l_whence = tswap16(target_fl64->l_whence); - fl64.l_start = tswap64(target_fl64->l_start); - fl64.l_len = tswap64(target_fl64->l_len); - fl64.l_pid = tswap32(target_fl64->l_pid); - unlock_user_struct(target_fl64, arg, 0); + } ret = get_errno(fcntl(fd, host_cmd, &fl64)); if (ret == 0) { - if (!lock_user_struct(VERIFY_WRITE, target_fl64, arg, 0)) + if (copy_to_user_flock64(arg, &fl64)) { return -TARGET_EFAULT; - target_fl64->l_type = - host_to_target_bitmask(tswap16(fl64.l_type), flock_tbl) >> 1; - target_fl64->l_whence = tswap16(fl64.l_whence); - target_fl64->l_start = tswap64(fl64.l_start); - target_fl64->l_len = tswap64(fl64.l_len); - target_fl64->l_pid = tswap32(fl64.l_pid); - unlock_user_struct(target_fl64, arg, 1); + } } break; case TARGET_F_SETLK64: case TARGET_F_SETLKW64: - if (!lock_user_struct(VERIFY_READ, target_fl64, arg, 1)) + if (copy_from_user_flock64(&fl64, arg)) { return -TARGET_EFAULT; - fl64.l_type = - target_to_host_bitmask(tswap16(target_fl64->l_type), flock_tbl) >> 1; - fl64.l_whence = tswap16(target_fl64->l_whence); - fl64.l_start = tswap64(target_fl64->l_start); - fl64.l_len = tswap64(target_fl64->l_len); - fl64.l_pid = tswap32(target_fl64->l_pid); - unlock_user_struct(target_fl64, arg, 0); + } ret = get_errno(fcntl(fd, host_cmd, &fl64)); break; @@ -9485,9 +9571,14 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, { int cmd; struct flock64 fl; - struct target_flock64 *target_fl; + from_flock64_fn *copyfrom = copy_from_user_flock64; + to_flock64_fn *copyto = copy_to_user_flock64; + #ifdef TARGET_ARM - struct target_eabi_flock64 *target_efl; + if (((CPUARMState *)cpu_env)->eabi) { + copyfrom = copy_from_user_eabi_flock64; + copyto = copy_to_user_eabi_flock64; + } #endif cmd = target_to_host_fcntl_cmd(arg2); @@ -9498,78 +9589,21 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1, switch(arg2) { case TARGET_F_GETLK64: -#ifdef TARGET_ARM - if (((CPUARMState *)cpu_env)->eabi) { - if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) - goto efault; - fl.l_type = tswap16(target_efl->l_type); - fl.l_whence = tswap16(target_efl->l_whence); - fl.l_start = tswap64(target_efl->l_start); - fl.l_len = tswap64(target_efl->l_len); - fl.l_pid = tswap32(target_efl->l_pid); - unlock_user_struct(target_efl, arg3, 0); - } else -#endif - { - if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) - goto efault; - fl.l_type = tswap16(target_fl->l_type); - fl.l_whence = tswap16(target_fl->l_whence); - fl.l_start = tswap64(target_fl->l_start); - fl.l_len = tswap64(target_fl->l_len); - fl.l_pid = tswap32(target_fl->l_pid); - unlock_user_struct(target_fl, arg3, 0); + if (copyfrom(&fl, arg3)) { + goto efault; } ret = get_errno(fcntl(arg1, cmd, &fl)); if (ret == 0) { -#ifdef TARGET_ARM - if (((CPUARMState *)cpu_env)->eabi) { - if (!lock_user_struct(VERIFY_WRITE, target_efl, arg3, 0)) - goto efault; - target_efl->l_type = tswap16(fl.l_type); - target_efl->l_whence = tswap16(fl.l_whence); - target_efl->l_start = tswap64(fl.l_start); - target_efl->l_len = tswap64(fl.l_len); - target_efl->l_pid = tswap32(fl.l_pid); - unlock_user_struct(target_efl, arg3, 1); - } else -#endif - { - if (!lock_user_struct(VERIFY_WRITE, target_fl, arg3, 0)) - goto efault; - target_fl->l_type = tswap16(fl.l_type); - target_fl->l_whence = tswap16(fl.l_whence); - target_fl->l_start = tswap64(fl.l_start); - target_fl->l_len = tswap64(fl.l_len); - target_fl->l_pid = tswap32(fl.l_pid); - unlock_user_struct(target_fl, arg3, 1); + if (copyto(arg3, &fl)) { + goto efault; } } break; case TARGET_F_SETLK64: case TARGET_F_SETLKW64: -#ifdef TARGET_ARM - if (((CPUARMState *)cpu_env)->eabi) { - if (!lock_user_struct(VERIFY_READ, target_efl, arg3, 1)) - goto efault; - fl.l_type = tswap16(target_efl->l_type); - fl.l_whence = tswap16(target_efl->l_whence); - fl.l_start = tswap64(target_efl->l_start); - fl.l_len = tswap64(target_efl->l_len); - fl.l_pid = tswap32(target_efl->l_pid); - unlock_user_struct(target_efl, arg3, 0); - } else -#endif - { - if (!lock_user_struct(VERIFY_READ, target_fl, arg3, 1)) - goto efault; - fl.l_type = tswap16(target_fl->l_type); - fl.l_whence = tswap16(target_fl->l_whence); - fl.l_start = tswap64(target_fl->l_start); - fl.l_len = tswap64(target_fl->l_len); - fl.l_pid = tswap32(target_fl->l_pid); - unlock_user_struct(target_fl, arg3, 0); + if (copyfrom(&fl, arg3)) { + goto efault; } ret = get_errno(fcntl(arg1, cmd, &fl)); break;
Use the __get_user() and __put_user() to handle reading and writing the guest structures in do_ioctl(). This has two benefits: * avoids possible errors due to misaligned guest pointers * correctly sign extends signed fields (like l_start in struct flock) which might be different sizes between guest and host To do this we abstract out into copy_from/to_user functions. We also standardize on always using host flock64 and the F_GETLK64 etc flock commands, as this means we always have 64 bit offsets whether the host is 64-bit or 32-bit and we don't need to support conversion to both host struct flock and struct flock64. In passing we fix errors in converting l_type from the host to the target (where we were doing a byteswap of the host value before trying to do the convert-bitmasks operation rather than otherwise, and inexplicably shifting left by 1). Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- linux-user/syscall.c | 280 +++++++++++++++++++++++++++++---------------------- 1 file changed, 157 insertions(+), 123 deletions(-)