[v2] linux-user: ioctl() command type is int

When executing a 64bit target chroot on 64bit host,
the ioctl() command can mismatch.

It seems the previous commit doesn't solve the problem in
my case:

	9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets

For example, a ppc64 chroot on an x86_64 host:

bash-4.3# ls
Unsupported ioctl: cmd=0x80087467
Unsupported ioctl: cmd=0x802c7415

The origin of the problem is in syscall.c:do_ioctl().

	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

In this case (ppc64) abi_long is long (on the x86_64), and

    cmd = 0x0000000080087467

then
	if (ie->target_cmd == cmd)

target_cmd is int, so target_cmd = 0x80087467
and to compare an int with a long, the sign is extended to 64bit,
so the comparison is:

	if (0xffffffff80087467 == 0x0000000080087467)

which doesn't match whereas it should.

This patch uses int in the case of the target command type
instead of abi_long.

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: Don't modify IOCTLEntry type (useless and introduce clang errors)
 linux-user/syscall.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

On 06/15/2015 04:35 PM, Laurent Vivier wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
> 

> 
> The origin of the problem is in syscall.c:do_ioctl().
> 
> 	static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)

It's annoying that Linux picked ioctl(int, unsigned long request, ...),
particularly since POSIX picked ioctl(int, int request, ...) [1] and
therefore Linux is constrained to never accept a 'request' that doesn't
fit in 32 bits.  Especially so since the POSIX definition of ioctl()
applies only to the obsolete STREAMS interface that Linux never really
picked up on. (The gnulib project has determined ways to write an
ioctl() wrapper that always takes an int request, then widens to long as
necessary before calling the real syscall, with no ill effects [2])

[1] http://pubs.opengroup.org/onlinepubs/9699919799/functions/ioctl.html
[2] http://git.savannah.gnu.org/cgit/gnulib.git/tree/lib/ioctl.c

However, I don't feel comfortable enough with this code to give a
competent review, only to offer up that bit of trivia and the vague
impression that it looks like you are safe in this patch.

On 16 June 2015 at 01:35, Laurent Vivier <laurent@vivier.eu> wrote:
> When executing a 64bit target chroot on 64bit host,
> the ioctl() command can mismatch.
>
> It seems the previous commit doesn't solve the problem in
> my case:
>
>         9c6bf9c7 linux-user: Fix ioctl cmd type mismatch on 64-bit targets
>
> For example, a ppc64 chroot on an x86_64 host:
>
> bash-4.3# ls
> Unsupported ioctl: cmd=0x80087467
> Unsupported ioctl: cmd=0x802c7415
>
> The origin of the problem is in syscall.c:do_ioctl().
>
>         static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
>
> In this case (ppc64) abi_long is long (on the x86_64), and
>
>     cmd = 0x0000000080087467
>
> then
>         if (ie->target_cmd == cmd)
>
> target_cmd is int, so target_cmd = 0x80087467
> and to compare an int with a long, the sign is extended to 64bit,
> so the comparison is:
>
>         if (0xffffffff80087467 == 0x0000000080087467)
>
> which doesn't match whereas it should.
>
> This patch uses int in the case of the target command type
> instead of abi_long.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> v2: Don't modify IOCTLEntry type (useless and introduce clang errors)

Thanks, tested to build with clang, will refresh pull request in a moment.

>  linux-user/syscall.c | 16 ++++++++--------
>  1 file changed, 8 insertions(+), 8 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index b98b7e7..5a280c3 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3645,7 +3645,7 @@ enum {
>  typedef struct IOCTLEntry IOCTLEntry;
>
>  typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                             int fd, abi_long cmd, abi_long arg);
> +                             int fd, int cmd, abi_long arg);
>
>  struct IOCTLEntry {
>      int target_cmd;
> @@ -3671,7 +3671,7 @@ struct IOCTLEntry {
>                              / sizeof(struct fiemap_extent))
>
>  static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                       int fd, abi_long cmd, abi_long arg)
> +                                       int fd, int cmd, abi_long arg)
>  {
>      /* The parameter for this ioctl is a struct fiemap followed
>       * by an array of struct fiemap_extent whose size is set
> @@ -3752,7 +3752,7 @@ static abi_long do_ioctl_fs_ioc_fiemap(const IOCTLEntry *ie, uint8_t *buf_temp,
>  #endif
>
>  static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      int target_size;
> @@ -3846,7 +3846,7 @@ static abi_long do_ioctl_ifconf(const IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>
>  static abi_long do_ioctl_dm(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
> -                            abi_long cmd, abi_long arg)
> +                            int cmd, abi_long arg)
>  {
>      void *argptr;
>      struct dm_ioctl *host_dm;
> @@ -4071,7 +4071,7 @@ out:
>  }
>
>  static abi_long do_ioctl_blkpg(const IOCTLEntry *ie, uint8_t *buf_temp, int fd,
> -                               abi_long cmd, abi_long arg)
> +                               int cmd, abi_long arg)
>  {
>      void *argptr;
>      int target_size;
> @@ -4124,7 +4124,7 @@ out:
>  }
>
>  static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                int fd, abi_long cmd, abi_long arg)
> +                                int fd, int cmd, abi_long arg)
>  {
>      const argtype *arg_type = ie->arg_type;
>      const StructEntry *se;
> @@ -4187,7 +4187,7 @@ static abi_long do_ioctl_rt(const IOCTLEntry *ie, uint8_t *buf_temp,
>  }
>
>  static abi_long do_ioctl_kdsigaccept(const IOCTLEntry *ie, uint8_t *buf_temp,
> -                                     int fd, abi_long cmd, abi_long arg)
> +                                     int fd, int cmd, abi_long arg)
>  {
>      int sig = target_to_host_signal(arg);
>      return get_errno(ioctl(fd, ie->host_cmd, sig));
> @@ -4204,7 +4204,7 @@ static IOCTLEntry ioctl_entries[] = {
>
>  /* ??? Implement proper locking for ioctls.  */
>  /* do_ioctl() Must return target values and target errnos. */
> -static abi_long do_ioctl(int fd, abi_long cmd, abi_long arg)
> +static abi_long do_ioctl(int fd, int cmd, abi_long arg)
>  {
>      const IOCTLEntry *ie;
>      const argtype *arg_type;
> --
> 2.4.3
>