| Message ID | 159725213748.104309.14834084670144632611.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr/nvram: Fix QEMU crash | expand |
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); > > >
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); > >
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); > > > > >
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); > > > > > > > > >
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); > > > > > > > > > > > > > >
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);
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(+)