[Qemu-devel] [PATCH] linux-user: Don't assert if guest tries shmdt(0)

[Qemu-devel] [PATCH] linux-user: Don't assert if guest tries shmdt(0)

[Peter Maydell]

Our implementation of shmat() and shmdt() for linux-user was
using "zero guest address" as its marker for "entry in the
shm_regions[] array is not in use". This meant that if the
guest did a shmdt(0) we would match on an unused array entry
and call page_set_flags() with both start and end addresses zero,
which causes an assertion failure.

Use an explicit in_use flag to manage the shm_regions[] array,
so that we avoid this problem.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
Reported-by: Pavel Shamis <pasharesearch@gmail.com>
---
 linux-user/syscall.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/linux-user/syscall.c b/linux-user/syscall.c
index 54ce14a..f46abf7 100644
--- a/linux-user/syscall.c
+++ b/linux-user/syscall.c
@@ -2598,8 +2598,9 @@ static abi_long do_socketcall(int num, abi_ulong vptr)
 #define N_SHM_REGIONS	32
 
 static struct shm_region {
-    abi_ulong	start;
-    abi_ulong	size;
+    abi_ulong start;
+    abi_ulong size;
+    bool in_use;
 } shm_regions[N_SHM_REGIONS];
 
 struct target_semid_ds
