Patchwork [v2] linux-user: correct semctl() and shmctl()

login
register
mail settings
Submitter Laurent Vivier
Date Jan. 2, 2013, 8:38 p.m.
Message ID <1357159110-13853-1-git-send-email-laurent@vivier.eu>
Download mbox | patch
Permalink /patch/209145/
State New
Headers show

Comments

Laurent Vivier - Jan. 2, 2013, 8:38 p.m.
The parameter "union semun" of semctl() is not a value
but a pointer to the value.

Moreover, all fields of target_su must be swapped (if needed).

The third argument of shmctl is a pointer.

WITHOUT this patch:

$ ipcs

kernel not configured for shared memory

qemu: uncaught target signal 11 (Segmentation fault) - core dumped

WITH this patch:

$ ipcs

------ Shared Memory Segments --------
key        shmid      owner      perms      bytes      nattch     status
0x4e545030 0          root      600        96         1
0x4e545031 32769      root      600        96         1
0x4e545032 65538      root      666        96         1
0x4e545033 98307      root      666        96         1
0x47505344 131076     root      666        8240       1
0x3c81b7f5 163845     laurent   666        4096       0
0x00000000 729513990  laurent   600        393216     2          dest
0x00000000 729546759  laurent   600        393216     2          dest
0x00000000 1879179273 laurent   600        393216     2          dest

------ Semaphore Arrays --------
key        semid      owner      perms      nsems
0x3c81b7f6 32768      laurent   666        1
0x1c44ac47 6586369    laurent   600        1

------ Message Queues --------
key        msqid      owner      perms      used-bytes   messages
0x1c44ac45 458752     laurent    600        0            0
0x1c44ac46 491521     laurent    600        0            0

Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
[v2] move lock_user_struct() in do_semctl()

 linux-user/syscall.c |   39 ++++++++++++++++++++-------------------
 1 file changed, 20 insertions(+), 19 deletions(-)
Laurent Vivier - Jan. 19, 2013, 11:28 p.m.
ping ?

