* [PATCH] q800: fix segfault with invalid MacROM
@ 2022-01-06 12:22 Laurent Vivier
2022-01-06 12:38 ` Philippe Mathieu-Daudé
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Laurent Vivier @ 2022-01-06 12:22 UTC (permalink / raw)
To: qemu-devel
Cc: Peter Maydell, Thomas Huth, Philippe Mathieu-Daudé, 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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] q800: fix segfault with invalid MacROM
2022-01-06 12:22 [PATCH] q800: fix segfault with invalid MacROM Laurent Vivier
@ 2022-01-06 12:38 ` Philippe Mathieu-Daudé
2022-01-07 7:23 ` Thomas Huth
2022-01-07 8:15 ` Mark Cave-Ayland
2 siblings, 0 replies; 7+ messages in thread
From: Philippe Mathieu-Daudé @ 2022-01-06 12:38 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: Peter Maydell, Thomas Huth
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] q800: fix segfault with invalid MacROM
2022-01-06 12:22 [PATCH] q800: fix segfault with invalid MacROM Laurent Vivier
2022-01-06 12:38 ` Philippe Mathieu-Daudé
@ 2022-01-07 7:23 ` Thomas Huth
2022-01-07 8:15 ` Mark Cave-Ayland
2 siblings, 0 replies; 7+ messages in thread
From: Thomas Huth @ 2022-01-07 7:23 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel; +Cc: Peter Maydell, Philippe Mathieu-Daudé
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>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] q800: fix segfault with invalid MacROM
2022-01-06 12:22 [PATCH] q800: fix segfault with invalid MacROM Laurent Vivier
2022-01-06 12:38 ` Philippe Mathieu-Daudé
2022-01-07 7:23 ` Thomas Huth
@ 2022-01-07 8:15 ` Mark Cave-Ayland
2022-01-07 8:55 ` Laurent Vivier
2 siblings, 1 reply; 7+ messages in thread
From: Mark Cave-Ayland @ 2022-01-07 8:15 UTC (permalink / raw)
To: Laurent Vivier, qemu-devel
Cc: Peter Maydell, Thomas Huth, Philippe Mathieu-Daudé
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] q800: fix segfault with invalid MacROM
2022-01-07 8:15 ` Mark Cave-Ayland
@ 2022-01-07 8:55 ` Laurent Vivier
2022-01-07 9:47 ` BALATON Zoltan
0 siblings, 1 reply; 7+ messages in thread
From: Laurent Vivier @ 2022-01-07 8:55 UTC (permalink / raw)
To: Mark Cave-Ayland, qemu-devel
Cc: Peter Maydell, Thomas Huth, Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] q800: fix segfault with invalid MacROM
2022-01-07 8:55 ` Laurent Vivier
@ 2022-01-07 9:47 ` BALATON Zoltan
2022-01-07 10:00 ` Laurent Vivier
0 siblings, 1 reply; 7+ messages in thread
From: BALATON Zoltan @ 2022-01-07 9:47 UTC (permalink / raw)
To: Laurent Vivier
Cc: Peter Maydell, Thomas Huth, Mark Cave-Ayland, qemu-devel,
Philippe Mathieu-Daudé
[-- 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] q800: fix segfault with invalid MacROM
2022-01-07 9:47 ` BALATON Zoltan
@ 2022-01-07 10:00 ` Laurent Vivier
0 siblings, 0 replies; 7+ messages in thread
From: Laurent Vivier @ 2022-01-07 10:00 UTC (permalink / raw)
To: BALATON Zoltan
Cc: Peter Maydell, Thomas Huth, Mark Cave-Ayland, qemu-devel,
Philippe Mathieu-Daudé
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2022-01-07 10:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-06 12:22 [PATCH] q800: fix segfault with invalid MacROM Laurent Vivier
2022-01-06 12:38 ` Philippe Mathieu-Daudé
2022-01-07 7:23 ` Thomas Huth
2022-01-07 8:15 ` Mark Cave-Ayland
2022-01-07 8:55 ` Laurent Vivier
2022-01-07 9:47 ` BALATON Zoltan
2022-01-07 10:00 ` Laurent Vivier
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.