[Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Thomas Huth]

When running QEMU with "-M none -device loader,file=kernel.elf", it
currently crashes with a segmentation fault, because the "none"-machine
does not have any CPU by default and the generic loader code tries
to dereference s->cpu. Fix it by adding an appropriate check for a
NULL pointer.

Reported-by: Laurent Vivier <laurent@vivier.eu>
Signed-off-by: Thomas Huth <thuth@redhat.com>
---
 hw/core/generic-loader.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
index 58f1f02..4601267 100644
--- a/hw/core/generic-loader.c
+++ b/hw/core/generic-loader.c
@@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
 #endif
 
     if (s->file) {
+        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
+
         if (!s->force_raw) {
             size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL,
-                               big_endian, 0, 0, 0, s->cpu->as);
+                               big_endian, 0, 0, 0, as);
 
             if (size < 0) {
                 size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL,
-                                      s->cpu->as);
+                                      as);
             }
         }
 
         if (size < 0 || s->force_raw) {
             /* Default to the maximum size being the machine's ram size */
-            size = load_image_targphys_as(s->file, s->addr, ram_size,
-                                          s->cpu->as);
+            size = load_image_targphys_as(s->file, s->addr, ram_size, as);
         } else {
             s->addr = entry;
         }
-- 
1.8.3.1

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Laurent Vivier]

Le 25/01/2017 à 21:45, Thomas Huth a écrit :
> When running QEMU with "-M none -device loader,file=kernel.elf", it
> currently crashes with a segmentation fault, because the "none"-machine
> does not have any CPU by default and the generic loader code tries
> to dereference s->cpu. Fix it by adding an appropriate check for a
> NULL pointer.
> 
> Reported-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/generic-loader.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index 58f1f02..4601267 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>  #endif
>  
>      if (s->file) {
> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;

Should we just abort if s->cpu is NULL?

Laurent

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Alistair Francis]

On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 25/01/2017 à 21:45, Thomas Huth a écrit :
>> When running QEMU with "-M none -device loader,file=kernel.elf", it
>> currently crashes with a segmentation fault, because the "none"-machine
>> does not have any CPU by default and the generic loader code tries
>> to dereference s->cpu. Fix it by adding an appropriate check for a
>> NULL pointer.
>>
>> Reported-by: Laurent Vivier <laurent@vivier.eu>
>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>> ---
>>  hw/core/generic-loader.c | 9 +++++----
>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>
>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>> index 58f1f02..4601267 100644
>> --- a/hw/core/generic-loader.c
>> +++ b/hw/core/generic-loader.c
>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>>  #endif
>>
>>      if (s->file) {
>> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
>
> Should we just abort if s->cpu is NULL?

I agree, what is the use case where you are loading images without a CPU?

If there is a use case (maybe some KVM thing?) then this patch looks fine to me.

Thanks,

Alistair

>
> Laurent
>
>

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Thomas Huth]

On 26.01.2017 00:26, Alistair Francis wrote:
> On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 25/01/2017 à 21:45, Thomas Huth a écrit :
>>> When running QEMU with "-M none -device loader,file=kernel.elf", it
>>> currently crashes with a segmentation fault, because the "none"-machine
>>> does not have any CPU by default and the generic loader code tries
>>> to dereference s->cpu. Fix it by adding an appropriate check for a
>>> NULL pointer.
>>>
>>> Reported-by: Laurent Vivier <laurent@vivier.eu>
>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>> ---
>>>  hw/core/generic-loader.c | 9 +++++----
>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>>> index 58f1f02..4601267 100644
>>> --- a/hw/core/generic-loader.c
>>> +++ b/hw/core/generic-loader.c
>>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>>>  #endif
>>>
>>>      if (s->file) {
>>> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
>>
>> Should we just abort if s->cpu is NULL?
> 
> I agree, what is the use case where you are loading images without a CPU?
> 
> If there is a use case (maybe some KVM thing?) then this patch looks fine to me.

I think there is no real use case yet. But this fix is 1) simpler than
doing an error_report() + exit() here, and 2) maybe the vision of
constructing machines on the fly with QEMU will eventually come true one
day in the distant future, so with that patch here, the code would
already be prepared for the case when QEMU gets started without CPUs and
the CPUs are then later added via QOM...
Well, I don't mind ... if you prefer an error message instead, feel free
to suggest another patch. I'm fine as long as we do not simply crash
with a segmentation fault here.

 Thomas

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Laurent Vivier]

