| Message ID | 1359661807-9307-1-git-send-email-laurent@vivier.eu |
|---|---|
| State | New |
| Headers | show |
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
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
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
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(-)