| Message ID | 159715981316.1635409.16117540313443167075.stgit@bahia.lan |
|---|---|
| State | New |
| Headers | show |
| Series | spapr/nvram: Fix QEMU crash | expand |
On 11/08/2020 17:30, 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 false, to know the required size and allocate > the NVRAM buffer accordingly. > > Fixes: 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to support the -prom-env parameter") > Signed-off-by: Greg Kurz <groug@kaod.org> > --- > hw/nvram/spapr_nvram.c | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c > index 992b818d34e7..1b74bec6200a 100644 > --- a/hw/nvram/spapr_nvram.c > +++ b/hw/nvram/spapr_nvram.c > @@ -165,6 +165,10 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) > if (ret < 0) { > return; > } > + } else if (nb_prom_envs > 0) { > + nvram->size = chrp_nvram_create_system_partition(NULL, > + MIN_NVRAM_SIZE / 4, > + true); I think this will break the migration: the prom-env parameters can be on the source side without being on the dest side. And so the pram size will differ and the migration will fail. Thanks, Laurent
On Tue, 11 Aug 2020 18:05:21 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > On 11/08/2020 17:30, 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 false, to know the required size and allocate > > the NVRAM buffer accordingly. > > > > Fixes: 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to support the -prom-env parameter") > > Signed-off-by: Greg Kurz <groug@kaod.org> > > --- > > hw/nvram/spapr_nvram.c | 4 ++++ > > 1 file changed, 4 insertions(+) > > > > diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c > > index 992b818d34e7..1b74bec6200a 100644 > > --- a/hw/nvram/spapr_nvram.c > > +++ b/hw/nvram/spapr_nvram.c > > @@ -165,6 +165,10 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) > > if (ret < 0) { > > return; > > } > > + } else if (nb_prom_envs > 0) { > > + nvram->size = chrp_nvram_create_system_partition(NULL, > > + MIN_NVRAM_SIZE / 4, > > + true); > > I think this will break the migration: the prom-env parameters can be on > the source side without being on the dest side. And so the pram size Huh ? Migration mandates to have the same arguments on the command line, and libvirt buys us that AFAIK or am I missing something ? > will differ and the migration will fail. > > Thanks, > Laurent >
On 11/08/2020 18:36, Greg Kurz wrote: > On Tue, 11 Aug 2020 18:05:21 +0200 > Laurent Vivier <lvivier@redhat.com> wrote: > >> On 11/08/2020 17:30, 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 false, to know the required size and allocate >>> the NVRAM buffer accordingly. >>> >>> Fixes: 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to support the -prom-env parameter") >>> Signed-off-by: Greg Kurz <groug@kaod.org> >>> --- >>> hw/nvram/spapr_nvram.c | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c >>> index 992b818d34e7..1b74bec6200a 100644 >>> --- a/hw/nvram/spapr_nvram.c >>> +++ b/hw/nvram/spapr_nvram.c >>> @@ -165,6 +165,10 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) >>> if (ret < 0) { >>> return; >>> } >>> + } else if (nb_prom_envs > 0) { >>> + nvram->size = chrp_nvram_create_system_partition(NULL, >>> + MIN_NVRAM_SIZE / 4, >>> + true); >> >> I think this will break the migration: the prom-env parameters can be on >> the source side without being on the dest side. And so the pram size > > Huh ? Migration mandates to have the same arguments on the command line, > and libvirt buys us that AFAIK or am I missing something ? It's not true for all the arguments: take for instance the blockdev, the chardev, the netdev, vnc, ... I think it's true only for parameters that define the devices, the machine. And until now for prom-env it was not the case, look at the migration-test test case. Thanks, Laurent
On Tue, 11 Aug 2020 19:27:50 +0200 Laurent Vivier <lvivier@redhat.com> wrote: > On 11/08/2020 18:36, Greg Kurz wrote: > > On Tue, 11 Aug 2020 18:05:21 +0200 > > Laurent Vivier <lvivier@redhat.com> wrote: > > > >> On 11/08/2020 17:30, 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 false, to know the required size and allocate > >>> the NVRAM buffer accordingly. > >>> > >>> Fixes: 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to support the -prom-env parameter") > >>> Signed-off-by: Greg Kurz <groug@kaod.org> > >>> --- > >>> hw/nvram/spapr_nvram.c | 4 ++++ > >>> 1 file changed, 4 insertions(+) > >>> > >>> diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c > >>> index 992b818d34e7..1b74bec6200a 100644 > >>> --- a/hw/nvram/spapr_nvram.c > >>> +++ b/hw/nvram/spapr_nvram.c > >>> @@ -165,6 +165,10 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) > >>> if (ret < 0) { > >>> return; > >>> } > >>> + } else if (nb_prom_envs > 0) { > >>> + nvram->size = chrp_nvram_create_system_partition(NULL, > >>> + MIN_NVRAM_SIZE / 4, > >>> + true); > >> > >> I think this will break the migration: the prom-env parameters can be on > >> the source side without being on the dest side. And so the pram size > > > > Huh ? Migration mandates to have the same arguments on the command line, > > and libvirt buys us that AFAIK or am I missing something ? > > It's not true for all the arguments: take for instance the blockdev, the > chardev, the netdev, vnc, ... I think it's true only for parameters > that define the devices, the machine. And until now for prom-env it was > not the case, look at the migration-test test case. > arch_source = g_strdup_printf("-nodefaults " "-prom-env 'use-nvramrc?=true' -prom-env " "'nvramrc=hex .\" _\" begin %x %x " "do i c@ 1 + i c! 1000 +loop .\" B\" 0 " "until'", end_address, start_address); arch_target = g_strdup(""); Ok, my bad... I should have run that before posting. So, I'll go the other way around and have QEMU to error out if the -prom-env data doesn't fit in the default size (64 Kib). > Thanks, > Laurent > >
diff --git a/hw/nvram/spapr_nvram.c b/hw/nvram/spapr_nvram.c index 992b818d34e7..1b74bec6200a 100644 --- a/hw/nvram/spapr_nvram.c +++ b/hw/nvram/spapr_nvram.c @@ -165,6 +165,10 @@ static void spapr_nvram_realize(SpaprVioDevice *dev, Error **errp) if (ret < 0) { return; } + } else if (nb_prom_envs > 0) { + nvram->size = chrp_nvram_create_system_partition(NULL, + MIN_NVRAM_SIZE / 4, + true); } else { nvram->size = DEFAULT_NVRAM_SIZE; }
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 false, to know the required size and allocate the NVRAM buffer accordingly. Fixes: 61f20b9dc5b7 ("spapr_nvram: Pre-initialize the NVRAM to support the -prom-env parameter") Signed-off-by: Greg Kurz <groug@kaod.org> --- hw/nvram/spapr_nvram.c | 4 ++++ 1 file changed, 4 insertions(+)