Le mercredi 02 janvier 2013 à 21:38 +0100, Laurent Vivier a écrit :
> The parameter "union semun" of semctl() is not a value
> but a pointer to the value.
> 
> Moreover, all fields of target_su must be swapped (if needed).
> 
> The third argument of shmctl is a pointer.
> 
> WITHOUT this patch:
> 
> $ ipcs
> 
> kernel not configured for shared memory
> 
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
> 
> WITH this patch:
> 
> $ ipcs
> 
> ------ Shared Memory Segments --------
> key        shmid      owner      perms      bytes      nattch     status
> 0x4e545030 0          root      600        96         1
> 0x4e545031 32769      root      600        96         1
> 0x4e545032 65538      root      666        96         1
> 0x4e545033 98307      root      666        96         1
> 0x47505344 131076     root      666        8240       1
> 0x3c81b7f5 163845     laurent   666        4096       0
> 0x00000000 729513990  laurent   600        393216     2          dest
> 0x00000000 729546759  laurent   600        393216     2          dest
> 0x00000000 1879179273 laurent   600        393216     2          dest
> 
> ------ Semaphore Arrays --------
> key        semid      owner      perms      nsems
> 0x3c81b7f6 32768      laurent   666        1
> 0x1c44ac47 6586369    laurent   600        1
> 
> ------ Message Queues --------
> key        msqid      owner      perms      used-bytes   messages
> 0x1c44ac45 458752     laurent    600        0            0
> 0x1c44ac46 491521     laurent    600        0            0
> 
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> [v2] move lock_user_struct() in do_semctl()
> 
>  linux-user/syscall.c |   39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e99adab..b2687e1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2637,8 +2637,9 @@ static inline abi_long host_to_target_semarray(int semid, abi_ulong target_addr,
>  }
>  
>  static inline abi_long do_semctl(int semid, int semnum, int cmd,
> -                                 union target_semun target_su)
> +                                 abi_ulong ptr)
>  {
> +    union target_semun *target_su;
>      union semun arg;
>      struct semid_ds dsarg;
>      unsigned short *array = NULL;
> @@ -2647,43 +2648,42 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
>      abi_long err;
>      cmd &= 0xff;
>  
> +    if (!lock_user_struct(VERIFY_READ, target_su, ptr, 1)) {
> +        return -TARGET_EFAULT;
> +    }
>      switch( cmd ) {
>  	case GETVAL:
>  	case SETVAL:
> -            arg.val = tswap32(target_su.val);
> +            arg.val = tswap32(target_su->val);
>              ret = get_errno(semctl(semid, semnum, cmd, arg));
> -            target_su.val = tswap32(arg.val);
> +            target_su->val = tswap32(arg.val);
>              break;
>  	case GETALL:
>  	case SETALL:
> -            err = target_to_host_semarray(semid, &array, target_su.array);
> +            err = target_to_host_semarray(semid, &array,
> +                                          tswapal(target_su->array));
>              if (err)
> -                return err;
> +                break;
>              arg.array = array;
>              ret = get_errno(semctl(semid, semnum, cmd, arg));
> -            err = host_to_target_semarray(semid, target_su.array, &array);
> -            if (err)
> -                return err;
> +            err = host_to_target_semarray(semid, tswapal(target_su->array),
> +                                          &array);
>              break;
>  	case IPC_STAT:
>  	case IPC_SET:
>  	case SEM_STAT:
> -            err = target_to_host_semid_ds(&dsarg, target_su.buf);
> +            err = target_to_host_semid_ds(&dsarg, tswapal(target_su->buf));
>              if (err)
> -                return err;
> +                break;
>              arg.buf = &dsarg;
>              ret = get_errno(semctl(semid, semnum, cmd, arg));
> -            err = host_to_target_semid_ds(target_su.buf, &dsarg);
> -            if (err)
> -                return err;
> +            err = host_to_target_semid_ds(tswapal(target_su->buf), &dsarg);
>              break;
>  	case IPC_INFO:
>  	case SEM_INFO:
>              arg.__buf = &seminfo;
>              ret = get_errno(semctl(semid, semnum, cmd, arg));
> -            err = host_to_target_seminfo(target_su.__buf, &seminfo);
> -            if (err)
> -                return err;
> +            err = host_to_target_seminfo(tswapal(target_su->__buf), &seminfo);
>              break;
>  	case IPC_RMID:
>  	case GETPID:
> @@ -2692,6 +2692,7 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
>              ret = get_errno(semctl(semid, semnum, cmd, NULL));
>              break;
>      }
> +    unlock_user_struct(target_su, ptr, 0);
>  
>      return ret;
>  }
> @@ -3162,7 +3163,7 @@ static abi_long do_ipc(unsigned int call, int first,
>          break;
>  
>      case IPCOP_semctl:
> -        ret = do_semctl(first, second, third, (union target_semun)(abi_ulong) ptr);
> +        ret = do_semctl(first, second, third, ptr);
>          break;
>  
>      case IPCOP_msgget:
> @@ -3229,7 +3230,7 @@ static abi_long do_ipc(unsigned int call, int first,
>  
>  	/* IPC_* and SHM_* command values are the same on all linux platforms */
>      case IPCOP_shmctl:
> -        ret = do_shmctl(first, second, third);
> +        ret = do_shmctl(first, second, ptr);
>          break;
>      default:
>  	gemu_log("Unsupported ipc call: %d (version %d)\n", call, version);
> @@ -6891,7 +6892,7 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
>  #endif
>  #ifdef TARGET_NR_semctl
>      case TARGET_NR_semctl:
> -        ret = do_semctl(arg1, arg2, arg3, (union target_semun)(abi_ulong)arg4);
> +        ret = do_semctl(arg1, arg2, arg3, arg4);
>          break;
>  #endif
>  #ifdef TARGET_NR_msgctl
Peter Maydell - Jan. 20, 2013, 11:24 a.m.
On 2 January 2013 20:38, Laurent Vivier <laurent@vivier.eu> wrote:
> The parameter "union semun" of semctl() is not a value
> but a pointer to the value.
>
> Moreover, all fields of target_su must be swapped (if needed).
>
> The third argument of shmctl is a pointer.
>
> WITHOUT this patch:
>
> $ ipcs
>
> kernel not configured for shared memory
>
> qemu: uncaught target signal 11 (Segmentation fault) - core dumped
>
> WITH this patch:
>
> $ ipcs
>
> ------ Shared Memory Segments --------
> key        shmid      owner      perms      bytes      nattch     status
> 0x4e545030 0          root      600        96         1
> 0x4e545031 32769      root      600        96         1
> 0x4e545032 65538      root      666        96         1
> 0x4e545033 98307      root      666        96         1
> 0x47505344 131076     root      666        8240       1
> 0x3c81b7f5 163845     laurent   666        4096       0
> 0x00000000 729513990  laurent   600        393216     2          dest
> 0x00000000 729546759  laurent   600        393216     2          dest
> 0x00000000 1879179273 laurent   600        393216     2          dest
>
> ------ Semaphore Arrays --------
> key        semid      owner      perms      nsems
> 0x3c81b7f6 32768      laurent   666        1
> 0x1c44ac47 6586369    laurent   600        1
>
> ------ Message Queues --------
> key        msqid      owner      perms      used-bytes   messages
> 0x1c44ac45 458752     laurent    600        0            0
> 0x1c44ac46 491521     laurent    600        0            0
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> [v2] move lock_user_struct() in do_semctl()
>
>  linux-user/syscall.c |   39 ++++++++++++++++++++-------------------
>  1 file changed, 20 insertions(+), 19 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index e99adab..b2687e1 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2637,8 +2637,9 @@ static inline abi_long host_to_target_semarray(int semid, abi_ulong target_addr,
>  }
>
>  static inline abi_long do_semctl(int semid, int semnum, int cmd,
> -                                 union target_semun target_su)
> +                                 abi_ulong ptr)
>  {
> +    union target_semun *target_su;
>      union semun arg;
>      struct semid_ds dsarg;
>      unsigned short *array = NULL;
> @@ -2647,43 +2648,42 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
>      abi_long err;
>      cmd &= 0xff;
>
> +    if (!lock_user_struct(VERIFY_READ, target_su, ptr, 1)) {
> +        return -TARGET_EFAULT;
> +    }
>      switch( cmd ) {
>         case GETVAL:
>         case SETVAL:
> -            arg.val = tswap32(target_su.val);
> +            arg.val = tswap32(target_su->val);
>              ret = get_errno(semctl(semid, semnum, cmd, arg));
> -            target_su.val = tswap32(arg.val);
> +            target_su->val = tswap32(arg.val);
>              break;
>         case GETALL:
>         case SETALL:
> -            err = target_to_host_semarray(semid, &array, target_su.array);
> +            err = target_to_host_semarray(semid, &array,
> +                                          tswapal(target_su->array));
>              if (err)
> -                return err;
> +                break;

(1) Coding style demands braces
(2) More importantly, this is going to break the return value -- instead
of returning 'err' we will break out of the switch and then return 'ret'.
There are similar issues in other cases.

-- PMM

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index e99adab..b2687e1 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2637,8 +2637,9 @@  static inline abi_long host_to_target_semarray(int semid, abi_ulong target_addr,
 }
 
 static inline abi_long do_semctl(int semid, int semnum, int cmd,
-                                 union target_semun target_su)
+                                 abi_ulong ptr)
 {
+    union target_semun *target_su;
     union semun arg;
     struct semid_ds dsarg;
     unsigned short *array = NULL;
@@ -2647,43 +2648,42 @@  static inline abi_long do_semctl(int semid, int semnum, int cmd,
     abi_long err;
     cmd &= 0xff;
 
+    if (!lock_user_struct(VERIFY_READ, target_su, ptr, 1)) {
+        return -TARGET_EFAULT;
+    }
     switch( cmd ) {
 	case GETVAL:
 	case SETVAL:
-            arg.val = tswap32(target_su.val);
+            arg.val = tswap32(target_su->val);
             ret = get_errno(semctl(semid, semnum, cmd, arg));
-            target_su.val = tswap32(arg.val);
+            target_su->val = tswap32(arg.val);
             break;
 	case GETALL:
 	case SETALL:
-            err = target_to_host_semarray(semid, &array, target_su.array);
+            err = target_to_host_semarray(semid, &array,
+                                          tswapal(target_su->array));
             if (err)
-                return err;
+                break;
             arg.array = array;
             ret = get_errno(semctl(semid, semnum, cmd, arg));
-            err = host_to_target_semarray(semid, target_su.array, &array);
-            if (err)
-                return err;
+            err = host_to_target_semarray(semid, tswapal(target_su->array),
+                                          &array);
             break;
 	case IPC_STAT:
 	case IPC_SET:
 	case SEM_STAT:
-            err = target_to_host_semid_ds(&dsarg, target_su.buf);
+            err = target_to_host_semid_ds(&dsarg, tswapal(target_su->buf));
             if (err)
-                return err;
+                break;
             arg.buf = &dsarg;
             ret = get_errno(semctl(semid, semnum, cmd, arg));
-            err = host_to_target_semid_ds(target_su.buf, &dsarg);
-            if (err)
-                return err;
+            err = host_to_target_semid_ds(tswapal(target_su->buf), &dsarg);
             break;
 	case IPC_INFO:
 	case SEM_INFO:
             arg.__buf = &seminfo;
             ret = get_errno(semctl(semid, semnum, cmd, arg));
-            err = host_to_target_seminfo(target_su.__buf, &seminfo);
-            if (err)
-                return err;
+            err = host_to_target_seminfo(tswapal(target_su->__buf), &seminfo);
             break;
 	case IPC_RMID:
 	case GETPID:
@@ -2692,6 +2692,7 @@  static inline abi_long do_semctl(int semid, int semnum, int cmd,
             ret = get_errno(semctl(semid, semnum, cmd, NULL));
             break;
     }
+    unlock_user_struct(target_su, ptr, 0);
 
     return ret;
 }
@@ -3162,7 +3163,7 @@  static abi_long do_ipc(unsigned int call, int first,
         break;
 
     case IPCOP_semctl:
-        ret = do_semctl(first, second, third, (union target_semun)(abi_ulong) ptr);
+        ret = do_semctl(first, second, third, ptr);
         break;
 
     case IPCOP_msgget:
@@ -3229,7 +3230,7 @@  static abi_long do_ipc(unsigned int call, int first,
 
 	/* IPC_* and SHM_* command values are the same on all linux platforms */
     case IPCOP_shmctl:
-        ret = do_shmctl(first, second, third);
+        ret = do_shmctl(first, second, ptr);
         break;
     default:
 	gemu_log("Unsupported ipc call: %d (version %d)\n", call, version);
@@ -6891,7 +6892,7 @@  abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
 #endif
 #ifdef TARGET_NR_semctl
     case TARGET_NR_semctl:
-        ret = do_semctl(arg1, arg2, arg3, (union target_semun)(abi_ulong)arg4);
+        ret = do_semctl(arg1, arg2, arg3, arg4);
         break;
 #endif
 #ifdef TARGET_NR_msgctl