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(-)
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
*** stack smashing detected ***: terminated
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>
---
v2: - fixed system partition size for sparc/sparc64 (Laurent)
---
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 d969f267048e..d4d10a7c03c7 100644
--- a/hw/nvram/chrp_nvram.c
+++ b/hw/nvram/chrp_nvram.c
@@ -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 beec1c4e4d11..11f2d31cdb20 100644
--- a/hw/nvram/mac_nvram.c
+++ b/hw/nvram/mac_nvram.c
@@ -141,7 +141,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 15d08281d411..386513499f59 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -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 9be930415f8e..cf7dfa4af5de 100644
--- a/hw/sparc/sun4m.c
+++ b/hw/sparc/sun4m.c
@@ -143,7 +143,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 9e30203dcc44..37310b73e6c2 100644
--- a/hw/sparc64/sun4u.c
+++ b/hw/sparc64/sun4u.c
@@ -136,7 +136,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 09941a9be454..4a0f5c21b884 100644
--- a/include/hw/nvram/chrp_nvram.h
+++ b/include/hw/nvram/chrp_nvram.h
@@ -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
On Fri, Aug 14, 2020 at 01:12:19AM +0200, Greg Kurz wrote:
> 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
>
> *** stack smashing detected ***: terminated
>
> 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>
Applied to ppc-for-5.2, thanks.
> ---
> v2: - fixed system partition size for sparc/sparc64 (Laurent)
> ---
> 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 d969f267048e..d4d10a7c03c7 100644
> --- a/hw/nvram/chrp_nvram.c
> +++ b/hw/nvram/chrp_nvram.c
> @@ -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 beec1c4e4d11..11f2d31cdb20 100644
> --- a/hw/nvram/mac_nvram.c
> +++ b/hw/nvram/mac_nvram.c
> @@ -141,7 +141,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 15d08281d411..386513499f59 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -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 9be930415f8e..cf7dfa4af5de 100644
> --- a/hw/sparc/sun4m.c
> +++ b/hw/sparc/sun4m.c
> @@ -143,7 +143,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 9e30203dcc44..37310b73e6c2 100644
> --- a/hw/sparc64/sun4u.c
> +++ b/hw/sparc64/sun4u.c
> @@ -136,7 +136,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 09941a9be454..4a0f5c21b884 100644
> --- a/include/hw/nvram/chrp_nvram.h
> +++ b/include/hw/nvram/chrp_nvram.h
> @@ -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
>
>
--
David Gibson | I'll have my music baroque, and my code
david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_
| _way_ _around_!
http://www.ozlabs.org/~dgibson
© 2016 - 2024 Red Hat, Inc.