[PATCH v2 0/2] spapr/nvram: Fix QEMU crash

[PATCH v2 0/2] spapr/nvram: Fix QEMU crash

[Greg Kurz]

This series fixes the following crash:

$ qemu-system-ppc64 -M pseries $(for ((x=0;x<128;x++)); do \
  echo -n " -prom-env "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
  done) # this requires ~128 Kib
malloc(): corrupted top size
Aborted (core dumped)

This also affects g3beige and mac99 machine types, and probably some
sparc/sparc64 machine types as well, but I prefer to leave the fixing
to knowledgeable people.

v2: - error out instead of increasing the partition size for the
      sake of migration (Laurent)
    - Cc'ing John Snow who reported the issue

---

Greg Kurz (2):
      nvram: Add dry_run argument to chrp_nvram_create_system_partition()
      spapr/nvram: Error out if NVRAM cannot contain all -prom-env data


 hw/nvram/chrp_nvram.c         |   34 +++++++++++++++++++++++-----------
 hw/nvram/mac_nvram.c          |    2 +-
 hw/nvram/spapr_nvram.c        |   18 +++++++++++++++++-
 hw/sparc/sun4m.c              |    2 +-
 hw/sparc64/sun4u.c            |    2 +-
 include/hw/nvram/chrp_nvram.h |    3 ++-
 6 files changed, 45 insertions(+), 16 deletions(-)

--
Greg

[PATCH v2 1/2] nvram: Add dry_run argument to chrp_nvram_create_system_partition()

[Greg Kurz]

Since commit 55d9950aaa8e ("nvram: Introduce helper functions for CHRP
"system" and "free space" partitions") it is possible to pre-initialize
a "system" partition in the NVRAM with the data passed to all -prom-env
parameters on the QEMU command line.

Unfortunately, this doesn't take the total size of the data into account
and chrp_nvram_create_system_partition() may crash at some point if the
caller hasn't allocated enough space.

Add a dry_run argument that causes chrp_nvram_create_system_partition()
to only return the size of the partition without actually copying data
into it. This can be used by callers to allocate enough memory.

Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/nvram/chrp_nvram.c         |   34 +++++++++++++++++++++++-----------
 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, 30 insertions(+), 16 deletions(-)

diff --git a/hw/nvram/chrp_nvram.c b/hw/nvram/chrp_nvram.c
index d969f267048e..f5f7b2a97c18 100644
--- a/hw/nvram/chrp_nvram.c
+++ b/hw/nvram/chrp_nvram.c
@@ -24,37 +24,48 @@
 #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,
+                              bool dry_run)
 {
     int len;
 
     len = strlen(str) + 1;
-    memcpy(&nvram[addr], str, len);
-
+    if (!dry_run) {
+        memcpy(&nvram[addr], str, len);
+    }
     return addr + len;
 }
 
 /**
  * Create a "system partition", used for the Open Firmware
- * environment variables.
+ * environment variables. If @dry_run is false, only returns
+ * the size of the partition but don't write the data.
  */
-int chrp_nvram_create_system_partition(uint8_t *data, int min_len)
+int chrp_nvram_create_system_partition(uint8_t *data, int min_len, bool dry_run)
 {
     ChrpNvramPartHdr *part_header;
     unsigned int i;
     int end;
 
+    assert(data || dry_run);
+
     part_header = (ChrpNvramPartHdr *)data;
-    part_header->signature = CHRP_NVPART_SYSTEM;
-    pstrcpy(part_header->name, sizeof(part_header->name), "system");
+
+    if (!dry_run) {
+        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], dry_run);
     }
 
     /* End marker */
