diff mbox series

[v2,41/86] hw/hppa/machine: Correctly check the firmware is in PDC range

Message ID 1579100861-73692-42-git-send-email-imammedo@redhat.com
State New
Headers show
Series refactor main RAM allocation to use hostmem backend | expand

Commit Message

Igor Mammedov Jan. 15, 2020, 3:06 p.m. UTC 
From: Philippe Mathieu-Daudé <f4bug@amsat.org>

The firmware has to reside in the PDC range. If the Elf file
expects to load it below FIRMWARE_START, it is incorrect,
regardless the RAM size.

Acked-by: Helge Deller <deller@gmx.de>
Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
Signed-off-by: Igor Mammedov <imammedo@redhat.com>
---
 hw/hppa/machine.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

BALATON Zoltan Jan. 15, 2020, 6:15 p.m. UTC | #1 
On Wed, 15 Jan 2020, Igor Mammedov wrote:
> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>
> The firmware has to reside in the PDC range. If the Elf file
> expects to load it below FIRMWARE_START, it is incorrect,
> regardless the RAM size.
>
> Acked-by: Helge Deller <deller@gmx.de>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> ---
> hw/hppa/machine.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> index 5d0de26..6775d87 100644
> --- a/hw/hppa/machine.c
> +++ b/hw/hppa/machine.c
> @@ -155,7 +155,7 @@ static void machine_hppa_init(MachineState *machine)
>     qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
>                   "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
>                   firmware_low, firmware_high, firmware_entry);
> -    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
> +    if (firmware_low < FIRMWARE_START || firmware_high >= FIRMWARE_END) {
>         error_report("Firmware overlaps with memory or IO space");
>         exit(1);

Should this also be EXIT_FAILURE like in other places when you're changing 
the line nearby?

Regards,
BALATON Zoltan
Philippe Mathieu-Daudé Jan. 15, 2020, 7:14 p.m. UTC | #2 
On 1/15/20 7:15 PM, BALATON Zoltan wrote:
> On Wed, 15 Jan 2020, Igor Mammedov wrote:
>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>
>> The firmware has to reside in the PDC range. If the Elf file
>> expects to load it below FIRMWARE_START, it is incorrect,
>> regardless the RAM size.
>>
>> Acked-by: Helge Deller <deller@gmx.de>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>> ---
>> hw/hppa/machine.c | 2 +-
>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>> index 5d0de26..6775d87 100644
>> --- a/hw/hppa/machine.c
>> +++ b/hw/hppa/machine.c
>> @@ -155,7 +155,7 @@ static void machine_hppa_init(MachineState *machine)
>>     qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
>>                   "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
>>                   firmware_low, firmware_high, firmware_entry);
>> -    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
>> +    if (firmware_low < FIRMWARE_START || firmware_high >= 
>> FIRMWARE_END) {
>>         error_report("Firmware overlaps with memory or IO space");
>>         exit(1);
> 
> Should this also be EXIT_FAILURE like in other places when you're 
> changing the line nearby?

I didn't changed this line, this seems unrelated to the patch purpose.
BALATON Zoltan Jan. 15, 2020, 9:59 p.m. UTC | #3 
On Wed, 15 Jan 2020, Philippe Mathieu-Daudé wrote:
> On 1/15/20 7:15 PM, BALATON Zoltan wrote:
>> On Wed, 15 Jan 2020, Igor Mammedov wrote:
>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> 
>>> The firmware has to reside in the PDC range. If the Elf file
>>> expects to load it below FIRMWARE_START, it is incorrect,
>>> regardless the RAM size.
>>> 
>>> Acked-by: Helge Deller <deller@gmx.de>
>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>> ---
>>> hw/hppa/machine.c | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>>> index 5d0de26..6775d87 100644
>>> --- a/hw/hppa/machine.c
>>> +++ b/hw/hppa/machine.c
>>> @@ -155,7 +155,7 @@ static void machine_hppa_init(MachineState *machine)
>>>     qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
>>>                   "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
>>>                   firmware_low, firmware_high, firmware_entry);
>>> -    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
>>> +    if (firmware_low < FIRMWARE_START || firmware_high >= FIRMWARE_END) {
>>>         error_report("Firmware overlaps with memory or IO space");
>>>         exit(1);
>> 
>> Should this also be EXIT_FAILURE like in other places when you're changing 
>> the line nearby?
>
> I didn't changed this line, this seems unrelated to the patch purpose.

Fair enough. Just thought because there was patch 85/86 making that change 
to keep consistency. Maybe you can change this in that patch but I don't 
really mind just spotted it.

Regards,
BALATON Zoltan
Philippe Mathieu-Daudé Jan. 16, 2020, 3:14 p.m. UTC | #4 
On 1/15/20 10:59 PM, BALATON Zoltan wrote:
> On Wed, 15 Jan 2020, Philippe Mathieu-Daudé wrote:
>> On 1/15/20 7:15 PM, BALATON Zoltan wrote:
>>> On Wed, 15 Jan 2020, Igor Mammedov wrote:
>>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>>
>>>> The firmware has to reside in the PDC range. If the Elf file
>>>> expects to load it below FIRMWARE_START, it is incorrect,
>>>> regardless the RAM size.
>>>>
>>>> Acked-by: Helge Deller <deller@gmx.de>
>>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
>>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
>>>> ---
>>>> hw/hppa/machine.c | 2 +-
>>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
>>>> index 5d0de26..6775d87 100644
>>>> --- a/hw/hppa/machine.c
>>>> +++ b/hw/hppa/machine.c
>>>> @@ -155,7 +155,7 @@ static void machine_hppa_init(MachineState 
>>>> *machine)
>>>>     qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
>>>>                   "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
>>>>                   firmware_low, firmware_high, firmware_entry);
>>>> -    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
>>>> +    if (firmware_low < FIRMWARE_START || firmware_high >= 
>>>> FIRMWARE_END) {
>>>>         error_report("Firmware overlaps with memory or IO space");
>>>>         exit(1);
>>>
>>> Should this also be EXIT_FAILURE like in other places when you're 
>>> changing the line nearby?
>>
>> I didn't changed this line, this seems unrelated to the patch purpose.
> 
> Fair enough. Just thought because there was patch 85/86 making that 
> change to keep consistency. Maybe you can change this in that patch but 
> I don't really mind just spotted it.

Ah this is because it is a patch of mine included in Igor series, and 
Igor uses EXIT_FAILURE in his other patches, OK now it makes sense.

Maybe the EXIT_FAILURE can be done in a new series, calling 'sed' to 
update the full repository.
Igor Mammedov Jan. 16, 2020, 4:34 p.m. UTC | #5 
On Thu, 16 Jan 2020 16:14:45 +0100
Philippe Mathieu-Daudé <philmd@redhat.com> wrote:

> On 1/15/20 10:59 PM, BALATON Zoltan wrote:
> > On Wed, 15 Jan 2020, Philippe Mathieu-Daudé wrote:  
> >> On 1/15/20 7:15 PM, BALATON Zoltan wrote:  
> >>> On Wed, 15 Jan 2020, Igor Mammedov wrote:  
> >>>> From: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>>
> >>>> The firmware has to reside in the PDC range. If the Elf file
> >>>> expects to load it below FIRMWARE_START, it is incorrect,
> >>>> regardless the RAM size.
> >>>>
> >>>> Acked-by: Helge Deller <deller@gmx.de>
> >>>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >>>> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> >>>> Signed-off-by: Igor Mammedov <imammedo@redhat.com>
> >>>> ---
> >>>> hw/hppa/machine.c | 2 +-
> >>>> 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>>
> >>>> diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
> >>>> index 5d0de26..6775d87 100644
> >>>> --- a/hw/hppa/machine.c
> >>>> +++ b/hw/hppa/machine.c
> >>>> @@ -155,7 +155,7 @@ static void machine_hppa_init(MachineState 
> >>>> *machine)
> >>>>     qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
> >>>>                   "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
> >>>>                   firmware_low, firmware_high, firmware_entry);
> >>>> -    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
> >>>> +    if (firmware_low < FIRMWARE_START || firmware_high >= 
> >>>> FIRMWARE_END) {
> >>>>         error_report("Firmware overlaps with memory or IO space");
> >>>>         exit(1);  
> >>>
> >>> Should this also be EXIT_FAILURE like in other places when you're 
> >>> changing the line nearby?  
> >>
> >> I didn't changed this line, this seems unrelated to the patch purpose.  
> > 
> > Fair enough. Just thought because there was patch 85/86 making that 
> > change to keep consistency. Maybe you can change this in that patch but 
> > I don't really mind just spotted it.  
> 
> Ah this is because it is a patch of mine included in Igor series, and 
> Igor uses EXIT_FAILURE in his other patches, OK now it makes sense.
> 
> Maybe the EXIT_FAILURE can be done in a new series, calling 'sed' to 
> update the full repository.

I'll fix it up on respin

>
diff mbox series

Patch

diff --git a/hw/hppa/machine.c b/hw/hppa/machine.c
index 5d0de26..6775d87 100644
--- a/hw/hppa/machine.c
+++ b/hw/hppa/machine.c
@@ -155,7 +155,7 @@  static void machine_hppa_init(MachineState *machine)
     qemu_log_mask(CPU_LOG_PAGE, "Firmware loaded at 0x%08" PRIx64
                   "-0x%08" PRIx64 ", entry at 0x%08" PRIx64 ".\n",
                   firmware_low, firmware_high, firmware_entry);
-    if (firmware_low < ram_size || firmware_high >= FIRMWARE_END) {
+    if (firmware_low < FIRMWARE_START || firmware_high >= FIRMWARE_END) {
         error_report("Firmware overlaps with memory or IO space");
         exit(1);
     }