@@ -3291,7 +3292,8 @@ static inline abi_ulong do_shmat(int shmid, abi_ulong shmaddr, int shmflg)
                    ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE));
 
     for (i = 0; i < N_SHM_REGIONS; i++) {
-        if (shm_regions[i].start == 0) {
+        if (!shm_regions[i].in_use) {
+            shm_regions[i].in_use = true;
             shm_regions[i].start = raddr;
             shm_regions[i].size = shm_info.shm_segsz;
             break;
@@ -3308,8 +3310,8 @@ static inline abi_long do_shmdt(abi_ulong shmaddr)
     int i;
 
     for (i = 0; i < N_SHM_REGIONS; ++i) {
-        if (shm_regions[i].start == shmaddr) {
-            shm_regions[i].start = 0;
+        if (shm_regions[i].in_use && shm_regions[i].start == shmaddr) {
+            shm_regions[i].in_use = false;
             page_set_flags(shmaddr, shmaddr + shm_regions[i].size, 0);
             break;
         }
-- 
1.9.1

Re: [Qemu-devel] [PATCH] linux-user: Don't assert if guest tries shmdt(0)

[Laurent Vivier]

Le 09/02/2016 16:57, Peter Maydell a écrit :
> Our implementation of shmat() and shmdt() for linux-user was
> using "zero guest address" as its marker for "entry in the
> shm_regions[] array is not in use". This meant that if the
> guest did a shmdt(0) we would match on an unused array entry

Is shmdt(0) valid ?

I mean, if shmat() is called with shmaddr equal to 0:
"the system chooses a suitable (unused) address at which
to attach the segment."

and

"The to-be-detached segment must be currently attached with shmaddr
equal to the value returned by the attaching shmat() call."

Did you check shmat() can return 0 ?
(I think our mmap_find_vma() cannot return 0)

Why don't you fail on shmdt(0) (EINVAL) ?

> and call page_set_flags() with both start and end addresses zero,
> which causes an assertion failure.
> 
> Use an explicit in_use flag to manage the shm_regions[] array,
> so that we avoid this problem.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Pavel Shamis <pasharesearch@gmail.com>
> ---
>  linux-user/syscall.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 54ce14a..f46abf7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2598,8 +2598,9 @@ static abi_long do_socketcall(int num, abi_ulong vptr)
>  #define N_SHM_REGIONS	32
>  
>  static struct shm_region {
> -    abi_ulong	start;
> -    abi_ulong	size;
> +    abi_ulong start;
> +    abi_ulong size;
> +    bool in_use;
>  } shm_regions[N_SHM_REGIONS];
>  
>  struct target_semid_ds
> @@ -3291,7 +3292,8 @@ static inline abi_ulong do_shmat(int shmid, abi_ulong shmaddr, int shmflg)
>                     ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE));
>  
>      for (i = 0; i < N_SHM_REGIONS; i++) {
> -        if (shm_regions[i].start == 0) {
> +        if (!shm_regions[i].in_use) {
> +            shm_regions[i].in_use = true;
>              shm_regions[i].start = raddr;
>              shm_regions[i].size = shm_info.shm_segsz;
>              break;
> @@ -3308,8 +3310,8 @@ static inline abi_long do_shmdt(abi_ulong shmaddr)
>      int i;
>  
>      for (i = 0; i < N_SHM_REGIONS; ++i) {
> -        if (shm_regions[i].start == shmaddr) {
> -            shm_regions[i].start = 0;
> +        if (shm_regions[i].in_use && shm_regions[i].start == shmaddr) {
> +            shm_regions[i].in_use = false;
>              page_set_flags(shmaddr, shmaddr + shm_regions[i].size, 0);
>              break;
>          }
> 

Re: [Qemu-devel] [PATCH] linux-user: Don't assert if guest tries shmdt(0)

[Peter Maydell]

On 10 February 2016 at 18:39, Laurent Vivier <laurent@vivier.eu> wrote:
>
>
> Le 09/02/2016 16:57, Peter Maydell a écrit :
>> Our implementation of shmat() and shmdt() for linux-user was
>> using "zero guest address" as its marker for "entry in the
>> shm_regions[] array is not in use". This meant that if the
>> guest did a shmdt(0) we would match on an unused array entry
>
> Is shmdt(0) valid ?

It's valid in the sense of "should detach" if shmat() ever
returned 0 (which I suspect it will never do but have not
attempted to determine). It's valid in the sense of "should
not cause an assert" anyway.

> I mean, if shmat() is called with shmaddr equal to 0:
> "the system chooses a suitable (unused) address at which
> to attach the segment."
>
> and
>
> "The to-be-detached segment must be currently attached with shmaddr
> equal to the value returned by the attaching shmat() call."
>
> Did you check shmat() can return 0 ?
> (I think our mmap_find_vma() cannot return 0)

Not wanting to try to figure this out is why I switched to
having an extra in_use flag in the shm_regions[] array.
0 is now not any kind of special value as far as addresses
go -- if shmat() returned 0 as a valid address then we'll
record it in the array, and shmdt() will work. If it
never did, then shmdt() won't find any valid entries,
we'll call the host with shmdt() on something that wasn't
an attached segment and the host kernel will fail the
syscall as it should.

> Why don't you fail on shmdt(0) (EINVAL) ?

We let the host kernel do the error checking and return
the errno for us, at which point it will indeed fail EINVAL.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] linux-user: Don't assert if guest tries shmdt(0)

[Laurent Vivier]

Le 09/02/2016 16:57, Peter Maydell a écrit :
> Our implementation of shmat() and shmdt() for linux-user was
> using "zero guest address" as its marker for "entry in the
> shm_regions[] array is not in use". This meant that if the
> guest did a shmdt(0) we would match on an unused array entry
> and call page_set_flags() with both start and end addresses zero,
> which causes an assertion failure.
> 
> Use an explicit in_use flag to manage the shm_regions[] array,
> so that we avoid this problem.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> Reported-by: Pavel Shamis <pasharesearch@gmail.com>
Reviewed-by: Laurent Vivier <laurent@vivier.eu>

> ---
>  linux-user/syscall.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
> index 54ce14a..f46abf7 100644
> --- a/linux-user/syscall.c
> +++ b/linux-user/syscall.c
> @@ -2598,8 +2598,9 @@ static abi_long do_socketcall(int num, abi_ulong vptr)
>  #define N_SHM_REGIONS	32
>  
>  static struct shm_region {
> -    abi_ulong	start;
> -    abi_ulong	size;
> +    abi_ulong start;
> +    abi_ulong size;
> +    bool in_use;
>  } shm_regions[N_SHM_REGIONS];
>  
>  struct target_semid_ds
> @@ -3291,7 +3292,8 @@ static inline abi_ulong do_shmat(int shmid, abi_ulong shmaddr, int shmflg)
>                     ((shmflg & SHM_RDONLY)? 0 : PAGE_WRITE));
>  
>      for (i = 0; i < N_SHM_REGIONS; i++) {
> -        if (shm_regions[i].start == 0) {
> +        if (!shm_regions[i].in_use) {
> +            shm_regions[i].in_use = true;
>              shm_regions[i].start = raddr;
>              shm_regions[i].size = shm_info.shm_segsz;
>              break;
> @@ -3308,8 +3310,8 @@ static inline abi_long do_shmdt(abi_ulong shmaddr)
>      int i;
>  
>      for (i = 0; i < N_SHM_REGIONS; ++i) {
> -        if (shm_regions[i].start == shmaddr) {
> -            shm_regions[i].start = 0;
> +        if (shm_regions[i].in_use && shm_regions[i].start == shmaddr) {
> +            shm_regions[i].in_use = false;
>              page_set_flags(shmaddr, shmaddr + shm_regions[i].size, 0);
>              break;
>          }
>