-    data[end++] = '\0';
+    if (!dry_run) {
+        data[end] = '\0';
+    }
+    end++;
 
     end = (end + 15) & ~15;
     /* XXX: OpenBIOS is not able to grow up a partition. Leave some space for
@@ -62,8 +73,9 @@ int chrp_nvram_create_system_partition(uint8_t *data, int min_len)
     if (end < min_len) {
         end = min_len;
     }
-    chrp_nvram_finish_partition(part_header, end);
-
+    if (!dry_run) {
+        chrp_nvram_finish_partition(part_header, end);
+    }
     return end;
 }
 
diff --git a/hw/nvram/mac_nvram.c b/hw/nvram/mac_nvram.c
index beec1c4e4d11..4396f893f14a 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, false) + 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..992b818d34e7 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,
+                                           false);
         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..61804ccd4286 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, false);
 
     /* 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..2409e739e81b 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, false);
 
     /* 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..1d32dbf61331 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);
+int chrp_nvram_create_system_partition(uint8_t *data, int min_len,
+                                       bool dry_run);
 int chrp_nvram_create_free_partition(uint8_t *data, int len);
 
 #endif

[PATCH v2 2/2] spapr/nvram: Error out if NVRAM cannot contain all -prom-env data

[Greg Kurz]

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 cases 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 "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
  done) # this requires ~128 Kib
malloc(): corrupted top size
Aborted (core dumped)

Call chrp_nvram_create_system_partition() first, with its recently added
parameter dry_run set to true, in order to know the required size and fail
gracefully if it's too small.

Reported-by: John Snow <jsnow@redhat.com>
Signed-off-by: Greg Kurz <groug@kaod.org>
---
 hw/nvram/spapr_nvram.c |   15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
index 992b818d34e7..c29d797ae1f0 100644
--- a/hw/nvram/spapr_nvram.c
+++ b/hw/nvram/spapr_nvram.c
@@ -145,6 +145,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr,
 
 static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
 {
+    ERRP_GUARD();
     SpaprNvram *nvram = VIO_SPAPR_NVRAM(dev);
     int ret;
 
@@ -187,6 +188,20 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
             return;
         }
     } else if (nb_prom_envs > 0) {
+        int len = chrp_nvram_create_system_partition(nvram->buf,
+                                                     MIN_NVRAM_SIZE / 4,
+                                                     true);
+
+        /* Check the partition is large enough for all the -prom-env data */
+        if (nvram->size < len) {
+            error_setg(errp, "-prom-env data requires %d bytes but spapr-nvram "
+                       "is only %d bytes in size", len, nvram->size);
+            error_append_hint(errp,
+                              "Try to pass %d less bytes to -prom-env.\n",
+                              len - nvram->size);
+            return;
+        }
+
         /* Create a system partition to pass the -prom-env variables */
         chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4,
                                            false);

Re: [PATCH v2 2/2] spapr/nvram: Error out if NVRAM cannot contain all -prom-env data

[John Snow]

On 8/12/20 1:08 PM, 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 cases 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 "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
>    done) # this requires ~128 Kib
> malloc(): corrupted top size
> Aborted (core dumped)
> 
> Call chrp_nvram_create_system_partition() first, with its recently added
> parameter dry_run set to true, in order to know the required size and fail
> gracefully if it's too small.
> 
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>

Fixes: 61f20b9dc5b7
RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1867739

Thanks :)

> ---
>   hw/nvram/spapr_nvram.c |   15 +++++++++++++++
>   1 file changed, 15 insertions(+)
> 
> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> index 992b818d34e7..c29d797ae1f0 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -145,6 +145,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr,
>   
>   static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
>   {
> +    ERRP_GUARD();
>       SpaprNvram *nvram = VIO_SPAPR_NVRAM(dev);
>       int ret;
>   
> @@ -187,6 +188,20 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
>               return;
>           }
>       } else if (nb_prom_envs > 0) {
> +        int len = chrp_nvram_create_system_partition(nvram->buf,
> +                                                     MIN_NVRAM_SIZE / 4,
> +                                                     true);
> +
> +        /* Check the partition is large enough for all the -prom-env data */
> +        if (nvram->size < len) {
> +            error_setg(errp, "-prom-env data requires %d bytes but spapr-nvram "
> +                       "is only %d bytes in size", len, nvram->size);
> +            error_append_hint(errp,
> +                              "Try to pass %d less bytes to -prom-env.\n",
> +                              len - nvram->size);
> +            return;
> +        }
> +
>           /* Create a system partition to pass the -prom-env variables */
>           chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4,
>                                              false);
> 
> 
> 

Re: [PATCH v2 2/2] spapr/nvram: Error out if NVRAM cannot contain all -prom-env data

