[PATCH] q800: fix segfault with invalid MacROM

[PATCH] q800: fix segfault with invalid MacROM

[Laurent Vivier]

"qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
in q800_init().
This happens because the code doesn't check that rom_ptr() returned
a non-NULL pointer .

Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
Reported-by: Peter Maydell <peter.maydell@linaro.org>
Signed-off-by: Laurent Vivier <laurent@vivier.eu>
---
 hw/m68k/q800.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
index e4c7c9b88ad0..6261716c8f7e 100644
--- a/hw/m68k/q800.c
+++ b/hw/m68k/q800.c
@@ -672,10 +672,16 @@ static void q800_init(MachineState *machine)
 
         /* Remove qtest_enabled() check once firmware files are in the tree */
         if (!qtest_enabled()) {
-            if (bios_size < 0 || bios_size > MACROM_SIZE) {
+            if (bios_size == -1) {
                 error_report("could not load MacROM '%s'", bios_name);
                 exit(1);
             }
+            if (bios_size != MACROM_SIZE) {
+                error_report("Invalid size for MacROM '%s': %d bytes,"
+                             " expected %d bytes", bios_name, bios_size,
+                             MACROM_SIZE);
+                exit(1);
+            }
 
             ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
             stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */
-- 
2.33.1

Re: [PATCH] q800: fix segfault with invalid MacROM

[Philippe Mathieu-Daudé]

On 6/1/22 13:22, Laurent Vivier wrote:
> "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault

Typo "crashes".

> in q800_init().
> This happens because the code doesn't check that rom_ptr() returned
> a non-NULL pointer .
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/m68k/q800.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH] q800: fix segfault with invalid MacROM

[Thomas Huth]

On 06/01/2022 13.22, Laurent Vivier wrote:
> "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
> in q800_init().
> This happens because the code doesn't check that rom_ptr() returned
> a non-NULL pointer .
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/m68k/q800.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index e4c7c9b88ad0..6261716c8f7e 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -672,10 +672,16 @@ static void q800_init(MachineState *machine)
>   
>           /* Remove qtest_enabled() check once firmware files are in the tree */
>           if (!qtest_enabled()) {
> -            if (bios_size < 0 || bios_size > MACROM_SIZE) {
> +            if (bios_size == -1) {
>                   error_report("could not load MacROM '%s'", bios_name);
>                   exit(1);
>               }
> +            if (bios_size != MACROM_SIZE) {
> +                error_report("Invalid size for MacROM '%s': %d bytes,"
> +                             " expected %d bytes", bios_name, bios_size,
> +                             MACROM_SIZE);
> +                exit(1);
> +            }
>   
>               ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
>               stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH] q800: fix segfault with invalid MacROM

[Mark Cave-Ayland]

On 06/01/2022 12:22, Laurent Vivier wrote:

> "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
> in q800_init().
> This happens because the code doesn't check that rom_ptr() returned
> a non-NULL pointer .
> 
> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
> Reported-by: Peter Maydell <peter.maydell@linaro.org>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
>   hw/m68k/q800.c | 8 +++++++-
>   1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
> index e4c7c9b88ad0..6261716c8f7e 100644
> --- a/hw/m68k/q800.c
> +++ b/hw/m68k/q800.c
> @@ -672,10 +672,16 @@ static void q800_init(MachineState *machine)
>   
>           /* Remove qtest_enabled() check once firmware files are in the tree */
>           if (!qtest_enabled()) {
> -            if (bios_size < 0 || bios_size > MACROM_SIZE) {
> +            if (bios_size == -1) {
>                   error_report("could not load MacROM '%s'", bios_name);
>                   exit(1);
>               }
> +            if (bios_size != MACROM_SIZE) {
> +                error_report("Invalid size for MacROM '%s': %d bytes,"
> +                             " expected %d bytes", bios_name, bios_size,
> +                             MACROM_SIZE);
> +                exit(1);
> +            }
>   
>               ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
>               stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */

The patch does fix the issue, but it seems a little odd that you can't use -bios 
path/to/m68k-binary to boot with an arbitrary sized binary which could be useful for 
reproducers such as https://gitlab.com/qemu-project/qemu/-/issues/360.

How easy would it be to add the extra rom_ptr() NULL check instead?


ATB,

Mark.

Re: [PATCH] q800: fix segfault with invalid MacROM

[Laurent Vivier]

Le 07/01/2022 à 09:15, Mark Cave-Ayland a écrit :
> On 06/01/2022 12:22, Laurent Vivier wrote:
> 
>> "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
>> in q800_init().
>> This happens because the code doesn't check that rom_ptr() returned
>> a non-NULL pointer .
>>
>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>>   hw/m68k/q800.c | 8 +++++++-
>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>
>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>> index e4c7c9b88ad0..6261716c8f7e 100644
>> --- a/hw/m68k/q800.c
>> +++ b/hw/m68k/q800.c
>> @@ -672,10 +672,16 @@ static void q800_init(MachineState *machine)
>>           /* Remove qtest_enabled() check once firmware files are in the tree */
>>           if (!qtest_enabled()) {
>> -            if (bios_size < 0 || bios_size > MACROM_SIZE) {
>> +            if (bios_size == -1) {
>>                   error_report("could not load MacROM '%s'", bios_name);
>>                   exit(1);
>>               }
>> +            if (bios_size != MACROM_SIZE) {
>> +                error_report("Invalid size for MacROM '%s': %d bytes,"
>> +                             " expected %d bytes", bios_name, bios_size,
>> +                             MACROM_SIZE);
>> +                exit(1);
>> +            }
>>               ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
>>               stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */
> 
> The patch does fix the issue, but it seems a little odd that you can't use -bios path/to/m68k-binary 
> to boot with an arbitrary sized binary which could be useful for reproducers such as 
> https://gitlab.com/qemu-project/qemu/-/issues/360.
> 
> How easy would it be to add the extra rom_ptr() NULL check instead?
> 

I was thinking that a smaller binary can be padded to 1 MB for use because on a real hardware the 
size of the ROM cannot be arbitrary.

But it seems reasonable to check only for the NULL pointer rather than the size, I'm going to send a v2.

Thanks,
Laurent

Re: [PATCH] q800: fix segfault with invalid MacROM

[BALATON Zoltan]

[-- Attachment #1: Type: text/plain, Size: 2571 bytes --]

On Fri, 7 Jan 2022, Laurent Vivier wrote:
> Le 07/01/2022 à 09:15, Mark Cave-Ayland a écrit :
>> On 06/01/2022 12:22, Laurent Vivier wrote:
>> 
>>> "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
>>> in q800_init().
>>> This happens because the code doesn't check that rom_ptr() returned
>>> a non-NULL pointer .
>>> 
>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>>   hw/m68k/q800.c | 8 +++++++-
>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>> 
>>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>>> index e4c7c9b88ad0..6261716c8f7e 100644
>>> --- a/hw/m68k/q800.c
>>> +++ b/hw/m68k/q800.c
>>> @@ -672,10 +672,16 @@ static void q800_init(MachineState *machine)
>>>           /* Remove qtest_enabled() check once firmware files are in the 
>>> tree */
>>>           if (!qtest_enabled()) {
>>> -            if (bios_size < 0 || bios_size > MACROM_SIZE) {
>>> +            if (bios_size == -1) {
>>>                   error_report("could not load MacROM '%s'", bios_name);
>>>                   exit(1);
>>>               }
>>> +            if (bios_size != MACROM_SIZE) {
>>> +                error_report("Invalid size for MacROM '%s': %d bytes,"
>>> +                             " expected %d bytes", bios_name, bios_size,
>>> +                             MACROM_SIZE);
>>> +                exit(1);
>>> +            }
>>>               ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
>>>               stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */
>> 
>> The patch does fix the issue, but it seems a little odd that you can't use 
>> -bios path/to/m68k-binary to boot with an arbitrary sized binary which 
>> could be useful for reproducers such as 
>> https://gitlab.com/qemu-project/qemu/-/issues/360.
>> 
>> How easy would it be to add the extra rom_ptr() NULL check instead?
>> 
>
> I was thinking that a smaller binary can be padded to 1 MB for use because on 
> a real hardware the size of the ROM cannot be arbitrary.
>
> But it seems reasonable to check only for the NULL pointer rather than the 
> size, I'm going to send a v2.

Instead of adding !rom_ptr as well, isn't it enough to change to
bios_size <= 0 
in the existing check?

Regards,
BALATON Zoltan

Re: [PATCH] q800: fix segfault with invalid MacROM

[Laurent Vivier]

Le 07/01/2022 à 10:47, BALATON Zoltan a écrit :
> On Fri, 7 Jan 2022, Laurent Vivier wrote:
>> Le 07/01/2022 à 09:15, Mark Cave-Ayland a écrit :
>>> On 06/01/2022 12:22, Laurent Vivier wrote:
>>>
>>>> "qemu-system-m68k -M q800 -bios /dev/null" crahses with a segfault
>>>> in q800_init().
>>>> This happens because the code doesn't check that rom_ptr() returned
>>>> a non-NULL pointer .
>>>>
>>>> Resolves: https://gitlab.com/qemu-project/qemu/-/issues/756
>>>> Reported-by: Peter Maydell <peter.maydell@linaro.org>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>>   hw/m68k/q800.c | 8 +++++++-
>>>>   1 file changed, 7 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/hw/m68k/q800.c b/hw/m68k/q800.c
>>>> index e4c7c9b88ad0..6261716c8f7e 100644
>>>> --- a/hw/m68k/q800.c
>>>> +++ b/hw/m68k/q800.c
>>>> @@ -672,10 +672,16 @@ static void q800_init(MachineState *machine)
>>>>           /* Remove qtest_enabled() check once firmware files are in the tree */
>>>>           if (!qtest_enabled()) {
>>>> -            if (bios_size < 0 || bios_size > MACROM_SIZE) {
>>>> +            if (bios_size == -1) {
>>>>                   error_report("could not load MacROM '%s'", bios_name);
>>>>                   exit(1);
>>>>               }
>>>> +            if (bios_size != MACROM_SIZE) {
>>>> +                error_report("Invalid size for MacROM '%s': %d bytes,"
>>>> +                             " expected %d bytes", bios_name, bios_size,
>>>> +                             MACROM_SIZE);
>>>> +                exit(1);
>>>> +            }
>>>>               ptr = rom_ptr(MACROM_ADDR, MACROM_SIZE);
>>>>               stl_phys(cs->as, 0, ldl_p(ptr));    /* reset initial SP */
>>>
>>> The patch does fix the issue, but it seems a little odd that you can't use -bios 
>>> path/to/m68k-binary to boot with an arbitrary sized binary which could be useful for reproducers 
>>> such as https://gitlab.com/qemu-project/qemu/-/issues/360.
>>>
>>> How easy would it be to add the extra rom_ptr() NULL check instead?
>>>
>>
>> I was thinking that a smaller binary can be padded to 1 MB for use because on a real hardware the 
>> size of the ROM cannot be arbitrary.
>>
>> But it seems reasonable to check only for the NULL pointer rather than the size, I'm going to send 
>> a v2.
> 
> Instead of adding !rom_ptr as well, isn't it enough to change to
> bios_size <= 0 in the existing check?
>

I agree. And to change rom_ptr(MACROM_ADDR, MACROM_SIZE) to rom_ptr(MACROM_ADDR, bios_size)

Thanks,
Laurent