[Qemu-devel] [PATCH] linux-user: ioctl() command type is int

[Qemu-devel] [PATCH] linux-user: ioctl() command type is int

[Laurent Vivier]

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 abi_int in the case of the target command type
instead of abi_long (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 linux-user/syscall.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..68801cf 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3293,8 +3293,8 @@ typedef abi_long do_ioctl_fn(const IOCTLEntry *ie, uint8_t *buf_temp,
                              int fd, abi_long cmd, abi_long arg);
 
 struct IOCTLEntry {
-    int target_cmd;
-    unsigned int host_cmd;
+    abi_int target_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3849,7 +3849,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, abi_int cmd, abi_long arg)
 {
     const IOCTLEntry *ie;
     const argtype *arg_type;
-- 
2.1.4

Re: [Qemu-devel] [PATCH] linux-user: ioctl() command type is int

[Peter Maydell]

On 21 May 2015 at 23:18, 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 abi_int in the case of the target command type
> instead of abi_long (and for consistency, update IOCTLEntry).
>

abi_int is really only needed in guest-layout structure
declarations, since it has alignment attributes but is
otherwise guaranteed to be an int32_t (and we assume all
over the place that int is 32 bits, so int is ok too).
So it's unnecessary in the function prototypes, and
I think actively harmful in the IOCTLEntry struct (since
that struct is not a shared-with-guest one).

I think also the do_ioctl_fn() prototype and all the
do_ioctl_* functions which are of that type need to have
the cmd parameter switched from abi_long. This avoids
potentially running into the same problem inside a
do_ioctl function if it does comparisons on the cmd.

thanks
-- PMM

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

[Laurent Vivier]

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 (and for consistency, update IOCTLEntry).

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
v2: use int instead of abi_int, as it is recommended by Peter Maydell.

 linux-user/syscall.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 1622ad6..c28cd05 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -3290,11 +3290,11 @@ 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;
-    unsigned int host_cmd;
+    int host_cmd;
     const char *name;
     int access;
     do_ioctl_fn *do_ioctl;
@@ -3316,7 +3316,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
@@ -3397,7 +3397,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;
@@ -3491,7 +3491,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;
@@ -3716,7 +3716,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;
@@ -3769,7 +3769,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;
@@ -3832,7 +3832,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));
@@ -3849,7 +3849,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.1.4

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Peter Maydell]

On 23 May 2015 at 14:17, 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 (and for consistency, update IOCTLEntry).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Reviewed-by: Peter Maydell <peter.maydell@linaro.org>

thanks
-- PMM

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Riku Voipio]

On Saturday, May 23, 2015 4:17:05 PM EEST, Laurent Vivier 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 (and for consistency, update IOCTLEntry).
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>

Applied to linux-user que,

Thanks

> ---
> v2: use int instead of abi_int, as it is recommended by Peter Maydell.
>
>  linux-user/syscall.c | 18 +++++++++---------
>  1 file changed, 9 insertions(+), 9 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 1622ad6..c28cd05 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -3290,11 +3290,11 @@ 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;
> -    unsigned int host_cmd;
> +    int host_cmd;
>      const char *name;
>      int access;
>      do_ioctl_fn *do_ioctl;
> @@ -3316,7 +3316,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
> @@ -3397,7 +3397,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;
> @@ -3491,7 +3491,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;
> @@ -3716,7 +3716,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;
> @@ -3769,7 +3769,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;
> @@ -3832,7 +3832,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));
> @@ -3849,7 +3849,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;

Re: [Qemu-devel] [PULL 0/6] linux-user patches for 2.4 softfreeze

[Peter Maydell]

On 15 June 2015 at 13:20,  <riku.voipio@linaro.org> wrote:
> From: Riku Voipio <riku.voipio@linaro.org>
>
> The following changes since commit 0a2df857a7038c75379cc575de5d4be4c0ac629e:
>
>   Merge remote-tracking branch 'remotes/stefanha/tags/net-pull-request' into staging (2015-06-12 15:39:05 +0100)
>
> are available in the git repository at:
>
>   git://git.linaro.org/people/riku.voipio/qemu.git tags/pull-linux-user-20150615
>
> for you to fetch changes up to d2897da1f1e97d684f80ff62d473c31b79bc643a:
>
>   linux-user: fix the breakpoint inheritance in spawned threads (2015-06-15 11:36:59 +0300)
>
> ----------------------------------------------------------------
> linux-user patches for 2.4 softfreeze
>
> ----------------------------------------------------------------

I get a lot of build errors with clang:

/home/petmay01/linaro/qemu-for-merges/linux-user/syscall.c:3610:10:
error: overflow
      converting case value to switch condition type (3241737481 to
18446744072656321801)
      [-Werror,-Wswitch]
    case DM_TABLE_LOAD:
         ^
/usr/include/linux/dm-ioctl.h:259:26: note: expanded from macro 'DM_TABLE_LOAD'
#define DM_TABLE_LOAD    _IOWR(DM_IOCTL, DM_TABLE_LOAD_CMD, struct dm_ioctl)
                         ^
/usr/include/asm-generic/ioctl.h:77:29: note: expanded from macro '_IOWR'
#define _IOWR(type,nr,size)
_IOC(_IOC_READ|_IOC_WRITE,(type),(nr),(_IOC_TYPECHE...
                                ^
/usr/include/asm-generic/ioctl.h:66:2: note: expanded from macro '_IOC'
        (((dir)  << _IOC_DIRSHIFT) | \
        ^
/home/petmay01/linaro/qemu-for-merges/linux-user/syscall.c:3606:10:
error: overflow
      converting case value to switch condition type (3241737486 to
18446744072656321806)
      [-Werror,-Wswitch]
    case DM_TARGET_MSG:
         ^
(etc etc for all the cases until clang gives up because it's emitted
too many errors).

Guessing this is the result of the ioctl patch?

thanks
-- PMM

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

[Laurent Vivier]

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(-)

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

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Eric Blake]

[-- Attachment #1: Type: text/plain, Size: 1295 bytes --]

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.

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 604 bytes --]

Re: [Qemu-devel] [PATCH v2] linux-user: ioctl() command type is int

[Riku Voipio]

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
>