[Laurent Vivier]

Le 12/08/2020 à 19:08, 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 cases 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 "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
>   done) # this requires ~128 Kib
> malloc(): corrupted top size
> Aborted (core dumped)
> 
> Call chrp_nvram_create_system_partition() first, with its recently added
> parameter dry_run set to true, in order to know the required size and fail
> gracefully if it's too small.

Why do you need the dry_run parameter?
Can't you fail on the normal case?

Thanks,
Laurent

> 
> Reported-by: John Snow <jsnow@redhat.com>
> Signed-off-by: Greg Kurz <groug@kaod.org>
> ---
>  hw/nvram/spapr_nvram.c |   15 +++++++++++++++
>  1 file changed, 15 insertions(+)
> 
> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> index 992b818d34e7..c29d797ae1f0 100644
> --- a/hw/nvram/spapr_nvram.c
> +++ b/hw/nvram/spapr_nvram.c
> @@ -145,6 +145,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr,
>  
>  static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
>  {
> +    ERRP_GUARD();
>      SpaprNvram *nvram = VIO_SPAPR_NVRAM(dev);
>      int ret;
>  
> @@ -187,6 +188,20 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
>              return;
>          }
>      } else if (nb_prom_envs > 0) {
> +        int len = chrp_nvram_create_system_partition(nvram->buf,
> +                                                     MIN_NVRAM_SIZE / 4,
> +                                                     true);
> +
> +        /* Check the partition is large enough for all the -prom-env data */
> +        if (nvram->size < len) {
> +            error_setg(errp, "-prom-env data requires %d bytes but spapr-nvram "
> +                       "is only %d bytes in size", len, nvram->size);
> +            error_append_hint(errp,
> +                              "Try to pass %d less bytes to -prom-env.\n",
> +                              len - nvram->size);
> +            return;
> +        }
> +
>          /* Create a system partition to pass the -prom-env variables */
>          chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4,
>                                             false);
> 
> 

Re: [PATCH v2 2/2] spapr/nvram: Error out if NVRAM cannot contain all -prom-env data

[Greg Kurz]

On Wed, 12 Aug 2020 19:29:26 +0200
Laurent Vivier <laurent@vivier.eu> wrote:

> Le 12/08/2020 à 19:08, 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 cases 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 "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
> >   done) # this requires ~128 Kib
> > malloc(): corrupted top size
> > Aborted (core dumped)
> > 
> > Call chrp_nvram_create_system_partition() first, with its recently added
> > parameter dry_run set to true, in order to know the required size and fail
> > gracefully if it's too small.
> 
> Why do you need the dry_run parameter?
> Can't you fail on the normal case?
> 

Not sure what the "normal case" stands for... but basically, only
chrp_nvram_create_system_partition() knows the exact size of the
partition (ie. size of the header + size of all prom-env strings
including the terminal nul + padding to the upper 16-byte aligment).

Another solution could be to pass the buffer size and errp to
chrp_nvram_create_system_partition() and chrp_nvram_set_var(),
and let chrp_nvram_set_var() check it won't memcpy() past the
buffer. But this is more code and since this is also used by
other machine types, I chose to go for the dry_run parameter.

Should I improve the changelog to make this clearer or are
you thinking to something else ?

> Thanks,
> Laurent
> 
> > 
> > Reported-by: John Snow <jsnow@redhat.com>
> > Signed-off-by: Greg Kurz <groug@kaod.org>
> > ---
> >  hw/nvram/spapr_nvram.c |   15 +++++++++++++++
> >  1 file changed, 15 insertions(+)
> > 
> > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> > index 992b818d34e7..c29d797ae1f0 100644
> > --- a/hw/nvram/spapr_nvram.c
> > +++ b/hw/nvram/spapr_nvram.c
> > @@ -145,6 +145,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr,
> >  
> >  static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
> >  {
> > +    ERRP_GUARD();
> >      SpaprNvram *nvram = VIO_SPAPR_NVRAM(dev);
> >      int ret;
> >  
> > @@ -187,6 +188,20 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
> >              return;
> >          }
> >      } else if (nb_prom_envs > 0) {
> > +        int len = chrp_nvram_create_system_partition(nvram->buf,
> > +                                                     MIN_NVRAM_SIZE / 4,
> > +                                                     true);
> > +
> > +        /* Check the partition is large enough for all the -prom-env data */
> > +        if (nvram->size < len) {
> > +            error_setg(errp, "-prom-env data requires %d bytes but spapr-nvram "
> > +                       "is only %d bytes in size", len, nvram->size);
> > +            error_append_hint(errp,
> > +                              "Try to pass %d less bytes to -prom-env.\n",
> > +                              len - nvram->size);
> > +            return;
> > +        }
> > +
> >          /* Create a system partition to pass the -prom-env variables */
> >          chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4,
> >                                             false);
> > 
> > 
> 

