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

[David Gibson]

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

signature.asc
Description: PGP signature