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