Re: [PATCH v2 2/2] spapr/nvram: Error out if NVRAM cannot contain all -prom-env data

[David Gibson]

[-- Attachment #1: Type: text/plain, Size: 4176 bytes --]

On Wed, Aug 12, 2020 at 09:06:54PM +0200, Greg Kurz wrote:
> On Wed, 12 Aug 2020 19:29:26 +0200
> Laurent Vivier <laurent@vivier.eu> wrote:
> 
> > Le 12/08/2020 à 19:08, 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 cases 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 "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
> > >   done) # this requires ~128 Kib
> > > malloc(): corrupted top size
> > > Aborted (core dumped)
> > > 
> > > Call chrp_nvram_create_system_partition() first, with its recently added
> > > parameter dry_run set to true, in order to know the required size and fail
> > > gracefully if it's too small.
> > 
> > Why do you need the dry_run parameter?
> > Can't you fail on the normal case?
> > 
> 
> Not sure what the "normal case" stands for... but basically, only
> chrp_nvram_create_system_partition() knows the exact size of the
> partition (ie. size of the header + size of all prom-env strings
> including the terminal nul + padding to the upper 16-byte aligment).
> 
> Another solution could be to pass the buffer size and errp to
> chrp_nvram_create_system_partition() and chrp_nvram_set_var(),
> and let chrp_nvram_set_var() check it won't memcpy() past the
> buffer. But this is more code and since this is also used by
> other machine types, I chose to go for the dry_run parameter.

Hm, it does feel like a more natural interface to me, though, rather
than always having to call it twice.  Basically just add a "max_size"
parameter.

> Should I improve the changelog to make this clearer or are
> you thinking to something else ?
> 
> > Thanks,
> > Laurent
> > 
> > > 
> > > Reported-by: John Snow <jsnow@redhat.com>
> > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > ---
> > >  hw/nvram/spapr_nvram.c |   15 +++++++++++++++
> > >  1 file changed, 15 insertions(+)
> > > 
> > > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> > > index 992b818d34e7..c29d797ae1f0 100644
> > > --- a/hw/nvram/spapr_nvram.c
> > > +++ b/hw/nvram/spapr_nvram.c
> > > @@ -145,6 +145,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr,
> > >  
> > >  static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
> > >  {
> > > +    ERRP_GUARD();
> > >      SpaprNvram *nvram = VIO_SPAPR_NVRAM(dev);
> > >      int ret;
> > >  
> > > @@ -187,6 +188,20 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
> > >              return;
> > >          }
> > >      } else if (nb_prom_envs > 0) {
> > > +        int len = chrp_nvram_create_system_partition(nvram->buf,
> > > +                                                     MIN_NVRAM_SIZE / 4,
> > > +                                                     true);
> > > +
> > > +        /* Check the partition is large enough for all the -prom-env data */
> > > +        if (nvram->size < len) {
> > > +            error_setg(errp, "-prom-env data requires %d bytes but spapr-nvram "
> > > +                       "is only %d bytes in size", len, nvram->size);
> > > +            error_append_hint(errp,
> > > +                              "Try to pass %d less bytes to -prom-env.\n",
> > > +                              len - nvram->size);
> > > +            return;
> > > +        }
> > > +
> > >          /* Create a system partition to pass the -prom-env variables */
> > >          chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4,
> > >                                             false);
> > > 
> > > 
> > 
> 

