Re: [PATCH] nvram: Exit QEMU if NVRAM cannot contain all -prom-env data

[Laurent Vivier]

Le 13/08/2020 à 17:55, Greg Kurz a écrit :
> 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>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
> 
> This replaces the following series:
> 
> [PATCH v2 0/2] spapr/nvram: Fix QEMU crash
> http://patchwork.ozlabs.org/project/qemu-devel/list/?series=195269
> ---
>  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/sparc/sun4m.c b/hw/sparc/sun4m.c
> index 9be930415f8e..7d791ee82368 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, sizeof(image));
>  
>      /* Free space partition */
>      chrp_nvram_create_free_partition(&image[sysp_end], 0x1fd0 - sysp_end);

It seems the max size of the system partition is 0x1fd0, not sizeof(image).

> diff --git a/hw/sparc64/sun4u.c b/hw/sparc64/sun4u.c
> index 9e30203dcc44..bd0a6283f339 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, sizeof(image));
>  
>      /* Free space partition */
>      chrp_nvram_create_free_partition(&image[sysp_end], 0x1fd0 - sysp_end);

ditto

All the other changes seem good.

With the sizeof(image) fixed:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>