Re: [PATCH 2/2] spapr/nvram: Allocate enough space for all -prom-env options

[Laurent Vivier <lvivier@redhat.com>]

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