Le 26/01/2017 à 06:50, Thomas Huth a écrit :
> On 26.01.2017 00:26, Alistair Francis wrote:
>> On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote:
>>> Le 25/01/2017 à 21:45, Thomas Huth a écrit :
>>>> When running QEMU with "-M none -device loader,file=kernel.elf", it
>>>> currently crashes with a segmentation fault, because the "none"-machine
>>>> does not have any CPU by default and the generic loader code tries
>>>> to dereference s->cpu. Fix it by adding an appropriate check for a
>>>> NULL pointer.
>>>>
>>>> Reported-by: Laurent Vivier <laurent@vivier.eu>
>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>> ---
>>>>  hw/core/generic-loader.c | 9 +++++----
>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>>>> index 58f1f02..4601267 100644
>>>> --- a/hw/core/generic-loader.c
>>>> +++ b/hw/core/generic-loader.c
>>>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>>>>  #endif
>>>>
>>>>      if (s->file) {
>>>> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
>>>
>>> Should we just abort if s->cpu is NULL?
>>
>> I agree, what is the use case where you are loading images without a CPU?
>>
>> If there is a use case (maybe some KVM thing?) then this patch looks fine to me.
> 
> I think there is no real use case yet. But this fix is 1) simpler than
> doing an error_report() + exit() here, and 2) maybe the vision of
> constructing machines on the fly with QEMU will eventually come true one
> day in the distant future, so with that patch here, the code would
> already be prepared for the case when QEMU gets started without CPUs and
> the CPUs are then later added via QOM...
> Well, I don't mind ... if you prefer an error message instead, feel free
> to suggest another patch. I'm fine as long as we do not simply crash
> with a segmentation fault here.

OK, the use of NULL as "as" seems reasonable (this is what uses
load_elf()), so:

Reviewed-by: Laurent Vivier <laurent@vivier.eu>

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Alistair Francis]

On Thu, Jan 26, 2017 at 12:38 AM, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 26/01/2017 à 06:50, Thomas Huth a écrit :
>> On 26.01.2017 00:26, Alistair Francis wrote:
>>> On Wed, Jan 25, 2017 at 12:52 PM, Laurent Vivier <laurent@vivier.eu> wrote:
>>>> Le 25/01/2017 à 21:45, Thomas Huth a écrit :
>>>>> When running QEMU with "-M none -device loader,file=kernel.elf", it
>>>>> currently crashes with a segmentation fault, because the "none"-machine
>>>>> does not have any CPU by default and the generic loader code tries
>>>>> to dereference s->cpu. Fix it by adding an appropriate check for a
>>>>> NULL pointer.
>>>>>
>>>>> Reported-by: Laurent Vivier <laurent@vivier.eu>
>>>>> Signed-off-by: Thomas Huth <thuth@redhat.com>
>>>>> ---
>>>>>  hw/core/generic-loader.c | 9 +++++----
>>>>>  1 file changed, 5 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
>>>>> index 58f1f02..4601267 100644
>>>>> --- a/hw/core/generic-loader.c
>>>>> +++ b/hw/core/generic-loader.c
>>>>> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>>>>>  #endif
>>>>>
>>>>>      if (s->file) {
>>>>> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
>>>>
>>>> Should we just abort if s->cpu is NULL?
>>>
>>> I agree, what is the use case where you are loading images without a CPU?
>>>
>>> If there is a use case (maybe some KVM thing?) then this patch looks fine to me.
>>
>> I think there is no real use case yet. But this fix is 1) simpler than
>> doing an error_report() + exit() here, and 2) maybe the vision of
>> constructing machines on the fly with QEMU will eventually come true one
>> day in the distant future, so with that patch here, the code would
>> already be prepared for the case when QEMU gets started without CPUs and
>> the CPUs are then later added via QOM...

Ok, that is a good enough reason for me.

Reviewed-by: Alistair Francis <alistair.francis@xilinx.com>

Thanks,

Alistair

>> Well, I don't mind ... if you prefer an error message instead, feel free
>> to suggest another patch. I'm fine as long as we do not simply crash
>> with a segmentation fault here.
>
> OK, the use of NULL as "as" seems reasonable (this is what uses
> load_elf()), so:
>
> Reviewed-by: Laurent Vivier <laurent@vivier.eu>
>
>
>

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Peter Maydell]

