| From aac48d07764ce73c2ba23e3f05ccd29db190024a Mon Sep 17 00:00:00 2001 |
| From: Greg Kurz <gkurz@redhat.com> |
| Date: Thu, 8 Oct 2020 11:06:43 -0400 |
| Subject: [PATCH 04/14] nvram: Exit QEMU if NVRAM cannot contain all -prom-env |
| data |
| |
| RH-Author: Greg Kurz <gkurz@redhat.com> |
| Message-id: <20201008110643.155902-2-gkurz@redhat.com> |
| Patchwork-id: 98577 |
| O-Subject: [RHEL-8.4.0 qemu-kvm PATCH 1/1] nvram: Exit QEMU if NVRAM cannot contain all -prom-env data |
| Bugzilla: 1874780 |
| RH-Acked-by: David Gibson <dgibson@redhat.com> |
| RH-Acked-by: Laurent Vivier <lvivier@redhat.com> |
| RH-Acked-by: Thomas Huth <thuth@redhat.com> |
| |
| From: Greg Kurz <groug@kaod.org> |
| |
| Since commit 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to |
| support the -prom-env parameter"), pseries machines can pre-initialize |
| the "system" partition in the NVRAM with the data passed to all -prom-env |
| parameters on the QEMU command line. |
| |
| In this case it is assumed that all the data fits in 64 KiB, but the user |
| can easily pass more and crash QEMU: |
| |
| $ qemu-system-ppc64 -M pseries $(for ((x=0;x<128;x++)); do \ |
| echo -n " -prom-env " ; printf "%0.sx" {1..1024}; \ |
| done) # this requires ~128 Kib |
| malloc(): corrupted top size |
| Aborted (core dumped) |
| |
| This happens because we don't check if all the prom-env data fits in |
| the NVRAM and chrp_nvram_set_var() happily memcpy() it passed the |
| buffer. |
| |
| This crash affects basically all ppc/ppc64 machine types that use -prom-env: |
| - pseries (all versions) |
| - g3beige |
| - mac99 |
| |
| and also sparc/sparc64 machine types: |
| - LX |
| - SPARCClassic |
| - SPARCbook |
| - SS-10 |
| - SS-20 |
| - SS-4 |
| - SS-5 |
| - SS-600MP |
| - Voyager |
| - sun4u |
| - sun4v |
| |
| Add a max_len argument to chrp_nvram_create_system_partition() so that |
| it can check the available size before writing to memory. |
| |
| Since NVRAM is populated at machine init, it seems reasonable to consider |
| this error as fatal. So, instead of reporting an error when we detect that |
| the NVRAM is too small and adapt all machine types to handle it, we simply |
| exit QEMU in all cases. This is still better than crashing. If someone |
| wants another behavior, I guess this can be reworked later. |
| |
| Tested with: |
| |
| $ yes q | \ |
| (for arch in ppc ppc64 sparc sparc64; do \ |
| echo == $arch ==; \ |
| qemu=${arch}-softmmu/qemu-system-$arch; \ |
| for mach in $($qemu -M help | awk '! /^Supported/ { print $1 }'); do \ |
| echo $mach; \ |
| $qemu -M $mach -monitor stdio -nodefaults -nographic \ |
| $(for ((x=0;x<128;x++)); do \ |
| echo -n " -prom-env " ; printf "%0.sx" {1..1024}; \ |
| done) >/dev/null; \ |
| done; echo; \ |
| done) |
| |
| Without the patch, affected machine types cause QEMU to report some |
| memory corruption and crash: |
| |
| malloc(): corrupted top size |
| |
| free(): invalid size |
| |
| |
| |
| With the patch, QEMU prints the following message and exits: |
| |
| NVRAM is too small. Try to pass less data to -prom-env |
| |
| It seems that the conditions for the crash have always existed, but it |
| affects pseries, the machine type I care for, since commit 61f20b9dc5b7 |
| only. |
| |
| Fixes: 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to support the -prom-env parameter") |
| RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1867739 |
| Reported-by: John Snow <jsnow@redhat.com> |
| Reviewed-by: Laurent Vivier <laurent@vivier.eu> |
| Signed-off-by: Greg Kurz <groug@kaod.org> |
| Message-Id: <159736033937.350502.12402444542194031035.stgit@bahia.lan> |
| Signed-off-by: David Gibson <david@gibson.dropbear.id.au> |
| (cherry picked from commit 37035df51eaabb8d26b71da75b88a1c6727de8fa) |
| Signed-off-by: Greg Kurz <gkurz@redhat.com> |
| Signed-off-by: Danilo C. L. de Paula <ddepaula@redhat.com> |
| |
| hw/nvram/chrp_nvram.c | 24 +++++++++++++++++++++--- |
| hw/nvram/mac_nvram.c | 2 +- |
| hw/nvram/spapr_nvram.c | 3 ++- |
| hw/sparc/sun4m.c | 2 +- |
| hw/sparc64/sun4u.c | 2 +- |
| include/hw/nvram/chrp_nvram.h | 3 ++- |
| 6 files changed, 28 insertions(+), 8 deletions(-) |
| |
| diff --git a/hw/nvram/chrp_nvram.c b/hw/nvram/chrp_nvram.c |
| index d969f26704..d4d10a7c03 100644 |
| |
| |
| @@ -21,14 +21,21 @@ |
| |
| #include "qemu/osdep.h" |
| #include "qemu/cutils.h" |
| +#include "qemu/error-report.h" |
| #include "hw/nvram/chrp_nvram.h" |
| #include "sysemu/sysemu.h" |
| |
| -static int chrp_nvram_set_var(uint8_t *nvram, int addr, const char *str) |
| +static int chrp_nvram_set_var(uint8_t *nvram, int addr, const char *str, |
| + int max_len) |
| { |
| int len; |
| |
| len = strlen(str) + 1; |
| + |
| + if (max_len < len) { |
| + return -1; |
| + } |
| + |
| memcpy(&nvram[addr], str, len); |
| |
| return addr + len; |
| @@ -38,19 +45,26 @@ static int chrp_nvram_set_var(uint8_t *nvram, int addr, const char *str) |
| * Create a "system partition", used for the Open Firmware |
| * environment variables. |
| */ |
| -int chrp_nvram_create_system_partition(uint8_t *data, int min_len) |
| +int chrp_nvram_create_system_partition(uint8_t *data, int min_len, int max_len) |
| { |
| ChrpNvramPartHdr *part_header; |
| unsigned int i; |
| int end; |
| |
| + if (max_len < sizeof(*part_header)) { |
| + goto fail; |
| + } |
| + |
| part_header = (ChrpNvramPartHdr *)data; |
| part_header->signature = CHRP_NVPART_SYSTEM; |
| pstrcpy(part_header->name, sizeof(part_header->name), "system"); |
| |
| end = sizeof(ChrpNvramPartHdr); |
| for (i = 0; i < nb_prom_envs; i++) { |
| - end = chrp_nvram_set_var(data, end, prom_envs[i]); |
| + end = chrp_nvram_set_var(data, end, prom_envs[i], max_len - end); |
| + if (end == -1) { |
| + goto fail; |
| + } |
| } |
| |
| /* End marker */ |
| @@ -65,6 +79,10 @@ int chrp_nvram_create_system_partition(uint8_t *data, int min_len) |
| chrp_nvram_finish_partition(part_header, end); |
| |
| return end; |
| + |
| +fail: |
| + error_report("NVRAM is too small. Try to pass less data to -prom-env"); |
| + exit(EXIT_FAILURE); |
| } |
| |
| /** |
| diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c |
| index 9a47e35b8e..ecfb36182f 100644 |
| |
| |
| @@ -152,7 +152,7 @@ static void pmac_format_nvram_partition_of(MacIONVRAMState *nvr, int off, |
| |
| /* OpenBIOS nvram variables partition */ |
| sysp_end = chrp_nvram_create_system_partition(&nvr->data[off], |
| - DEF_SYSTEM_SIZE) + off; |
| + DEF_SYSTEM_SIZE, len) + off; |
| |
| /* Free space partition */ |
| chrp_nvram_create_free_partition(&nvr->data[sysp_end], len - sysp_end); |
| diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c |
| index 838082b451..225cd69b49 100644 |
| |
| |
| @@ -188,7 +188,8 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) |
| } |
| } else if (nb_prom_envs > 0) { |
| /* Create a system partition to pass the -prom-env variables */ |
| - chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4); |
| + chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4, |
| + nvram->size); |
| chrp_nvram_create_free_partition(&nvram->buf[MIN_NVRAM_SIZE / 4], |
| nvram->size - MIN_NVRAM_SIZE / 4); |
| } |
| diff --git a/hw/sparc/sun4m.c b/hw/sparc/sun4m.c |
| index 2aaa5bf1ae..cf2d0762d9 100644 |
| |
| |
| @@ -142,7 +142,7 @@ static void nvram_init(Nvram *nvram, uint8_t *macaddr, |
| memset(image, '\0', sizeof(image)); |
| |
| /* OpenBIOS nvram variables partition */ |
| - sysp_end = chrp_nvram_create_system_partition(image, 0); |
| + sysp_end = chrp_nvram_create_system_partition(image, 0, 0x1fd0); |
| |
| /* Free space partition */ |
| chrp_nvram_create_free_partition(&image[sysp_end], 0x1fd0 - sysp_end); |
| diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c |
| index 955082773b..f5295a687e 100644 |
| |
| |
| @@ -137,7 +137,7 @@ static int sun4u_NVRAM_set_params(Nvram *nvram, uint16_t NVRAM_size, |
| memset(image, '\0', sizeof(image)); |
| |
| /* OpenBIOS nvram variables partition */ |
| - sysp_end = chrp_nvram_create_system_partition(image, 0); |
| + sysp_end = chrp_nvram_create_system_partition(image, 0, 0x1fd0); |
| |
| /* Free space partition */ |
| chrp_nvram_create_free_partition(&image[sysp_end], 0x1fd0 - sysp_end); |
| diff --git a/include/hw/nvram/chrp_nvram.h b/include/hw/nvram/chrp_nvram.h |
| index 09941a9be4..4a0f5c21b8 100644 |
| |
| |
| @@ -50,7 +50,8 @@ chrp_nvram_finish_partition(ChrpNvramPartHdr *header, uint32_t size) |
| header->checksum = sum & 0xff; |
| } |
| |
| -int chrp_nvram_create_system_partition(uint8_t *data, int min_len); |
| +/* chrp_nvram_create_system_partition() failure is fatal */ |
| +int chrp_nvram_create_system_partition(uint8_t *data, int min_len, int max_len); |
| int chrp_nvram_create_free_partition(uint8_t *data, int len); |
| |
| #endif |
| -- |
| 2.27.0 |
| |