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

[Greg Kurz <groug@kaod.org>]

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