* Re: [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl()
2012-12-20 20:58 [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl() Laurent Vivier
@ 2013-01-01 23:10 ` Laurent Vivier
2013-01-02 0:00 ` Peter Maydell
` (3 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2013-01-01 23:10 UTC (permalink / raw)
To: qemu-devel; +Cc: Riku Voipio
Ping !
Le jeudi 20 décembre 2012 à 21:58 +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>
> ---
> linux-user/syscall.c | 37 ++++++++++++++++++++++++++-----------
> 1 file changed, 26 insertions(+), 11 deletions(-)
>
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index c2a2343..7bab006 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2656,24 +2656,26 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
> 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;
> arg.array = array;
> ret = get_errno(semctl(semid, semnum, cmd, arg));
> - err = host_to_target_semarray(semid, target_su.array, &array);
> + err = host_to_target_semarray(semid, tswapal(target_su.array),
> + &array);
> if (err)
> return err;
> 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;
> arg.buf = &dsarg;
> ret = get_errno(semctl(semid, semnum, cmd, arg));
> - err = host_to_target_semid_ds(target_su.buf, &dsarg);
> + err = host_to_target_semid_ds(tswapal(target_su.buf), &dsarg);
> if (err)
> return err;
> break;
> @@ -2681,7 +2683,7 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
> case SEM_INFO:
> arg.__buf = &seminfo;
> ret = get_errno(semctl(semid, semnum, cmd, arg));
> - err = host_to_target_seminfo(target_su.__buf, &seminfo);
> + err = host_to_target_seminfo(tswapal(target_su.__buf), &seminfo);
> if (err)
> return err;
> break;
> @@ -3161,10 +3163,16 @@ static abi_long do_ipc(unsigned int call, int first,
> ret = get_errno(semget(first, second, third));
> break;
>
> - case IPCOP_semctl:
> - ret = do_semctl(first, second, third, (union target_semun)(abi_ulong) ptr);
> + case IPCOP_semctl: {
> + union target_semun *target_su;
> + if (!lock_user_struct(VERIFY_READ, target_su, ptr, 1)) {
> + ret = -TARGET_EFAULT;
> + break;
> + }
> + ret = do_semctl(first, second, third, *target_su);
> + unlock_user_struct(target_su, ptr, 0);
> break;
> -
> + }
> case IPCOP_msgget:
> ret = get_errno(msgget(first, second));
> break;
> @@ -3229,7 +3237,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);
> @@ -6996,9 +7004,16 @@ abi_long do_syscall(void *cpu_env, int num, abi_long arg1,
> break;
> #endif
> #ifdef TARGET_NR_semctl
> - case TARGET_NR_semctl:
> - ret = do_semctl(arg1, arg2, arg3, (union target_semun)(abi_ulong)arg4);
> + case TARGET_NR_semctl: {
> + union target_semun *target_su;
> + if (!lock_user_struct(VERIFY_READ, target_su, arg4, 1)) {
> + ret = -TARGET_EFAULT;
> + break;
> + }
> + ret = do_semctl(arg1, arg2, arg3, *target_su);
> + unlock_user_struct(target_su, ptr, 0);
> break;
> + }
> #endif
> #ifdef TARGET_NR_msgctl
> case TARGET_NR_msgctl:
--
"Just play. Have fun. Enjoy the game."
- Michael Jordan
"Just play. Have fun. Enjoy the game."
- Michael Jordan
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl()
2012-12-20 20:58 [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl() Laurent Vivier
2013-01-01 23:10 ` Laurent Vivier
@ 2013-01-02 0:00 ` Peter Maydell
2013-01-02 20:38 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
` (2 subsequent siblings)
4 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2013-01-02 0:00 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, qemu-devel
On 20 December 2012 20:58, Laurent Vivier <laurent@vivier.eu> wrote:
> The parameter "union semun" of semctl() is not a value
> but a pointer to the value.
> @@ -3161,10 +3163,16 @@ static abi_long do_ipc(unsigned int call, int first,
> ret = get_errno(semget(first, second, third));
> break;
>
> - case IPCOP_semctl:
> - ret = do_semctl(first, second, third, (union target_semun)(abi_ulong) ptr);
> + case IPCOP_semctl: {
> + union target_semun *target_su;
> + if (!lock_user_struct(VERIFY_READ, target_su, ptr, 1)) {
> + ret = -TARGET_EFAULT;
> + break;
> + }
> + ret = do_semctl(first, second, third, *target_su);
> + unlock_user_struct(target_su, ptr, 0);
Rather than doing the lock/unlock in both callers to do_semctl,
just pass do_semctl an abi_long and have it do the lock/unlock.
Rest of patch looks ok.
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread* [Qemu-devel] [PATCH][v2] linux-user: correct semctl() and shmctl()
2012-12-20 20:58 [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl() Laurent Vivier
2013-01-01 23:10 ` Laurent Vivier
2013-01-02 0:00 ` Peter Maydell
@ 2013-01-02 20:38 ` Laurent Vivier
2013-01-19 23:28 ` Laurent Vivier
2013-01-20 11:24 ` Peter Maydell
2013-01-20 21:12 ` [Qemu-devel] [PATCH] " Laurent Vivier
2013-01-21 6:25 ` [Qemu-devel] [PATCH][v3] " Laurent Vivier
4 siblings, 2 replies; 9+ messages in thread
From: Laurent Vivier @ 2013-01-02 20:38 UTC (permalink / raw)
To: Peter Maydell; +Cc: Riku Voipio, qemu-devel, Laurent Vivier
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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH][v2] linux-user: correct semctl() and shmctl()
2013-01-02 20:38 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
@ 2013-01-19 23:28 ` Laurent Vivier
2013-01-20 11:24 ` Peter Maydell
1 sibling, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2013-01-19 23:28 UTC (permalink / raw)
To: Peter Maydell; +Cc: Riku Voipio, qemu-devel
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
--
"Just play. Have fun. Enjoy the game."
- Michael Jordan
"Just play. Have fun. Enjoy the game."
- Michael Jordan
^ permalink raw reply [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH][v2] linux-user: correct semctl() and shmctl()
2013-01-02 20:38 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
2013-01-19 23:28 ` Laurent Vivier
@ 2013-01-20 11:24 ` Peter Maydell
1 sibling, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2013-01-20 11:24 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, qemu-devel
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl()
2012-12-20 20:58 [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl() Laurent Vivier
` (2 preceding siblings ...)
2013-01-02 20:38 ` [Qemu-devel] [PATCH][v2] " Laurent Vivier
@ 2013-01-20 21:12 ` Laurent Vivier
2013-01-20 21:45 ` Peter Maydell
2013-01-21 6:25 ` [Qemu-devel] [PATCH][v3] " Laurent Vivier
4 siblings, 1 reply; 9+ messages in thread
From: Laurent Vivier @ 2013-01-20 21:12 UTC (permalink / raw)
To: Peter Maydell; +Cc: Riku Voipio, qemu-devel, Laurent Vivier
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>
---
linux-user/syscall.c | 29 ++++++++++++++++++-----------
1 file changed, 18 insertions(+), 11 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 693e66f..3c1bb3d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2635,8 +2635,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;
@@ -2645,33 +2646,38 @@ 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;
arg.array = array;
ret = get_errno(semctl(semid, semnum, cmd, arg));
- err = host_to_target_semarray(semid, target_su.array, &array);
+ err = host_to_target_semarray(semid, tswapal(target_su->array),
+ &array);
if (err)
return err;
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;
arg.buf = &dsarg;
ret = get_errno(semctl(semid, semnum, cmd, arg));
- err = host_to_target_semid_ds(target_su.buf, &dsarg);
+ err = host_to_target_semid_ds(tswapal(target_su->buf), &dsarg);
if (err)
return err;
break;
@@ -2679,7 +2685,7 @@ static inline abi_long do_semctl(int semid, int semnum, int cmd,
case SEM_INFO:
arg.__buf = &seminfo;
ret = get_errno(semctl(semid, semnum, cmd, arg));
- err = host_to_target_seminfo(target_su.__buf, &seminfo);
+ err = host_to_target_seminfo(tswapal(target_su->__buf), &seminfo);
if (err)
return err;
break;
@@ -2690,6 +2696,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;
}
@@ -3160,7 +3167,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:
@@ -3227,7 +3234,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);
@@ -6909,7 +6916,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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl()
2013-01-20 21:12 ` [Qemu-devel] [PATCH] " Laurent Vivier
@ 2013-01-20 21:45 ` Peter Maydell
0 siblings, 0 replies; 9+ messages in thread
From: Peter Maydell @ 2013-01-20 21:45 UTC (permalink / raw)
To: Laurent Vivier; +Cc: Riku Voipio, qemu-devel
On 20 January 2013 21:12, Laurent Vivier <laurent@vivier.eu> wrote:
> 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;
This version is back to the same bug as before where the error
case here skips unlocking the struct.
PS: please include the version number (v2,v3,etc) in the [PATCH]
subject tag so it's easier to distinguish which is the most recent
version of a patch when you submit newer versions.
thanks
-- PMM
^ permalink raw reply [flat|nested] 9+ messages in thread
* [Qemu-devel] [PATCH][v3] linux-user: correct semctl() and shmctl()
2012-12-20 20:58 [Qemu-devel] [PATCH] linux-user: correct semctl() and shmctl() Laurent Vivier
` (3 preceding siblings ...)
2013-01-20 21:12 ` [Qemu-devel] [PATCH] " Laurent Vivier
@ 2013-01-21 6:25 ` Laurent Vivier
4 siblings, 0 replies; 9+ messages in thread
From: Laurent Vivier @ 2013-01-21 6:25 UTC (permalink / raw)
To: Peter Maydell; +Cc: Riku Voipio, qemu-devel, Laurent Vivier
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
linux-user/syscall.c | 49 +++++++++++++++++++++++++++++++++----------------
1 file changed, 33 insertions(+), 16 deletions(-)
diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 693e66f..d44558d 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2635,8 +2635,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;
@@ -2645,43 +2646,58 @@ 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)
+ err = target_to_host_semarray(semid, &array,
+ tswapal(target_su->array));
+ if (err) {
+ unlock_user_struct(target_su, ptr, 0);
return err;
+ }
arg.array = array;
ret = get_errno(semctl(semid, semnum, cmd, arg));
- err = host_to_target_semarray(semid, target_su.array, &array);
- if (err)
+ err = host_to_target_semarray(semid, tswapal(target_su->array),
+ &array);
+ if (err) {
+ unlock_user_struct(target_su, ptr, 0);
return err;
+ }
break;
case IPC_STAT:
case IPC_SET:
case SEM_STAT:
- err = target_to_host_semid_ds(&dsarg, target_su.buf);
- if (err)
+ err = target_to_host_semid_ds(&dsarg, tswapal(target_su->buf));
+ if (err) {
+ unlock_user_struct(target_su, ptr, 0);
return err;
+ }
arg.buf = &dsarg;
ret = get_errno(semctl(semid, semnum, cmd, arg));
- err = host_to_target_semid_ds(target_su.buf, &dsarg);
- if (err)
+ err = host_to_target_semid_ds(tswapal(target_su->buf), &dsarg);
+ if (err) {
+ unlock_user_struct(target_su, ptr, 0);
return 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)
+ err = host_to_target_seminfo(tswapal(target_su->__buf), &seminfo);
+ if (err) {
+ unlock_user_struct(target_su, ptr, 0);
return err;
+ }
break;
case IPC_RMID:
case GETPID:
@@ -2690,6 +2706,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;
}
@@ -3160,7 +3177,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:
@@ -3227,7 +3244,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);
@@ -6909,7 +6926,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
--
1.7.10.4
^ permalink raw reply related [flat|nested] 9+ messages in thread