Re: [Qemu-devel] [PATCH for-4.1] tests/migration-test: Fix read off end of aarch64_kernel array

[Philippe Mathieu-Daudé]

On 7/2/19 5:03 PM, Peter Maydell wrote:
> The test aarch64 kernel is in an array defined with
>  unsigned char aarch64_kernel[] = { [...] }
> 
> which means it could be any size; currently it's quite small.
> However we write it to a file using init_bootfile(), which
> writes exactly 512 bytes to the file. This will break if
> we ever end up with a kernel larger than that, and will
> read garbage off the end of the array in the current setup
> where the kernel is smaller.
> 
> Make init_bootfile() take an argument giving the length of
> the data to write. This allows us to use it for all architectures
> (previously s390 had a special-purpose init_bootfile_s390x
> which hardcoded the file to write so it could write the
> correct length). We assert that the x86 bootfile really is
> exactly 512 bytes as it should be (and as we were previously
> just assuming it was).
> 
> This was detected by the clang-7 asan:
> ==15607==ERROR: AddressSanitizer: global-buffer-overflow on address 
> 0x55a796f51d20 at pc 0x55a796b89c2f bp 0x7ffc58e89160 sp 0x7ffc58e88908
> READ of size 512 at 0x55a796f51d20 thread T0
>     #0 0x55a796b89c2e in fwrite 
> (/home/petmay01/linaro/qemu-from-laptop/qemu/build/sanitizers/tests/migration-test+0xb0c2e)
>     #1 0x55a796c46492 in init_bootfile 
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration-test.c:99:5
>     #2 0x55a796c46492 in test_migrate_start 
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration-test.c:593
>     #3 0x55a796c44101 in test_baddest 
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration-test.c:854:9
>     #4 0x7f906ffd3cc9  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72cc9)
>     #5 0x7f906ffd3bfa  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72bfa)
>     #6 0x7f906ffd3bfa  (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72bfa)
>     #7 0x7f906ffd3ea1 in g_test_run_suite 
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72ea1)
>     #8 0x7f906ffd3ec0 in g_test_run 
> (/usr/lib/x86_64-linux-gnu/libglib-2.0.so.0+0x72ec0)
>     #9 0x55a796c43707 in main 
> /home/petmay01/linaro/qemu-from-laptop/qemu/tests/migration-test.c:1187:11
>     #10 0x7f906e9abb96 in __libc_start_main 
> /build/glibc-OTsEL5/glibc-2.27/csu/../csu/libc-start.c:310
>     #11 0x55a796b6c2d9 in _start 
> (/home/petmay01/linaro/qemu-from-laptop/qemu/build/sanitizers/tests/migration-test+0x932d9)
> 
> Signed-off-by: Peter Maydell <address@hidden>
> ---
> patchew's asan build doesn't spot this, so it's presumably
> using an older version of the sanitizers...
> 
> 
>  tests/migration-test.c | 22 +++++++---------------
>  1 file changed, 7 insertions(+), 15 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index 0cd014dbe51..b6434628e1c 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -91,23 +91,13 @@ static const char *tmpfs;
>   */
>  #include "tests/migration/i386/a-b-bootblock.h"
>  #include "tests/migration/aarch64/a-b-kernel.h"
> -
> -static void init_bootfile(const char *bootpath, void *content)
> -{
> -    FILE *bootfile = fopen(bootpath, "wb");
> -
> -    g_assert_cmpint(fwrite(content, 512, 1, bootfile), ==, 1);
> -    fclose(bootfile);
> -}
> -
>  #include "tests/migration/s390x/a-b-bios.h"
>  
> -static void init_bootfile_s390x(const char *bootpath)
> +static void init_bootfile(const char *bootpath, void *content, size_t len)
>  {
>      FILE *bootfile = fopen(bootpath, "wb");
> -    size_t len = sizeof(s390x_elf);
>  
> -    g_assert_cmpint(fwrite(s390x_elf, len, 1, bootfile), ==, 1);
> +    g_assert_cmpint(fwrite(content, len, 1, bootfile), ==, 1);
>      fclose(bootfile);
>  }
>  
> @@ -537,7 +527,9 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>      got_stop = false;
>      bootpath = g_strdup_printf("%s/bootsect", tmpfs);
>      if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {
> -        init_bootfile(bootpath, x86_bootsect);
> +        /* the assembled x86 boot sector should be exactly one sector large 
> */
> +        assert(sizeof(x86_bootsect) == 512);
> +        init_bootfile(bootpath, x86_bootsect, sizeof(x86_bootsect));
>          extra_opts = use_shmem ? get_shmem_opts("150M", shmem_path) : NULL;
>          cmd_src = g_strdup_printf("-machine accel=%s -m 150M"
>                                    " -name source,debug-threads=on"
> @@ -555,7 +547,7 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>          start_address = X86_TEST_MEM_START;
>          end_address = X86_TEST_MEM_END;
>      } else if (g_str_equal(arch, "s390x")) {
> -        init_bootfile_s390x(bootpath);
> +        init_bootfile(bootpath, s390x_elf, sizeof(s390x_elf));
>          extra_opts = use_shmem ? get_shmem_opts("128M", shmem_path) : NULL;
>          cmd_src = g_strdup_printf("-machine accel=%s -m 128M"
>                                    " -name source,debug-threads=on"
> @@ -590,7 +582,7 @@ static int test_migrate_start(QTestState **from, 
> QTestState **to,
>          start_address = PPC_TEST_MEM_START;
>          end_address = PPC_TEST_MEM_END;
>      } else if (strcmp(arch, "aarch64") == 0) {
> -        init_bootfile(bootpath, aarch64_kernel);
> +        init_bootfile(bootpath, aarch64_kernel, sizeof(aarch64_kernel));
>          extra_opts = use_shmem ? get_shmem_opts("150M", shmem_path) : NULL;
>          cmd_src = g_strdup_printf("-machine virt,accel=%s,gic-version=max "
>                                    "-name vmsource,debug-threads=on -cpu max "
> 

Reviewed-by: Philippe Mathieu-Daudé <address@hidden>