Message ID | 20220106122247.771454-1-laurent@vivier.eu |
---|---|
State | New |
Headers | show |
Series | q800: fix segfault with invalid MacROM | expand |
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>
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>
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.
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
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
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
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 */
"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(-)