fuzz: do not use POSIX shm for coverage bitmap

[PATCH] fuzz: do not use POSIX shm for coverage bitmap

Posted by Alexander Bulekov 5 years, 7 months ago

We used shm_open with mmap to share libfuzzer's coverage bitmap with
child (runner) processes. The same functionality can be achieved with
MAP_SHARED | MAP_ANONYMOUS, since we do not care about naming or
permissioning the shared memory object.

Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
---
This might fix:
qemu-fuzz-i386-target-virtio-net-socket: Unexpected-exit in
counter_shm_init 
https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23636 (private link)

oss-fuzz does not provide access to /dev/, so it is likely that shm_open
breaks, when it tries to access /dev/shm. This seems likely, based on
the oss-fuzz minijail setup:
https://github.com/google/oss-fuzz/blob/3740c751fd9edea138c17783995d370d6b1b89bc/infra/base-images/base-runner/run_minijail

 tests/qtest/fuzz/fork_fuzz.c | 40 ++++++++++++------------------------
 1 file changed, 13 insertions(+), 27 deletions(-)

diff --git a/tests/qtest/fuzz/fork_fuzz.c b/tests/qtest/fuzz/fork_fuzz.c
index 2bd0851903..6ffb2a7937 100644
--- a/tests/qtest/fuzz/fork_fuzz.c
+++ b/tests/qtest/fuzz/fork_fuzz.c
@@ -17,39 +17,25 @@
 
 void counter_shm_init(void)
 {
-    char *shm_path = g_strdup_printf("/qemu-fuzz-cntrs.%d", getpid());
-    int fd = shm_open(shm_path, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
-    g_free(shm_path);
-
-    if (fd == -1) {
-        perror("Error: ");
-        exit(1);
-    }
-    if (ftruncate(fd, &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START) == -1) {
-        perror("Error: ");
-        exit(1);
-    }
-    /* Copy what's in the counter region to the shm.. */
-    void *rptr = mmap(NULL ,
-            &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
-            PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
-    memcpy(rptr,
+    /* Copy what's in the counter region to a temporary buffer.. */
+    void *copy = malloc(&__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
+    memcpy(copy,
            &__FUZZ_COUNTERS_START,
            &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
 
-    munmap(rptr, &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
-
-    /* And map the shm over the counter region */
-    rptr = mmap(&__FUZZ_COUNTERS_START,
-            &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
-            PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED, fd, 0);
-
-    close(fd);
-
-    if (!rptr) {
+    /* Map a shared region over the counter region */
+    if (mmap(&__FUZZ_COUNTERS_START,
+             &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
+             PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED | MAP_ANONYMOUS,
+             0, 0) == MAP_FAILED) {
         perror("Error: ");
         exit(1);
     }
+
+    /* Copy the original data back to the counter-region */
+    memcpy(&__FUZZ_COUNTERS_START, copy,
+           &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
+    free(copy);
 }
 
 
-- 
2.26.2

Re: [PATCH] fuzz: do not use POSIX shm for coverage bitmap

Posted by Darren Kenny 5 years, 7 months ago

Hi Alex,

On Monday, 2020-06-22 at 12:50:40 -04, Alexander Bulekov wrote:
> We used shm_open with mmap to share libfuzzer's coverage bitmap with
> child (runner) processes. The same functionality can be achieved with
> MAP_SHARED | MAP_ANONYMOUS, since we do not care about naming or
> permissioning the shared memory object.
>
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
> This might fix:
> qemu-fuzz-i386-target-virtio-net-socket: Unexpected-exit in
> counter_shm_init 
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23636 (private link)
>
> oss-fuzz does not provide access to /dev/, so it is likely that shm_open
> breaks, when it tries to access /dev/shm. This seems likely, based on
> the oss-fuzz minijail setup:
> https://github.com/google/oss-fuzz/blob/3740c751fd9edea138c17783995d370d6b1b89bc/infra/base-images/base-runner/run_minijail
>
>  tests/qtest/fuzz/fork_fuzz.c | 40 ++++++++++++------------------------
>  1 file changed, 13 insertions(+), 27 deletions(-)
>
> diff --git a/tests/qtest/fuzz/fork_fuzz.c b/tests/qtest/fuzz/fork_fuzz.c
> index 2bd0851903..6ffb2a7937 100644
> --- a/tests/qtest/fuzz/fork_fuzz.c
> +++ b/tests/qtest/fuzz/fork_fuzz.c
> @@ -17,39 +17,25 @@
>  
>  void counter_shm_init(void)
>  {
> -    char *shm_path = g_strdup_printf("/qemu-fuzz-cntrs.%d", getpid());
> -    int fd = shm_open(shm_path, O_CREAT | O_RDWR, S_IRUSR | S_IWUSR);
> -    g_free(shm_path);
> -
> -    if (fd == -1) {
> -        perror("Error: ");
> -        exit(1);
> -    }
> -    if (ftruncate(fd, &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START) == -1) {
> -        perror("Error: ");
> -        exit(1);
> -    }
> -    /* Copy what's in the counter region to the shm.. */
> -    void *rptr = mmap(NULL ,
> -            &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
> -            PROT_READ | PROT_WRITE, MAP_SHARED, fd, 0);
> -    memcpy(rptr,
> +    /* Copy what's in the counter region to a temporary buffer.. */
> +    void *copy = malloc(&__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
> +    memcpy(copy,
>             &__FUZZ_COUNTERS_START,
>             &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
>  
> -    munmap(rptr, &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
> -
> -    /* And map the shm over the counter region */
> -    rptr = mmap(&__FUZZ_COUNTERS_START,
> -            &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
> -            PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED, fd, 0);
> -
> -    close(fd);
> -
> -    if (!rptr) {
> +    /* Map a shared region over the counter region */
> +    if (mmap(&__FUZZ_COUNTERS_START,
> +             &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START,
> +             PROT_READ | PROT_WRITE, MAP_SHARED | MAP_FIXED | MAP_ANONYMOUS,
> +             0, 0) == MAP_FAILED) {

It's not really necessary I guess, but for completeness you might want
to free(copy) here too.

Otherwise, this looks good, so:

Reviewed-by: Darren Kenny <darren.kenny@oracle.com>

Thanks,

Darren

>          perror("Error: ");
>          exit(1);
>      }
> +
> +    /* Copy the original data back to the counter-region */
> +    memcpy(&__FUZZ_COUNTERS_START, copy,
> +           &__FUZZ_COUNTERS_END - &__FUZZ_COUNTERS_START);
> +    free(copy);
>  }
>  
>  
> -- 
> 2.26.2

Re: [PATCH] fuzz: do not use POSIX shm for coverage bitmap

Posted by Stefan Hajnoczi 5 years, 7 months ago

On Mon, Jun 22, 2020 at 12:50:40PM -0400, Alexander Bulekov wrote:
> We used shm_open with mmap to share libfuzzer's coverage bitmap with
> child (runner) processes. The same functionality can be achieved with
> MAP_SHARED | MAP_ANONYMOUS, since we do not care about naming or
> permissioning the shared memory object.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>
> ---
> This might fix:
> qemu-fuzz-i386-target-virtio-net-socket: Unexpected-exit in
> counter_shm_init 
> https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=23636 (private link)
> 
> oss-fuzz does not provide access to /dev/, so it is likely that shm_open
> breaks, when it tries to access /dev/shm. This seems likely, based on
> the oss-fuzz minijail setup:
> https://github.com/google/oss-fuzz/blob/3740c751fd9edea138c17783995d370d6b1b89bc/infra/base-images/base-runner/run_minijail
> 
>  tests/qtest/fuzz/fork_fuzz.c | 40 ++++++++++++------------------------
>  1 file changed, 13 insertions(+), 27 deletions(-)

Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>

Re: [PATCH] fuzz: do not use POSIX shm for coverage bitmap

Posted by Thomas Huth 5 years, 7 months ago

On 22/06/2020 18.50, Alexander Bulekov wrote:
> We used shm_open with mmap to share libfuzzer's coverage bitmap with
> child (runner) processes. The same functionality can be achieved with
> MAP_SHARED | MAP_ANONYMOUS, since we do not care about naming or
> permissioning the shared memory object.
> 
> Signed-off-by: Alexander Bulekov <alxndr@bu.edu>

Thanks, queued to qtest-next now:

  https://gitlab.com/huth/qemu/-/commits/qtest-next/

  Thomas