On 26 January 2017 at 05:50, Thomas Huth <thuth@redhat.com> wrote:
> I think there is no real use case yet. But this fix is 1) simpler than
> doing an error_report() + exit() here, and 2) maybe the vision of
> constructing machines on the fly with QEMU will eventually come true one
> day in the distant future, so with that patch here, the code would
> already be prepared for the case when QEMU gets started without CPUs and
> the CPUs are then later added via QOM...

For the latter to work you'd need to do something more anyway,
because you still need to be able to say "load this image
into this CPU's address space", even if the CPUs are getting
added later via QOM. You'd need to say "delay the image
load until we actually have a CPU to load it for" or
something... Passing a NULL address space to load_elf_as()
etc is saying "I am definitely a board which has
system_address_space as its one and only address space",
which is OK if you're a board model but a bit more dubious
if you're a generic device like this one.

thanks
-- PMM

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Thomas Huth]

On 25.01.2017 21:45, Thomas Huth wrote:
> When running QEMU with "-M none -device loader,file=kernel.elf", it
> currently crashes with a segmentation fault, because the "none"-machine
> does not have any CPU by default and the generic loader code tries
> to dereference s->cpu. Fix it by adding an appropriate check for a
> NULL pointer.
> 
> Reported-by: Laurent Vivier <laurent@vivier.eu>
> Signed-off-by: Thomas Huth <thuth@redhat.com>
> ---
>  hw/core/generic-loader.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/hw/core/generic-loader.c b/hw/core/generic-loader.c
> index 58f1f02..4601267 100644
> --- a/hw/core/generic-loader.c
> +++ b/hw/core/generic-loader.c
> @@ -137,20 +137,21 @@ static void generic_loader_realize(DeviceState *dev, Error **errp)
>  #endif
>  
>      if (s->file) {
> +        AddressSpace *as = s->cpu ? s->cpu->as :  NULL;
> +
>          if (!s->force_raw) {
>              size = load_elf_as(s->file, NULL, NULL, &entry, NULL, NULL,
> -                               big_endian, 0, 0, 0, s->cpu->as);
> +                               big_endian, 0, 0, 0, as);
>  
>              if (size < 0) {
>                  size = load_uimage_as(s->file, &entry, NULL, NULL, NULL, NULL,
> -                                      s->cpu->as);
> +                                      as);
>              }
>          }
>  
>          if (size < 0 || s->force_raw) {
>              /* Default to the maximum size being the machine's ram size */
> -            size = load_image_targphys_as(s->file, s->addr, ram_size,
> -                                          s->cpu->as);
> +            size = load_image_targphys_as(s->file, s->addr, ram_size, as);
>          } else {
>              s->addr = entry;
>          }

Ping?

 Thomas

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Alistair Francis]

On Fri, Jan 27, 2017 at 9:06 AM, Peter Maydell <peter.maydell@linaro.org> wrote:
> On 26 January 2017 at 05:50, Thomas Huth <thuth@redhat.com> wrote:
>> I think there is no real use case yet. But this fix is 1) simpler than
>> doing an error_report() + exit() here, and 2) maybe the vision of
>> constructing machines on the fly with QEMU will eventually come true one
>> day in the distant future, so with that patch here, the code would
>> already be prepared for the case when QEMU gets started without CPUs and
>> the CPUs are then later added via QOM...
>
> For the latter to work you'd need to do something more anyway,
> because you still need to be able to say "load this image
> into this CPU's address space", even if the CPUs are getting
> added later via QOM. You'd need to say "delay the image
> load until we actually have a CPU to load it for" or
> something... Passing a NULL address space to load_elf_as()
> etc is saying "I am definitely a board which has
> system_address_space as its one and only address space",
> which is OK if you're a board model but a bit more dubious
> if you're a generic device like this one.

Doesn't this not just crashing at least move us a step closer in the
right direction (even if it isn't the correct address space)? The
other option of just printing an error and exit doesn't seem super
useful.

Either is better then a segfault though, so we need to do something here.

Thanks,

Alistair

>
> thanks
> -- PMM
>

Re: [Qemu-devel] [PATCH] hw/core/generic-loader: Fix crash when running without CPU

[Michael Tokarev]

27.02.2017 22:36, Thomas Huth wrote:
> On 25.01.2017 21:45, Thomas Huth wrote:
>> When running QEMU with "-M none -device loader,file=kernel.elf", it
>> currently crashes with a segmentation fault, because the "none"-machine
>> does not have any CPU by default and the generic loader code tries
>> to dereference s->cpu. Fix it by adding an appropriate check for a
>> NULL pointer.

Applied to -trivial, thanks!

/mjt