[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH for-4.1] tests/migration-test: Fix read off end
From: |
Laurent Vivier |
Subject: |
Re: [Qemu-devel] [PATCH for-4.1] tests/migration-test: Fix read off end of aarch64_kernel array |
Date: |
Tue, 2 Jul 2019 17:19:43 +0200 |
User-agent: |
Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.7.0 |
On 02/07/2019 17:03, 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: Laurent Vivier <address@hidden>