-- 
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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 2/2] spapr/nvram: Error out if NVRAM cannot contain all -prom-env data

[Greg Kurz]

[-- Attachment #1: Type: text/plain, Size: 4452 bytes --]

On Thu, 13 Aug 2020 16:43:36 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Aug 12, 2020 at 09:06:54PM +0200, Greg Kurz wrote:
> > On Wed, 12 Aug 2020 19:29:26 +0200
> > Laurent Vivier <laurent@vivier.eu> wrote:
> > 
> > > Le 12/08/2020 à 19:08, 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 cases 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 "$(for ((y=0;y<1024;y++)); do echo -n x ; done) ; \
> > > >   done) # this requires ~128 Kib
> > > > malloc(): corrupted top size
> > > > Aborted (core dumped)
> > > > 
> > > > Call chrp_nvram_create_system_partition() first, with its recently added
> > > > parameter dry_run set to true, in order to know the required size and fail
> > > > gracefully if it's too small.
> > > 
> > > Why do you need the dry_run parameter?
> > > Can't you fail on the normal case?
> > > 
> > 
> > Not sure what the "normal case" stands for... but basically, only
> > chrp_nvram_create_system_partition() knows the exact size of the
> > partition (ie. size of the header + size of all prom-env strings
> > including the terminal nul + padding to the upper 16-byte aligment).
> > 
> > Another solution could be to pass the buffer size and errp to
> > chrp_nvram_create_system_partition() and chrp_nvram_set_var(),
> > and let chrp_nvram_set_var() check it won't memcpy() past the
> > buffer. But this is more code and since this is also used by
> > other machine types, I chose to go for the dry_run parameter.
> 
> Hm, it does feel like a more natural interface to me, though, rather
> than always having to call it twice.  Basically just add a "max_size"
> parameter.
> 

Ok, I can do that but we won't be able to report much to the
user appart from "spapr-nvram (64 KiB) is too small", without
any accurate hint to reduce the size of the -prom-env data.

> > Should I improve the changelog to make this clearer or are
> > you thinking to something else ?
> > 
> > > Thanks,
> > > Laurent
> > > 
> > > > 
> > > > Reported-by: John Snow <jsnow@redhat.com>
> > > > Signed-off-by: Greg Kurz <groug@kaod.org>
> > > > ---
> > > >  hw/nvram/spapr_nvram.c |   15 +++++++++++++++
> > > >  1 file changed, 15 insertions(+)
> > > > 
> > > > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c
> > > > index 992b818d34e7..c29d797ae1f0 100644
> > > > --- a/hw/nvram/spapr_nvram.c
> > > > +++ b/hw/nvram/spapr_nvram.c
> > > > @@ -145,6 +145,7 @@ static void rtas_nvram_store(PowerPCCPU *cpu, SpaprMachineState *spapr,
> > > >  
> > > >  static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
> > > >  {
> > > > +    ERRP_GUARD();
> > > >      SpaprNvram *nvram = VIO_SPAPR_NVRAM(dev);
> > > >      int ret;
> > > >  
> > > > @@ -187,6 +188,20 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp)
> > > >              return;
> > > >          }
> > > >      } else if (nb_prom_envs > 0) {
> > > > +        int len = chrp_nvram_create_system_partition(nvram->buf,
> > > > +                                                     MIN_NVRAM_SIZE / 4,
> > > > +                                                     true);
> > > > +
> > > > +        /* Check the partition is large enough for all the -prom-env data */
> > > > +        if (nvram->size < len) {
> > > > +            error_setg(errp, "-prom-env data requires %d bytes but spapr-nvram "
> > > > +                       "is only %d bytes in size", len, nvram->size);
> > > > +            error_append_hint(errp,
> > > > +                              "Try to pass %d less bytes to -prom-env.\n",
> > > > +                              len - nvram->size);
> > > > +            return;
> > > > +        }
> > > > +
> > > >          /* Create a system partition to pass the -prom-env variables */
> > > >          chrp_nvram_create_system_partition(nvram->buf, MIN_NVRAM_SIZE / 4,
> > > >                                             false);
> > > > 
> > > > 
> > > 
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]