On Thu, 13 Aug 2020 16:43:36 +1000 David Gibson <da...@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 <laur...@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 <js...@redhat.com>
> > > > Signed-off-by: Greg Kurz <gr...@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);
> > > >
> > > >
> > >
> >
>
pgpb0hqp063qH.pgp
Description: OpenPGP digital signature
