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

login
register
mail settings
Submitter Laurent Vivier
Date Jan. 31, 2013, 7:50 p.m.
Message ID <1359661807-9307-1-git-send-email-laurent@vivier.eu>
Download mbox | patch
Permalink /patch/217249/
State New
Headers show

Comments

Laurent Vivier - Jan. 31, 2013, 7:50 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()
v3: correctly set the return value
v3: don't duplicate unlock_user_struct(), set err to ret instead
v4: replace all return by if (err) { ret = err; break; }

 linux-user/syscall.c |   56 +++++++++++++++++++++++++++++++-------------------
 1 file changed, 35 insertions(+), 21 deletions(-)
Peter Maydell - Feb. 4, 2013, 3:16 p.m.
On 31 January 2013 19:50, Laurent Vivier <laurent@vivier.eu> wrote:
> The parameter "union semun" of semctl() is not a value
> but a pointer to the value.

Hi. For your next patch could you make sure you send it as
a fresh email rather than a followup to the previous version?
Anthony's patch-handling tools don't really like followups.

> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2652,8 +2652,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;
> @@ -2662,43 +2663,55 @@ 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;
> +    }

This breaks x86_64 linux-user. The fourth argument to semctl()
is a union of pointers, not a pointer to a union. That means that
the lock_user_struct/whatever has to be done differently for the
individual cases, depending on how we are supposed to interpret
the argument (which field of the union we're using).

My testcase is simple:

QEMU_STRACE=1 ./x86_64-linux-user/qemu-x86_64 /usr/bin/ipcs

which before your patch does this:

14654 semctl(0,0,SEM_INFO,0x0000004000800490) = 0
14654 write(1,0x10d4000,33)------ Semaphore Arrays --------
 = 33

(ie we successfully get back the info)

14654 write(1,0x10d4000,55)key        semid      owner      perms
nsems
 = 55
14654 semctl(0,0,SEM_STAT,0x0000004000800420) = -1 errno=22 (Invalid argument)
14654 write(1,0x10d4000,1)
 = 1

and afterwards does this:

14723 semctl(0,0,SEM_INFO,0x0000004000800490) = -1 errno=14 (Bad address)
14723 write(1,0x10d4000,37)kernel not configured for semaphores
 = 37

(SEM_INFO fails and ipcs prints a failure message)

because we end up with target_su->__buf == 11 which isn't a
valid address to pass to host_to_target_seminfo().

-- PMM
Laurent Vivier - Feb. 4, 2013, 9:03 p.m.
Le lundi 04 février 2013 à 15:16 +0000, Peter Maydell a écrit :
> On 31 January 2013 19:50, Laurent Vivier <laurent@vivier.eu> wrote:
> > The parameter "union semun" of semctl() is not a value
> > but a pointer to the value.
> 
> Hi. For your next patch could you make sure you send it as
> a fresh email rather than a followup to the previous version?
> Anthony's patch-handling tools don't really like followups.

OK

> > --- a/linux-user/syscall.c
> > +++ b/linux-user/syscall.c
> > @@ -2652,8 +2652,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;
> > @@ -2662,43 +2663,55 @@ 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;
> > +    }
> 
> This breaks x86_64 linux-user. The fourth argument to semctl()
> is a union of pointers, not a pointer to a union. That means that

In fact, it depends on the architecture. After a look in the kernel
sources, it seems compat_sys_semctl() uses a pointer, sys_semctl() an
union. compat_sys_semctl() seems to be used by mips32, pp32, sparc32 and
x86_32.

> the lock_user_struct/whatever has to be done differently for the
> individual cases, depending on how we are supposed to interpret
> the argument (which field of the union we're using).
> 
> My testcase is simple:
> 
> QEMU_STRACE=1 ./x86_64-linux-user/qemu-x86_64 /usr/bin/ipcs
> 
> which before your patch does this:
> 
> 14654 semctl(0,0,SEM_INFO,0x0000004000800490) = 0
> 14654 write(1,0x10d4000,33)------ Semaphore Arrays --------
>  = 33
> 
> (ie we successfully get back the info)
> 
> 14654 write(1,0x10d4000,55)key        semid      owner      perms
> nsems
>  = 55
> 14654 semctl(0,0,SEM_STAT,0x0000004000800420) = -1 errno=22 (Invalid argument)
> 14654 write(1,0x10d4000,1)
>  = 1
> 
> and afterwards does this:
> 
> 14723 semctl(0,0,SEM_INFO,0x0000004000800490) = -1 errno=14 (Bad address)
> 14723 write(1,0x10d4000,37)kernel not configured for semaphores
>  = 37
> 
> (SEM_INFO fails and ipcs prints a failure message)
> 
> because we end up with target_su->__buf == 11 which isn't a
> valid address to pass to host_to_target_seminfo().

Thank you for your help,
Laurent

Patch

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 08538fc..6610c24 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2652,8 +2652,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;
@@ -2662,43 +2663,55 @@  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);
-            if (err)
-                return err;
+            err = target_to_host_semarray(semid, &array,
+                                          tswapal(target_su->array));
+            if (err) {
+                ret = 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);
+            if (err) {
+                ret = err;
+            }
             break;
 	case IPC_STAT:
 	case IPC_SET:
 	case SEM_STAT:
-            err = target_to_host_semid_ds(&dsarg, target_su.buf);
-            if (err)
-                return err;
+            err = target_to_host_semid_ds(&dsarg, tswapal(target_su->buf));
+            if (err) {
+                ret = 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);
+            if (err) {
+                ret = err;
+            }
             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);
+            if (err) {
+                ret = err;
+            }
             break;
 	case IPC_RMID:
 	case GETPID:
@@ -2707,6 +2720,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;
 }
@@ -3177,7 +3191,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:
@@ -3244,7 +3258,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);
@@ -6933,7 +6947,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